amp-sidebar: Propagate trust from action->event #25095
Conversation
| @@ -618,13 +636,23 @@ describes.realWin( | |||
| }); | |||
| owners.scheduleLayout = sandbox.stub(); | |||
|
|
|||
| impl.open_(); | |||
| execute(impl, 'open', /* trust */ 123); | |||
There was a problem hiding this comment.
Nit: Can we just use the ActionTrust.HIGH and ActionTrust.LOW enums here to make this more readable? I think they're already imported at the top of this file. =)
There was a problem hiding this comment.
I intentionally used a bogus value to make sure that the trust is being propagated and not hard-coded. :)
|
|
||
| this.registerDefaultAction(invocation => { | ||
| const {trust, caller} = invocation; | ||
| this.open_(trust, caller); |
There was a problem hiding this comment.
Could these methods (open, close, and toggle) accept an invocation instead?
This would reduce the script needed here a bit.
There was a problem hiding this comment.
Hm, most of their callers don't take ActionInvocation so we'd need to instantiate them which seems like a bad fit.
| impl.element, | ||
| 'sidebarOpen', | ||
| sinon.match.any, | ||
| /* trust */ 123 |
There was a problem hiding this comment.
Same here for enum values as @cathyxz pointed at.
| impl.element, | ||
| 'sidebarClose', | ||
| sinon.match.any, | ||
| /* trust */ 456 |
|
Just for clarification, why is this change necessary? The actions registered by default don't specify a min-trust, so they default to high trust (i.e. they will never be invoked with low trust, so we shouldn't need to worry about trust propagation). Is this for future proofing with a higher level of trust? |
That's right. I added some detail in #24894 and I'll provide more context in design review next week. |
But wouldn't that be adding a lower level of trust? Or is the plan to change everything that uses high trust to default trust? I'm not sure that is a good idea. For sidebar specifically, I'm not sure we should change this to the proposed default trust. |
That was the idea to minimize impact to existing usage. Though we can certainly revisit e.g. whether XHR-delayed gestures should be allowed to open a sidebar. |
Oh, I see, everything that is currently high falls under the new definition of default. Makes sense. |
|
@kristoferbaxter @cathyxz answered a couple questions, PTAL. :) |
|
Thanks for the reviews! |
* Plumb trust in amp-sidebar. * Minor clean up. * Also fix 0.1. * Update tests.
* Plumb trust in amp-sidebar. * Minor clean up. * Also fix 0.1. * Update tests.
Partial for #24894.
Ensure that the same trust level open/close/toggle action is called with is passed to the resulting open/close event to prevent trust escalation. Similar to #24425.
/to @leafsy /cc @sparhami