Page MenuHomePhabricator

Command::restrict( Shell::RESTRICT_NONE ) doesn't actually work
Closed, ResolvedPublic

Description

Command::restrict() is supposed to add bitfields to $restrictions, not overwrite them. So ->restrict( Shell:RESTRICT_NONE ), which has a value of 0, will do absolutely nothing, leaving the default shell restrictions in place.

My proposal is to make restrict() overwrite the existing restrictions. This would mean that anyone setting a restriction would need to always do something like ->restrict( Shell:RESTRICT_DEFAULT | Shell::NO_FOO ).

Given that codesearch shows every non-test caller already doing this, I believe this is a safe and useful breaking change to make.

Event Timeline

Legoktm created this task.Jul 7 2020, 12:08 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 7 2020, 12:08 AM
Legoktm claimed this task.Jul 7 2020, 12:15 AM

Change 609869 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Change 609870 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Change 609869 merged by jenkins-bot:
[mediawiki/core@master] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Tgr added a subscriber: Tgr.Jul 8 2020, 2:21 PM

Deprecate and rename to setRestrictions? (Or just add that method and leave restrict as it is.) The current name with the new behavior isn't really intuitive, IMO.

Ideally I think it would be called restrictions(), following the convention that chaining mutators take the name of the thing they are mutating. But the fact that there are already callers expecting this behaviour makes the proposed patch good enough for me.

Change 609870 merged by jenkins-bot:
[mediawiki/core@master] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Legoktm closed this task as Resolved.Jul 29 2020, 5:23 AM

Sorry, I forgot to reply earlier.

Deprecate and rename to setRestrictions? (Or just add that method and leave restrict as it is.) The current name with the new behavior isn't really intuitive, IMO.

Ideally I think it would be called restrictions(), following the convention that chaining mutators take the name of the thing they are mutating. But the fact that there are already callers expecting this behaviour makes the proposed patch good enough for me.

Right, changing the name would've just been extra work in that we'd have to update all the uses just to change the name, not change any behavior. Even though this was technically a breaking change, it only did fixing, not really breaking.

Change 616977 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@REL1_35] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Change 616977 merged by jenkins-bot:
[mediawiki/core@REL1_35] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Tgr added a comment.Jul 29 2020, 1:42 PM

Right, changing the name would've just been extra work in that we'd have to update all the uses just to change the name, not change any behavior. Even though this was technically a breaking change, it only did fixing, not really breaking.

It did break expectations created by the name, though. It doesn't sound like $command->restrict( Shell::NO_NETWORK ) would make your code less safe, but it actually does. Intuitive naming is especially important for security-sensitive code.

Change 622785 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_34] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Change 623121 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_34] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Change 623122 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_31] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Change 623123 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_31] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Change 623122 merged by jenkins-bot:
[mediawiki/core@REL1_31] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Change 623123 merged by jenkins-bot:
[mediawiki/core@REL1_31] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

Change 622785 merged by jenkins-bot:
[mediawiki/core@REL1_34] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

Change 623121 merged by jenkins-bot:
[mediawiki/core@REL1_34] shell: Make ->restrict( RESTRICT_NONE ) actually work

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