As an aside, the repo needs pushing to gerrit for deployment to WMF wikis... AFAIK we can't branch from Phab...

In T157077#3048611, @Reedy wrote:

As an aside, the repo needs pushing to gerrit for deployment to WMF wikis... AFAIK we can't branch from Phab...

That was filed as T158881: Create repo for extension 3d and import into gerrit.

ThreeDHandler.php has no security concerns

Other non security specific (dumping here due to lack of more general "review" task) issues...

PHP code

• https://phabricator.wikimedia.org/source/3d/browse/master/ThreeDHandler.php;f5dbb9ecfac876eda0d4c1129272147a5dc9b1bc$104 $removed is undefined

• https://phabricator.wikimedia.org/source/3d/browse/master/ThreeDHandler.php;f5dbb9ecfac876eda0d4c1129272147a5dc9b1bc$101 should the memory limit be factored out into some global, with a "sane" default set? Like we do with various other things like that. Or, just reuse $wgMaxShellMemory

• ThreeDHooks.php has a hook that doesn't do anything. If it's gonna be used, great, use it. If not, remove it, and probably remove the whole class as aswell as it has no use (at the moment, at least)

• https://phabricator.wikimedia.org/source/3d/browse/master/ThreeDHandler.php;f5dbb9ecfac876eda0d4c1129272147a5dc9b1bc$44 inconsistent comments

JS

• https://phabricator.wikimedia.org/source/3d/browse/master/modules/mmv.3d.js;f5dbb9ecfac876eda0d4c1129272147a5dc9b1bc$2 -- Fairly sure it's in the 3d extension
• three.js is MIT licence, as is seemingly the rest of the extension... But see T157409 as there are other inconsistencies
• three.js is on r75, r84 is out. https://github.com/mrdoob/three.js/compare/r75...r84 - No obvious signs of security issues fixed (or, if they were, they're not highlighted). Worth looking if we should upgrade now/in the near future
  • T107561 of course would make this nicer rather than copying in JS files
• cf T132063#2435663 do we want JSZip? And did the other comments get addressed?

General

• tests folder contents should use tests/phpunit and tests/qunit for autoloading goodness, at least, on phpunit. Or is that manifest_version 2? Either way, better structure, even if we don't use the test autoloading
• both test files are empty though

Oh, another one from the PHP...

https://phabricator.wikimedia.org/source/3d/browse/master/ThreeDHandler.php;f5dbb9ecfac876eda0d4c1129272147a5dc9b1bc$83 both $height and $width are undefined. They don't come part of those variables till 86/87

en.json, qqq.json and extension.json are all space indented, should be tabs

mmv.3d.js

• lots of == should they be === ? Type coercion etc. I notice we mostly seem to use triple equals in other MW JS code
• I keep thinking threed is a typo for thread (due to all lower case)

There's various == that maybe should be === in Three too?

Security:

• Should three/AMFLoader.js still be about? It was removed from 3d2png in rTDTPf8be12e34ce91fed5caa728e3597dfebb46818b3 due to security concerns with XML formats. And similar, if not, should we remove the amf code from mmv.3d.js; no point it being around otherwise

General

• three.js is hell to even try and code review. Over 40k lines of javascript? And I notice, it seems to be comprised of various individual files. I guess it came from here? My browser doesn't like it, and neither does PhpStorm
  • https://github.com/mrdoob/three.js/blob/dev/src/Three.js vs https://phabricator.wikimedia.org/source/3d/browse/master/modules/three/three.js vs https://github.com/mrdoob/three.js/blob/r75/src/Three.js
  • Can we just include them 1:1 from upstream? Again, T107561 would be a much nicer way to fix/include these for sure

Disclaimer: As most of you know, I'm not a JS developer.... I don't see anything obviously wrong from a security point of view. But there are some more general improvements that should be made to the extension with this and the last few comments to fix various other issues

D580 should fix these concerns, but I skipped a couple as noted in the commit message.

Change 340330 had a related patch set uploaded (by Jforrester; owner: MarkTraceur):
Fix security/general concerns

https://gerrit.wikimedia.org/r/340330

Change 340330 merged by Jforrester:
[mediawiki/extensions/3D] Fix security/general concerns

https://gerrit.wikimedia.org/r/340330

Could deployment to Commons please be delayed until some tools etc. has been created to deal with copyvio detection and protection work etc.? Right now Commons users does not have any tools to detect if a 3d file is a copyvio (code-wise), nor an easy way to check if is a 3d-derviative of something copyrightable, or of other 3D-files.

Can some time be spend by the Multimedia team be spent on helping the community finding ways to detect bad and "stolen" 3d-works/file, before deploying this live on Commons?

@Josve05a The security review isn't a good fit to discuss deployment strategy. It seems T132058 is used for coordination. I've copied your comment there.

In T157077#3078624, @Dereckson wrote:

@Josve05a The security review isn't a good fit to discuss deployment strategy. It seems T132058 is used for coordination. I've copied your comment there.

Oh, I had that task open in another tab, and wrote my comment in the wrong tab. Sorry 'bout that.