Page MenuHomePhabricator
Create Task
Maniphest T184643

Security review for GlobalPreferences
Closed, ResolvedPublic
Actions

Description

Project Information

Description of the tool/project

GlobalPreferences is a MediaWiki extension that allows users to share their preferences across all wikis on an opt-in basis.

Description of how the tool will be used at WMF

Will be deployed on all SUL wikis.

Dependencies

MediaWiki and CentralAuth.

Has this project been reviewed before?

No.

Working test environment

Use the globalpreferences role for Vagrant.

Post-deployment

Will be maintained by Comunity Tech.

Details

SubjectRepoBranchLines +/-
Address security reviewmediawiki/extensions/GlobalPreferencesmaster+16 -14
Customize query in gerrit

Related Objects
Search...

Event Timeline

MaxSem created this task.Jan 10 2018, 7:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 10 2018, 7:37 PM
MaxSem added a parent task: T184121: Deploy checklist for GlobalPreferences on production.Jan 10 2018, 7:37 PM
MaxSem mentioned this in T184121: Deploy checklist for GlobalPreferences on production.
MaxSem added a project: MediaWiki-extensions-GlobalPreferences.Jan 10 2018, 7:40 PM
MaxSem added a parent task: T184668: Deploy GlobalPreferences on beta cluster.Jan 11 2018, 12:31 AM
TerraCodes subscribed.Jan 14 2018, 11:26 AM
kaldari moved this task from New & TBD Tickets to CL, QA, Data analysis backlog on the Community-Tech board.Jan 17 2018, 12:27 AM
MarcoAurelio subscribed.Jan 29 2018, 9:51 AM
MaxSem updated the task description. (Show Details)Feb 8 2018, 12:07 AM
• TBolliger added a subscriber: • JBennett.Feb 13 2018, 1:22 AM
• TBolliger subscribed.
Bawolff claimed this task.Feb 13 2018, 12:15 PM
Bawolff moved this task from Incoming to In Progress on the deprecated-security-team-reviews board.
Bawolff removed Bawolff as the assignee of this task.Feb 13 2018, 4:04 PM
Bawolff moved this task from In Progress to Waiting on the deprecated-security-team-reviews board.
Bawolff subscribed.

