Page MenuHomePhabricator

Add a button to maniphest tasks to escalate security issues which need to be restricted.
ClosedPublic

Authored by mmodell on Jan 28 2016, 10:56 PM.

Diff Detail

Repository
rPHES phabricator-Security
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mmodell updated this revision to Diff 328.Jan 28 2016, 10:56 PM
mmodell retitled this revision from to Add a "lock task" button to maniphest tasks.
mmodell updated this object.
mmodell edited the test plan for this revision. (Show Details)
mmodell added a subscriber: epriestley.
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptJan 28 2016, 10:56 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript

@csteipp: Who should be allowed to lock a task using the new button that epriestley has so kindly outlined for us? Should it be available to all users or only the task author + some limited group of people?

Couple of inlines, generally looks sane.

src/WMFLockTaskController.php
41–46 ↗(On Diff #328)

This isn't currently consistent with the actual rule the code enforces -- maybe $can_lock above should be something like:

$can_lock = ($viewer->getPHID() == $task->getAuthorPHID()) ||
  $is_viewer_a_member_of_the_security_team;

Or you can just remove this dialog entirely if you're intentionally allowing any logged-in user to lock tasks.

74 ↗(On Diff #328)

I think $project_phids won't be defined here yet? (Maybe the block below just needs to move up?)

80 ↗(On Diff #328)

This might need an extra $project_phids = array_fuse($project_phids) -- the edge edit API is a little weird. You'll get an error about the format of the new value for the edge if this is an issue.

src/WMFLockTaskEventListener.php
29 ↗(On Diff #328)

(If $can_lock changes in the controller, it should change here too.)

Oh, yeah, you're already on top of the $can_lock stuff.

I'm also only requiring CAN_VIEW to lock a task (provided you pass other checks), but one possible easy implementation might be "anyone who can edit a task can lock it". I can run you through implementing that if that ultimately seems like a reasonable rule, I just wasn't sure who you wanted to be able to lock stuff.

mmodell updated this revision to Diff 329.Jan 29 2016, 6:33 AM
mmodell edited edge metadata.

Added a lot of polish to the UI and thoroughly tested on my development instance of phabricator.

@epriestley: I'm not sure who should be able to lock it either. It might be a specific group of people or maybe anyone who can edit. It's not my call - hopefully @csteipp has a better idea about that than me.

I reworked the dialog box with lots of scary markup:

.

Tested and working on my development instance, this is good stuff!

Thanks @epriestley. I like this a lot better than editing the security custom-field. If we want to implement the other types of hidden tasks ('access request', 'sensitive issue') in this same UI then it might not be terrible to add a radio button group to the dialog box...

Lock => Can not view or Lock => Can not edit?

@csteipp: Please review the wording and the UI in general. Does this seem reasonable?

mmodell added a subscriber: chasemp.

@chasemp: Check it out if you have time. This is mostly just a heads-up so you are in the loop.

Lock => Can not view or Lock => Can not edit?

Can not view. Security bugs are treated as highly sensitive. I changed the wording so that it doesn't mention "lock" in the UI, since that might be confusing. The markup in the dialog box goes to great lengths explaining exactly what is about to happen, so hopefully nobody will actually be confused.

I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?

Some general questions:

src/policy/WMFLockTaskController.php
74

Why change both? If a user can't see a task, he can not edit the task too, I think.

125

See above, I guess a "hide" is more clearer.

142

Will there be an option to show tasks only to NDA too?

I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?

Agreed.

csteipp edited edge metadata.Jan 29 2016, 8:54 PM

@csteipp: Who should be allowed to lock a task using the new button that epriestley has so kindly outlined for us? Should it be available to all users or only the task author + some limited group of people?

I think everyone should be able to do this.

I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?

I think "This is a security issue" or "Flag for Security" might be more appropriate. "Lock task" feels a lot like page protection on wiki, where the page is still public but unprivileged users can't edit. I'm worried wiki users might expect that instead of making it a security bug.

Luke081515 edited edge metadata.EditedJan 29 2016, 9:27 PM
In D113#2707, @csteipp wrote:

@csteipp: Who should be allowed to lock a task using the new button that epriestley has so kindly outlined for us? Should it be available to all users or only the task author + some limited group of people?

I think everyone should be able to do this.

I think that is better, so security issues get hidden fast as possible. Currently we had this system, and there are no problems (that I know).

I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?

I think "This is a security issue" or "Flag for Security" might be more appropriate. "Lock task" feels a lot like page protection on wiki, where the page is still public but unprivileged users can't edit. I'm worried wiki users might expect that instead of making it a security bug.

I think "hide task" is short, and fits. But what do we use, if we want to make a NDA button too?

I think "hide task" is short, and fits. But what do we use, if we want to make a NDA button too?

I really don't think we should add an NDA button. See my reply to your comments on the diff.

src/policy/WMFLockTaskController.php
74

You are probably right, maybe this is unnecessary.

142

That will be handled elsewhere. This feature is only for the Security bug reports. Submitting other types of hidden tasks will be done through custom forms. Anyone who is a policy admin can hide tasks that need to be restricted so we don't need a dedicated feature for that.

In D113#2707, @csteipp wrote:

@csteipp: Who should be allowed to lock a task using the new button that epriestley has so kindly outlined for us? Should it be available to all users or only the task author + some limited group of people?

I think everyone should be able to do this.

Agreed. Cool, that makes implementation easy too!

I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?

I think "This is a security issue" or "Flag for Security" might be more appropriate. "Lock task" feels a lot like page protection on wiki, where the page is still public but unprivileged users can't edit. I'm worried wiki users might expect that instead of making it a security bug.

"This is a security issue" could work. It doesn't imply the action but the dialog that pops up explains the process pretty well I think.

mmodell retitled this revision from Add a "lock task" button to maniphest tasks to Add a button to maniphest tasks to escalate security issues which need to be restricted..EditedJan 30 2016, 12:25 AM
mmodell edited edge metadata.

One more thing that I'm not 100% sure about. What icon should we use for the link?

I chose in the diff but we could also use or any other icon from Font-Awesome.

@csteipp: do you have a preference?

Some other possibilities:

is more symbolic, I think the user can imagine, what this button means, so maybe a bit clearer. As an alternative, I' prefering , but I think the first one is more clear.

Ok so, here's the latest version of the text / markup. I think it might be too long winded, should I just leave off the 2nd and 3rd paragraph, and leave it to the wiki page to describe in detail?


This feature is intended for security bugs that were incorrectly submitted as
regular software bug reports. You should only escalate tasks that describe
real or potential security vulnerabilities with Wikimedia software or services.

Security bugs should be escalated because of the potential that the information
could be used to develop an exploit or otherwise harm Wikimedia services or the
many people and organizations who utilize our software.

Generally the task will be made public again once the security team has
had time to properly addressed the issue and announced the vulnerability
publically.

See Reporting security bugs for more
information about reporting a vulnerability.

Escalating will restrict visibility of $monogram so that only the following
people can view it:

  • Members of the Security team.
  • Subscribers to the task.
  • Author of the task.
NOTE: Unless you are one of the people listed above, you will not be be able to view the task after you click Escalate.
In D113#2719, @mmodell wrote:

Ok so, here's the latest version of the text / markup. I think it might be too long winded, should I just leave off the 2nd and 3rd paragraph, and leave it to the wiki page to describe in detail?

I agree, 2nd and 3rd paragraph should be left off. Knowing what the workflow of the task after its submitted as being a security issue isn't as important as just knowing that it will be a security issue and the user may not be able to see it anymore. (keep the link to the wiki article)

is more symbolic, I think the user can imagine, what this button means, so maybe a bit clearer. As an alternative, I' prefering , but I think the first one is more clear.

Mmm, I like .

mmodell updated this revision to Diff 340.Feb 1 2016, 2:53 AM

Extensive UI improvements and much technical writing went into this revision. Should be ready to go now.

Initial dialog:

After clicking to expand the detailed instructions:

Can you publish this code at phab-01, so that we can test that?

(Some minor technical stuff inline.)

src/policy/WMFLockTaskController.php
47

You're doing a bit of magic here so this may not really apply, but broadly you can (and should) avoid the use of phutil_safe_html() by using arrays to concatenate blocks of markup instead of the . operator. That is, in most contexts, these two are the same:

array($a, $b);
phutil_safe_html($a . $b);

...except that the former is completely safe if $a or $b are raw, user-controlled content, while the latter introduces an XSS hole if either $a or $b is raw user-controlled content.

I suspect you can do this one with array($hide_action, $show_action). Couple more examples below.

Since users don't control any of this stuff there's no actual security issue here, but generally the array() pattern is a safer pattern.

60

phutil_safe_html() is likely unnecessary (and, conceptually, slightly dangerous) here.

62

Can probably just return array($link_div, $content_div).

108

Stray phlog().

mmodell added inline comments.Feb 1 2016, 6:23 PM
src/policy/WMFLockTaskController.php
60

I couldn't get this to work without the phutil_safe_html - my markup always ends up getting escaped.

62

same as above...

108

thanks, I missed that.

mmodell added inline comments.Feb 1 2016, 6:25 PM
src/policy/WMFLockTaskController.php
47

Anything that I add to a form view gets rendered through phutil_escape_html, so it seems like the only solution is to use safe_html or subclass the view and kill the escaping.

This revision was automatically updated to reflect the committed changes.
mmodell reopened this revision.Feb 1 2016, 9:22 PM

I didn't mean to close this by pushing to a branch. I fixed the diffusion repo config so that it only auto-closes on master and production branches.

Can you publish this code at phab-01, so that we can test that?

phab-01 is broken and I can't even log in to fix it, so:

https://phab-scap.wmflabs.org/T1

Luke081515 accepted this revision.Feb 2 2016, 12:41 AM
Luke081515 edited edge metadata.


Looks good, so approval from my site, but someone other should review too.

This revision is now accepted and ready to land.Feb 2 2016, 12:41 AM
Negative24 added inline comments.Feb 2 2016, 1:16 AM
src/policy/WMFLockTaskEventListener.php
40

Should this possible be changed to "Protect as a security issue"?

mmodell added inline comments.Feb 3 2016, 12:20 AM
src/policy/WMFLockTaskEventListener.php
40

Yes, good catch.

mmodell requested a review of this revision.Feb 3 2016, 6:58 PM
mmodell edited edge metadata.

I agree, 2nd and 3rd paragraph should be left off. Knowing what the workflow of the task after its submitted as being a security issue isn't as important as just knowing that it will be a security issue and the user may not be able to see it anymore. (keep the link to the wiki article)

Agree, I think keeping it simple is best.

I chose in the diff but we could also use or any other icon from Font-Awesome.
@csteipp: do you have a preference?
Some other possibilities:

I'd prefer or instead of .

@csteipp: The current revision is using , though I could still change it.

In D113#2759, @mmodell wrote:

@csteipp: The current revision is using , though I could still change it.

WFM

mmodell mentioned this in Unknown Object (Event).Feb 4 2016, 1:27 AM
csteipp accepted this revision.Feb 4 2016, 4:39 PM
csteipp edited edge metadata.

I have not code reviewed the patch, but as long as the patch does what it says, then the feature is correct.

This revision is now accepted and ready to land.Feb 4 2016, 4:39 PM
Luke081515 accepted this revision.EditedFeb 4 2016, 4:51 PM
Luke081515 edited edge metadata.

D113#2743
So I guess we can land, and correct line 39...

This revision was automatically updated to reflect the committed changes.