https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/GWToolset

on friday, 2013-11-01 we agreed that the tentative review date will be monday, 2013-11-11 depending on the mvp status, architecture review and chris’ schedule.

Correct, I'm targeting 11/11 as a start date for the review, which I should be able finish sometime that week. If you can have everything merged by then, that would help.

Hi Dan,

I'm working through the review. There are a couple of things I'd like to see fixed at their root, so I don't have to document every occurrence, so this is just a partial list for now. Let me know if you have questions.

includes/Php/Filter.php

• $filter_sanitize === null in sanitize() should not be allowed. Since you use Filter::evaluate() to sanitize for xss in several places, there can't be a chance that would return without any filtering. If you can remove setting a custom filter, that would be even better.
• Split array handling into another class that calls the scalar version, so all filtering logic is in one class, but when you filter, it's explicit if you're expecting an array or scalar.

includes/Handlers/Forms/FormHandler.php
Line 109: getForm is called without validating the class beyond existence. Please check this is a form class you're expecting (have them all inherit and check instanceof?)

includes/Handlers/Forms/MetadataDetectHandler.php

• getUserOptions() - doing urldecode after filtering invalidates filtering
• You have MAX_FILE_SIZE come in from the form, although it looks like you never actually use it. You should obviously never rely on user-submitted data to check sizes. You can probably just remove that field, in case another developer decides to trust it?

includes/functions/functions.php

• is there a reason these aren't static methods in a utility class?
• Attack can get arbitrary values through getWhitelistedPost by passing in a whitelisted name as an array with 'filter-sanitize'=null. Fixing Filter should prevent this from being an issue.

includes/Specials/SpecialGWToolset.php

• Line 143: Please log, don't print_r
• Line 164: I would make things easier if you could escape the exception message, or demonstrate that it's been properly sanitized. Otherwise all exceptions have to be sanitized for xss (more places to get something wrong).

includes/Handlers/Xml/XmlHandler.php

• XMLReader must disable external entity loading before reading xml

• filter: https://gerrit.wikimedia.org/r/#/c/97051/
• functions: https://gerrit.wikimedia.org/r/#/c/97062/
• form-handler: https://gerrit.wikimedia.org/r/#/c/97369/
• max-file-size: https://gerrit.wikimedia.org/r/#/c/97465/
• external-entity: https://gerrit.wikimedia.org/r/#/c/97521/

• sanitized-exceptions: https://gerrit.wikimedia.org/r/#/c/97676/

includes/Helpers/FileChecks.php

• mimeTypeAndExtensionMatch doesn't ensure mime = extension. mimeTypeAndExtensionMatch() should use MimeMagic::isMatchingExtension()
• It looks like this is only used for the xml metadata. If so, can you please document that in the code?

includes/Jobs/UploadMetadataJob.php

• The variables put into the $_POST global state can be anything, since the whitelisting for MetadataMappingHandler is based on the templates, which are user-controlled. If you need arbitrary values to be input from the form, you need to use something other than $_POST to pass the state to the dependent objects.

includes/Php/File.php

• Use MimeMagic for mimetype detection

includes/Handlers/UploadHandler.php

• Remove augmentAllowedExtensions if it's unused

includes/Handlers/Xml/XmlDetectHandler.php

• getButtonRowNoMetadata, getFirstRow use Sanitizer::escapeId for html id attr
• xml_validator.asp needs to be an external link

includes/Handlers/Forms/MetadataMappingHandler.php

• no access control on who can access files in the backend?

includes/Forms/PreviewForm.php

• Can getPostAsHiddenFields only return a whitelist of fields?

includes/Adapters/Php/MediawikiTemplatePhpAdapter.php

• Should use a derivative request object instead of curling the api. If this isn't possible for some reason, it needs to use https instead of http.

includes/Adapters/Php/MappingPhpAdapter.php

• Needs to check of the user has access, or get FOR_PUBLIC

Not security related

table-create-gwtoolset-mediawiki-templates.sql

• missing hooks for update.php

includes/Handlers/Forms/MetadataMappingHandler.php

• Checking for PHP_SAPI = 'cli' seems like the wrong way to check if this is running from a job

ext.gwtoolset.js

• Line 311: Don't include error message in that string, since it's sent to .html().

• js-error-output: https://gerrit.wikimedia.org/r/#/c/98686/
• mime-type: https://gerrit.wikimedia.org/r/#/c/98725/
• table-create: https://gerrit.wikimedia.org/r/#/c/98735/
• sanitizer-escape-id: https://gerrit.wikimedia.org/r/#/c/98742/
• derivative-request: https://gerrit.wikimedia.org/r/#/c/98807/
• augmentAllowedExtensions: https://gerrit.wikimedia.org/r/#/c/98812/
• PreviewForm: https://gerrit.wikimedia.org/r/#/c/98825/
• FOR_PUBLIC: https://gerrit.wikimedia.org/r/#/c/98835/
• original-post: https://gerrit.wikimedia.org/r/#/c/98846/

MetadataMappingHandler.php

no access control on who can access files in the backend.

as far as i can tell there is no way to restrict MediaWiki User access similar to the way UploadStash does. the only “security” i’ve seen is via the FileBackend->prepare() and FileBackend->secure() methods. the code is already implementing the prepare() method.

1. is there a way to secure the files per User?
2. is that necessary?
3. if there’s no way to do it yet and it’s necessary, how do we move forward?

filebackend-user-access: https://gerrit.wikimedia.org/r/#/c/99387/

I think Dan's addressed everything. LGTM.

(In reply to comment #11)

I think Dan's addressed everything. LGTM.

Is this bug resolved/fixed then? :-)

(In reply to comment #12)

(In reply to comment #11)

I think Dan's addressed everything. LGTM.

Is this bug resolved/fixed then? :-)

Yep :)