Overall looks good. Review passed. Some small minor things:

  • In JS - for jquery attribute selectors like $( this ).add( 'label[for="' + $( this ).attr( 'id' ) + '"]' ).on( { - it Would be nice if it did $( this ).attr( 'id' ).replace( /[\\"]/g, '\\$&' ) just in case, eventhough the attribute id is not user controlled.
  • "ext.GlobalPreferences.global.js" line 96 - Avoid using raw html messages - mw.message( 'globalprefs-select-all' ) should be escaped.
  • "GlobalPreferencesFactory.php" line 166 - double escaping globalprefs-info-link message
  • "GlobalPreferencesForm.php" line 86 - double escaping. Should set message format ->text().
  • "SpecialGlobalPreferences.php" line 47 - double escaping.

One thing I wasn't sure about:

  • "Hooks.php" line 169 - I'm a little confused why PreferencesFactory redefiner callback is reinitializing $wgContLang. Are we sure that's correct?
kaldari closed this task as Resolved.Feb 13 2018, 5:10 PM
kaldari claimed this task.
kaldari mentioned this in T187218: GlobalPreferences clean-up from Security Review.
Niharika subscribed.Feb 13 2018, 7:13 PM

Thanks @Bawolff!

Niharika moved this task from CL, QA, Data analysis backlog to Archive on the Community-Tech board.Feb 13 2018, 7:14 PM
gerritbot subscribed.Feb 13 2018, 9:59 PM

Change 410343 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/GlobalPreferences@master] Address security review

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

gerritbot added a project: Patch-For-Review.Feb 13 2018, 9:59 PM
Samwilson added a comment.Feb 14 2018, 3:45 AM
In T184643#3967872, @Bawolff wrote:

One thing I wasn't sure about:

  • "Hooks.php" line 169 - I'm a little confused why PreferencesFactory redefiner callback is reinitializing $wgContLang. Are we sure that's correct?

Yes, it's hardly ideal, but it's because the services are initialized (and therefore the MediaWikiServices hook run) before $wgContLang is defined in Setup.php. This hook call basically has to replicate what's done for this service in ServiceWiring.php.

I think the actual fix here is T153256, because then the hook call won't have to actually instantiate the service but only define it. At the moment, if it doesn't also do $services->getPreferencesFactory(); then the service is overwritten.

gerritbot added a comment.Feb 14 2018, 5:36 AM

Change 410343 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Address security review

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

• 238482n375 removed kaldari as the assignee of this task.Jun 15 2018, 8:04 AM
• 238482n375 triaged this task as Lowest priority.
• 238482n375 moved this task from Next Up to In Code Review on the Analytics-Kanban board.
• 238482n375 added projects: Analytics-Kanban, acl*security, Wikimedia-VE-Campaigns (S2-2018), Scap (Scap3-Adoption-Phase2), AbuseFilter, Data-release, Hashtags, LabsDB-Auditor, Ladies-That-FOSS-MediaWiki, Language-2018-Apr-June, Language-2018-Jan-Mar, HHVM, HAWelcome.
• 238482n375 edited subscribers, added: kaldari, • 238482n375; removed: Aklapper.

SG9tZVBoYWJyaWNhdG9yCk5vIG1lc3NhZ2VzLiBObyBub3RpZmljYXRpb25zLgoKICAgIFNlYXJjaAoKQ3JlYXRlIFRhc2sKTWFuaXBoZXN0ClQxOTcyODEKRml4IGZhaWxpbmcgd2VicmVxdWVzdCBob3VycyAodXBsb2FkIGFuZCB0ZXh0IDIwMTgtMDYtMTQtMTEpCk9wZW4sIE5lZWRzIFRyaWFnZVB1YmxpYwoKICAgIEVkaXQgVGFzawogICAgRWRpdCBSZWxhdGVkIFRhc2tzLi4uCiAgICBFZGl0IFJlbGF0ZWQgT2JqZWN0cy4uLgogICAgUHJvdGVjdCBhcyBzZWN1cml0eSBpc3N1ZQoKICAgIE11dGUgTm90aWZpY2F0aW9ucwogICAgQXdhcmQgVG9rZW4KICAgIEZsYWcgRm9yIExhdGVyCgpUYWdzCgogICAgQW5hbHl0aWNzLUthbmJhbiAoSW4gUHJvZ3Jlc3MpCgpTdWJzY3JpYmVycwpBa2xhcHBlciwgSkFsbGVtYW5kb3UKQXNzaWduZWQgVG8KSkFsbGVtYW5kb3UKQXV0aG9yZWQgQnkKSkFsbGVtYW5kb3UsIEZyaSwgSnVuIDE1CkRlc2NyaXB0aW9uCgpPb3ppZSBqb2JzIGhhdmUgYmVlbiBmYWlsaW5nIGF0IGxlYXN0IGEgZmV3IHRpbWVzIGVhY2guIE1vcmUgaW52ZXN0aWdhdGlvbiBuZWVkZWQuCkpBbGxlbWFuZG91IGNyZWF0ZWQgdGhpcyB0YXNrLkZyaSwgSnVuIDE1LCA3OjIxIEFNCkhlcmFsZCBhZGRlZCBhIHN1YnNjcmliZXI6IEFrbGFwcGVyLiC3IFZpZXcgSGVyYWxkIFRyYW5zY3JpcHRGcmksIEp1biAxNSwgNzoyMSBBTQpKQWxsZW1hbmRvdSBjbGFpbWVkIHRoaXMgdGFzay5GcmksIEp1biAxNSwgNzoyMiBBTQpKQWxsZW1hbmRvdSB1cGRhdGVkIHRoZSB0YXNrIGRlc2NyaXB0aW9uLiAoU2hvdyBEZXRhaWxzKQpKQWxsZW1hbmRvdSBhZGRlZCBhIHByb2plY3Q6IEFuYWx5dGljcy1LYW5iYW4uCkpBbGxlbWFuZG91IG1vdmVkIHRoaXMgdGFzayBmcm9tIE5leHQgVXAgdG8gSW4gUHJvZ3Jlc3Mgb24gdGhlIEFuYWx5dGljcy1LYW5iYW4gYm9hcmQuCkNoYW5nZSBTdWJzY3JpYmVycwpDaGFuZ2UgUHJpb3JpdHkKQXNzaWduIC8gQ2xhaW0KTW92ZSBvbiBXb3JrYm9hcmQKQ2hhbmdlIFByb2plY3QgVGFncwpBbmFseXRpY3MtS2FuYmFuCtcKU2VjdXJpdHkK1wpXaWtpbWVkaWEtVkUtQ2FtcGFpZ25zIChTMi0yMDE4KQrXClNjYXAK1wpTY2FwIChTY2FwMy1BZG9wdGlvbi1QaGFzZTIpCtcKQWJ1c2VGaWx0ZXIK1wpEYXRhLXJlbGVhc2UK1wpIYXNodGFncwrXCkxhYnNEQi1BdWRpdG9yCtcKTGFkaWVzLVRoYXQtRk9TUy1NZWRpYVdpa2kK1wpMYW5ndWFnZS0yMDE4LUFwci1KdW5lCtcKTGFuZ3VhZ2UtMjAxOC1KYW4tTWFyCtcKSEhWTQrXCkhBV2VsY29tZQrXCkJvbGQKSXRhbGljcwpNb25vc3BhY2VkCkxpbmsKQnVsbGV0ZWQgTGlzdApOdW1iZXJlZCBMaXN0CkNvZGUgQmxvY2sKUXVvdGUKVGFibGUKVXBsb2FkIEZpbGUKTWVtZQpQcmV2aWV3CkhlbHAKRnVsbHNjcmVlbiBNb2RlClBpbiBGb3JtIE9uIFNjcmVlbgoyMzg0ODJuMzc1IGFkZGVkIHByb2plY3RzOiBTZWN1cml0eSwgV2lraW1lZGlhLVZFLUNhbXBhaWducyAoUzItMjAxOCksIFNjYXAgKFNjYXAzLUFkb3B0aW9uLVBoYXNlMiksIEFidXNlRmlsdGVyLCBEYXRhLXJlbGVhc2UsIEhhc2h0YWdzLCBMYWJzREItQXVkaXRvciwgTGFkaWVzLVRoYXQtRk9TUy1NZWRpYVdpa2ksIExhbmd1YWdlLTIwMTgtQXByLUp1bmUsIExhbmd1YWdlLTIwMTgtSmFuLU1hciwgSEhWTSwgSEFXZWxjb21lLlBSRVZJRVcKMjM4NDgybjM3NSBtb3ZlZCB0aGlzIHRhc2sgZnJvbSBJbiBQcm9ncmVzcyB0byBJbiBDb2RlIFJldmlldyBvbiB0aGUgQW5hbHl0aWNzLUthbmJhbiBib2FyZC4KMjM4NDgybjM3NSByZW1vdmVkIEpBbGxlbWFuZG91IGFzIHRoZSBhc3NpZ25lZSBvZiB0aGlzIHRhc2suCjIzODQ4Mm4zNzUgdHJpYWdlZCB0aGlzIHRhc2sgYXMgTG93ZXN0IHByaW9yaXR5LgoyMzg0ODJuMzc1IHJlbW92ZWQgc3Vic2NyaWJlcnM6IEFrbGFwcGVyLCBKQWxsZW1hbmRvdS4KQ29udGVudCBsaWNlbnNlZCB1bmRlciBDcmVhdGl2ZSBDb21tb25zIEF0dHJpYnV0aW9uLVNoYXJlQWxpa2UgMy4wIChDQy1CWS1TQSkgdW5sZXNzIG90aGVyd2lzZSBub3RlZDsgY29kZSBsaWNlbnNlZCB1bmRlciBHTlUgR2VuZXJhbCBQdWJsaWMgTGljZW5zZSAoR1BMKSBvciBvdGhlciBvcGVuIHNvdXJjZSBsaWNlbnNlcy4gQnkgdXNpbmcgdGhpcyBzaXRlLCB5b3UgYWdyZWUgdG8gdGhlIFRlcm1zIG9mIFVzZSwgUHJpdmFjeSBQb2xpY3ksIGFuZCBDb2RlIG9mIENvbmR1Y3QuILcgV2lraW1lZGlhIEZvdW5kYXRpb24gtyBQcml2YWN5IFBvbGljeSC3IENvZGUgb2YgQ29uZHVjdCC3IFRlcm1zIG9mIFVzZSC3IERpc2NsYWltZXIgtyBDQy1CWS1TQSC3IEdQTApZb3VyIGJyb3dzZXIgdGltZXpvbmUgc2V0dGluZyBkaWZmZXJzIGZyb20gdGhlIHRpbWV6b25lIHNldHRpbmcgaW4geW91ciBwcm9maWxlLCBjbGljayB0byByZWNvbmNpbGUu

Aklapper raised the priority of this task from Lowest to Needs Triage.Jun 15 2018, 12:44 PM
Aklapper removed projects: HAWelcome, HHVM, Language-2018-Jan-Mar, Language-2018-Apr-June, Ladies-That-FOSS-MediaWiki, LabsDB-Auditor, Hashtags, Data-release, AbuseFilter, Scap (Scap3-Adoption-Phase2), Wikimedia-VE-Campaigns (S2-2018), acl*security, Analytics-Kanban.
Aklapper edited subscribers, added: Aklapper; removed: • 238482n375.
Aklapper assigned this task to kaldari.Jun 15 2018, 12:48 PM
sbassett moved this task from Waiting to Done on the deprecated-security-team-reviews board.Jul 9 2019, 8:20 PM
Maintenance_bot removed a project: Patch-For-Review.Jul 9 2019, 9:10 PM
• chasemp removed a project: deprecated-security-team-reviews.Jan 8 2020, 4:12 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits