-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add details on security for invokers #942
Open
keithamus
wants to merge
2
commits into
openui:main
Choose a base branch
from
keithamus:add-details-on-security-for-invokers
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+143
−0
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,6 +268,149 @@ If the _Invokee_ is a `<dialog>` element the _Invoker_ implicitly receives an | |
will be `aria-expanded=true` when the _Invokee_ is open and | ||
`aria-expanded=false` otherwise. | ||
|
||
### Security | ||
|
||
This proposal adds new capabilities in that elements can be interacted with | ||
using HTML where prior these required scripting (read: JS). Care has been taken | ||
within this proposal to ensure there are no substantially new security | ||
implications, and that the risk is kept low. | ||
|
||
In general, _preventing scripting is not a security barrier_ because very few | ||
users or websites disable scripting. That being said, we should not grant | ||
significant new capabilities with or without scripting. | ||
|
||
Invokers, by design, have two security affordances: | ||
|
||
- Without scripting, they require user interaction - the invoker button needs | ||
to be clicked for the browser to fire an Invoke Event. Scripts are able to | ||
synthesise the `InvokeEvent`, or the `click` event on the button - but | ||
scripts are already capable of causing the same interactions without Invokers. | ||
- The `invoketarget` takes an IDREF which must reference an ID within the same | ||
document. `invokeTargetElement` can be assigned a cross-root element, but | ||
this requires scripting which means all actions are already possible. | ||
|
||
To go into more detail, below is a brief summary of the security considerations | ||
per interaction: | ||
|
||
#### Popovers | ||
|
||
This proposal allows opening and closing popovers with `invoketarget`. This is | ||
already possible with `popovertarget`, and so there is no new capability here. | ||
|
||
#### Dialogs | ||
|
||
This proposal allows opening and closing `<dialog>` elements with | ||
`invoketarget`. While this is new capability (prior to this proposal scripting | ||
is required), it poses the same security considerations as popovers in that it | ||
toggles the visiblity of a new element. Modal dialog elements are also placed | ||
on the "top layer" which means they occlude other content. This is already | ||
possible with popovers, which are also placed on the top layer, so while | ||
buttons to open `<dialog>` elements is a new capability, it is not a novel new | ||
security threat considering attackers could already craft a popover. | ||
|
||
Modal dialogs also trap focus, such that focus can only be moved within | ||
elements inside the dialog or the browser native controls (such as the URL | ||
bar). This is considered to not pose a significant threat as the invoking | ||
button requires user activation, the dialog cannot render outside of the | ||
document frame (iframe or browser window) and so cannot occlude the browsers | ||
Chrome, and there are built in gestures to close a dialog (such as the Esc key | ||
via a `CloseWatcher`) that cannot be opted out of with or without invokers. | ||
|
||
#### Details (proposed follow-on) | ||
|
||
This proposal aims to allow opening and closing `<details>` elements with | ||
`invoketarget`. This is not considered to be new capability, as it is already | ||
possible to do this with the `<summary>` element. | ||
|
||
#### Input (proposed follow-on) | ||
|
||
It should be noted that input pickers (like the file picker dialog) render native | ||
controls outside of the bounds of the document frame (iframe or browser window). | ||
Due to this, the security implications of invoking these elements should be | ||
carefully considered. | ||
|
||
Scripts can call `.showPicker()` on form elements, and the picker will open. | ||
There are some cross-origin restrictions, for example calling `.showPicker()` | ||
on elements in a cross-origin context only works for `<input type=file>` or | ||
`<input type=color>`. | ||
|
||
It is also possible to wrap an `<input>` in a `<label>`, where invoking the | ||
label will open the picker. This is quite common practice when trying to style | ||
`<input type=file>` for example; dressing the `<label>` to look like a | ||
`<button>` and hiding the `<input>`. | ||
|
||
This proposal allows showing the pickers of input elements, for example an | ||
invoker can target an `<input type=file>` and clicking the invoker will open | ||
the operating systems file picker dialog. As stated above, this is already | ||
possible with `<label>` elements, but care should be taken to ensure that | ||
the same cross-origin restrictions apply for Invokers as they do for scripting. | ||
|
||
#### Fullscreen elements (proposed follow-on) | ||
|
||
Turning an element into a fullscreen needs to be carefully consdidered. | ||
Scripting already allows this via the `.requestFullscreen()` API which, as the | ||
name suggests, may not always be successful given the UA context. This API | ||
requires the document context to be active, for example requiring a user | ||
activiation (this is true of invokers in the general case), it also requires | ||
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
the Permissions Policy to allow for this behaviour, and requires the element to | ||
not already be on the Top Layer. | ||
|
||
All of these constrains should also be true for invokers opening fullscreen | ||
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
elements. This means the only _significant_ new behavioural difference between | ||
an invoker opening a fullscreen element vs the existing behaviour which | ||
requires scripting, is that sanboxed iframes which disallow scripting but allow | ||
fullscreen may now declare a button that can fullscreen. Consider the following: | ||
|
||
```html | ||
<iframe sandbox allow="fullscreen"> | ||
<html id="invokee"> | ||
<button invoketarget="invokee" invokeaction="toggleFullscreen">Invoke</button> | ||
<button onclick="invokee.requestFullscreen()">Go fullscreen</button> | ||
</html> | ||
</iframe> | ||
```` | ||
|
||
In this example, the `<button onclick>` will do nothing as the iframe is | ||
sanboxed and does not allow scripting. The `<button invoketarget>` will | ||
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fullscreen the element, however, as the `allow=fullscreen` Permission Policy is | ||
enabled to allow fullscreen. | ||
|
||
This is considered to be a low security threat, as it requires opt-in to the | ||
fullscreen Permission Policy explitly via iframe attributes or scripting. We | ||
will, however, continue to explore if this should be a possibility. | ||
|
||
#### Media Elements (proposed follow-on) | ||
|
||
This proposal allows video and audio elements to be controlled with buttons | ||
outside of the browsers native video controls. This includes playing, pausing, | ||
and toggling mute/unmute. Today this is only possible using scripting via the | ||
equivalent scripting APIs: `.play()` / `.pause()` / `.muted=`. These APIs are | ||
guarded by Permissions Policy, and so should the invoker equivalent. Some User | ||
Agents can be configered to reject calls to `.play()`, and this should also be | ||
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
true of invokers. As such the key new capability here is the ability to call | ||
these APIs without scripting enabled. This is effectively the same concern as | ||
fullscreen; the security model is guarded around the Permissions Policy. | ||
|
||
There is also additional concern around the media element invokers being able | ||
to circumvent autoplay policies. Invokers should not be able to cicurmvent | ||
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
these, and so the play or playpause actions should only be functional in | ||
environments which allow autoplaying of videos. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is the correct mitigation. They should just behave the same way as the JS APIs, for example if there's no activation and you play then it plays but muted. Would need to understand the exact specifics across browsers here. |
||
|
||
#### Further proposals... | ||
|
||
There are continued suggestions for new capabilities of Invokers, such as | ||
[invoking picture-in-picture](https://github.com/openui/open-ui/issues/916). | ||
|
||
Each of these will need to be taken into consideration on a case-per-case | ||
basis. The following questions will need to be answered for each of these: | ||
|
||
- Is it possible for the user to do this today with scripting? | ||
- Is it possible for the user to do this today without scripting? | ||
- Does this behaviour bypass scripting sandboxing rules, such as | ||
Permissions Policy or iframes? | ||
- Does this enable buttons to invoke content that appears outside of the | ||
browser frame? | ||
|
||
### Defaults | ||
|
||
Depending on the target set by `invoketarget`, invoking the button will trigger | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth explaining that invokers as currently implemented fully enforce the cross-origin blocking and don't including the file or color carveouts that showPicker has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have the explainer speak to a current implementation or a desired one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desired one. And I think the implementation is the desired one we shouldn't special case file and color for invokers imo