Skip to content
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.

Sign up for GitHub

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 security considerations section #338

Merged
merged 4 commits into from
May 27, 2022
Merged

Add security considerations section #338

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fa3537
Add security considerations section
beaufortfrancois May 23, 2022
5bcb683
Removed some opened design issues
beaufortfrancois May 24, 2022
6444c92
Add more consideration
beaufortfrancois May 24, 2022
81bea55
Address will's feedback
beaufortfrancois May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions brightness-mode-explainer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,8 @@ The following issues remain open for discussion:
- How bright is too bright? Consider 100% brightness with HDR displays, for example.
- Take a discrete or continuous value?
- Related to whether script should be allowed to reduce brightness.
- Permission model
- Would it require a user gesture (request but not consume it)?
- While brightness changes
- What if users change the brightness level while the lock is held?
- When dropping a screen brightness request
- Does the UA have to restore the previous brightness level?
- What about external displays? Do UAs need to keep track of levels for each display?
- Should script be allowed to "hold the lock" indefinitely?
- What about external displays? Do UAs need to keep track of levels for each display?

## Goals

Expand Down Expand Up @@ -121,6 +115,18 @@ Some form of "scannable element" property. When an element with said property is
<body style="brightness: max">*
```

## Security considerations

- The API is available to secure browsing contexts.

- The screen brightness can be controlled only in response to a user gesture to ensure user retain control over their screen brightness. This prevents a situation where a site increases screen brightness every time the system or user overrides it manually.

- If the page visibility becomes hidden after screen brightness has been increased, the screen brightness should be restored automatically.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add that if the brightness change request is denied, it could/should be denied with a generic reason where it isn't possible to necessarily distinguish why the request failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @willmorgan I've actually went further. See my latest patch. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to know whether the request succeeded or not, but not necessarily why. So we would blanket deny with a NotAllowedError and not go any more granular than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the requirements to know whether the request succeeded or not?

Thinking out loud: If the screen brightness is already at its maximum and you call await screen.requestBrightnessIncrease() would you expect this to fail even it didn't succeed?

I think having this baked in would help adoption of this feature from a security/privacy perspective.

Copy link
Contributor

@willmorgan willmorgan May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think if there's no gesture, battery level is too low, or feature policy is misconfigured (assuming we're doing feature policy!), I'd expect to see something like a NotAllowedError.

If the brightness is already at 100% and we call await screen.requestBrightnessIncrease(), then I'd expect the promise to fulfil. Otherwise, it might be possible to infer the brightness of the screen. If we allowed that to happen it would contradict the intention to prevent reading the screen brightness - reading the screen brightness is a non-goal of this API for privacy reasons anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willmorgan Thank you! I've updated the explainer with your feedback. PTAL


- To avoid possible user fingerprinting issues, when the request to control screen brightness is denied, a site should not be able to detect the exact reason why it happened. In other words, a generic `NotAllowedError` DOMException should be raised when there is no user gesture or battery level is too low for instance.

- If the screen brightness is at its maximum and a site requests a brighter screen, the request should succeed anyway so that it can't infer the screen brightness.

## Past discussions
- https://github.com/WICG/proposals/issues/17
- https://github.com/w3c/screen-wake-lock/issues/129
Expand Down