Refs T125104. This is based on example code provided by
@epriestley.
Details
Details
- Maniphest Tasks
- T122556: Figure out what upstream "Can Edit Task Policies" policy deprecation means for our Spaces/ACL setup
T125104: Evaluate the feasibility of phasing out the Phabricator Security extension - Reviewers
• chasemp csteipp Luke081515 - Commits
- rPHEXc9b7c132a592: Add a button to maniphest tasks to escalate security issues which need to be…
rPHESc9b7c132a592: Add a button to maniphest tasks to escalate security issues which need to be…
rPHES3375b9dc76a7: Add a "lock task" button to maniphest tasks - Patch without arc
- git checkout -b D113 && curl -L https://phabricator.wikimedia.org/D113?download=true | git apply
Currently untested but it should be nearly
ready to go.
Diff Detail
Diff Detail
- Repository
- rPHES phabricator-Security
- Branch
- production
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 313 Build 428: arc lint + arc unit
Event Timeline
Comment Actions
@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?
Comment Actions
Couple of inlines, generally looks sane.
| src/WMFLockTaskController.php | ||
|---|---|---|
| 41–46 | 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 | I think $project_phids won't be defined here yet? (Maybe the block below just needs to move up?) | |
| 80 | 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 | (If $can_lock changes in the controller, it should change here too.) | |
Comment Actions
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.
Comment Actions
Added a lot of polish to the UI and thoroughly tested on my development instance of phabricator.
Comment Actions
.
@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...
Comment Actions
@csteipp: Please review the wording and the UI in general. Does this seem reasonable?
Comment Actions
@chasemp: Check it out if you have time. This is mostly just a heads-up so you are in the loop.
Comment Actions
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.
Comment Actions
I think the title "hide task" may be better than "lock task", you can understand better what the button does. What do you think?
Comment Actions
@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.
Comment Actions
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?
Comment Actions
I really don't think we should add an NDA button. See my reply to your comments on the diff.
| src/policy/WMFLockTaskController.php | ||
|---|---|---|
| 73 ↗ | (On Diff #329) | You are probably right, maybe this is unnecessary. |
| 141 ↗ | (On Diff #329) | 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. |
Comment Actions
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.
Comment Actions
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:
Comment Actions
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.
Comment Actions
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.
Comment Actions
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)
Mmm, I like .
Comment Actions
Extensive UI improvements and much technical writing went into this revision. Should be ready to go now.
Comment Actions
(Some minor technical stuff inline.)
| src/policy/WMFLockTaskController.php | ||
|---|---|---|
| 46 ↗ | (On Diff #340) | 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. |
| 59 ↗ | (On Diff #340) | phutil_safe_html() is likely unnecessary (and, conceptually, slightly dangerous) here. |
| 61 ↗ | (On Diff #340) | Can probably just return array($link_div, $content_div). |
| 107 ↗ | (On Diff #340) | Stray phlog(). |
| src/policy/WMFLockTaskController.php | ||
|---|---|---|
| 46 ↗ | (On Diff #340) | 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. |
Comment Actions
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.
| src/policy/WMFLockTaskEventListener.php | ||
|---|---|---|
| 39 ↗ | (On Diff #341) | Should this possible be changed to "Protect as a security issue"? |
| src/policy/WMFLockTaskEventListener.php | ||
|---|---|---|
| 39 ↗ | (On Diff #341) | Yes, good catch. |
Comment Actions
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 .
Comment Actions
I have not code reviewed the patch, but as long as the patch does what it says, then the feature is correct.