Page MenuHomePhabricator

Security review of Extension:3d
Closed, ResolvedPublic

Description

Please review the 3d extension.

This extension enables upload of, thumbnailing, and interactive viewing of three-dimensional model files, currently only the STL file format, but with the potential for expansion based on request.

We intend to enable this on beta-commons, then the testwikis, then on Commons to see how file uploaders make use of it. If all of those experiments go well, and we are able to respond to requests, we will push the extension out to other wikis to allow use of models across the projects.

@MarkTraceur is the primary contact. The Multimedia team is generally responsible for the project. @Jdforrester-WMF
is the product manager.

Target dates for deployments: 2017-03-01 for beta, 1-2 weeks later for test, 1-2 weeks later for Commons.

The command-line utility used for thumbnailing, 3d2png, was already reviewed - T132063

Languages: JavaScript, a tiny bit of PHP

Repository: https://phabricator.wikimedia.org/source/3d/

https://www.mediawiki.org/wiki/Extension:3d

Thanks!

Details

Related Gerrit Patches:
mediawiki/extensions/3D : masterFix security/general concerns

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 2 2017, 9:46 PM
Jopparn added a subscriber: Jopparn.Feb 6 2017, 2:12 PM
Reedy added a subscriber: Reedy.Feb 23 2017, 12:26 AM

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

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.

Reedy added a comment.EditedFeb 26 2017, 12:53 AM

ThreeDHandler.php has no security concerns

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

PHP code

  • 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)

JS

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
Reedy added a project: 3D.Feb 26 2017, 12:54 AM
Reedy added a comment.Feb 26 2017, 4:46 PM

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

Reedy added a comment.Feb 26 2017, 5:20 PM

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

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?

Dereckson added a subscriber: Dereckson.EditedMar 7 2017, 3:16 AM

@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.

@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.

Reedy closed this task as Resolved.Mar 7 2017, 2:45 PM
Reedy claimed this task.
Restricted Application added a project: Multimedia. · View Herald TranscriptJul 9 2019, 8:27 PM