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.

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

Restrict usage to within a secure context. #132

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

cfredric
Copy link
Contributor

@cfredric cfredric commented Oct 24, 2022

This PR restricts usage of the API to within a secure context, and closes #125. As the Storage Access API is intended to address authenticated embeds while protecting user privacy, this change is not expected to be controversial.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@annevk
Copy link
Collaborator

annevk commented Oct 25, 2022

Okay, so the idea is that we continue to expose the API everywhere, but we require a secure context as one of the conditions for a successful invocation.

It would be good to have test coverage for this. I also wonder if there are usage numbers.

@bvandersloot-mozilla
Copy link
Collaborator

I think it is worth adding this constraint, even if there is some usage in non-secure contexts. We should sort out #134 first, though, and that is likely to cause merge conflicts

I also think this check should be further up the initial checks here, before the top-level test.

Copy link
Member

@johannhof johannhof left a comment

Choose a reason for hiding this comment

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

I agree with Ben that we should move this check up, to reject in all cases, even on (insecure) top-level documents, out of consistency (though this is a weakly held opinion, happy to change my mind if there's a good reason to do it differently).

@johannhof
Copy link
Member

(added Firefox as interested in the PR description @bvandersloot-mozilla, lmk if that's inaccurate)

@bvandersloot-mozilla
Copy link
Collaborator

(added Firefox as interested in the PR description @bvandersloot-mozilla, lmk if that's inaccurate)

That is accurate. I would have added it myself if I had permission...

@cfredric
Copy link
Contributor Author

Thanks for the comments - done, moved the secure context check higher up.

Re: tests, happy to add those; just wanted to verify that there was consensus on this first.

Copy link
Member

@johannhof johannhof left a comment

Choose a reason for hiding this comment

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

Thank you Chris, this seems fine to me. Let's add tests and file bugs for implementors.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 31, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 31, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
@cfredric
Copy link
Contributor Author

I've opened bugs for the major implementors (listed in the description above), and have an in-progress CL adding some WPT for this (https://crrev.com/c/3991336).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 31, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 1, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 1, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 7, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
@johannhof
Copy link
Member

Thanks Chris, I think per our checklist this is fine to merge, then.

@johannhof johannhof merged commit 79c2ecb into privacycg:main Nov 7, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
@cfredric cfredric deleted the secure_context branch November 8, 2022 00:17
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 8, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3991336
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068742}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3991336
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068742}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2022
This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3991336
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068742}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 12, 2022
…ecure contexts., a=testonly

Automatic update from web-platform-tests
Disallow Storage Access API calls in insecure contexts.

This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3991336
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068742}

--

wpt-commits: f9d5e9475ede2e2d0b0410c55c4b8618d7a4d9ef
wpt-pr: 36743
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 14, 2022
…ecure contexts., a=testonly

Automatic update from web-platform-tests
Disallow Storage Access API calls in insecure contexts.

This corresponds to the spec change introduced by
privacycg/storage-access#132.

This also adds cfredric as a suggested_reviewer for this directory.

Bug: 1380155, 989663
Change-Id: I63d0f15ea53f1aea6a2746cd94f4a0c6434b9d86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3991336
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068742}

--

wpt-commits: f9d5e9475ede2e2d0b0410c55c4b8618d7a4d9ef
wpt-pr: 36743
@drzraf
Copy link

drzraf commented Jul 1, 2023

As a user, I strongly disagree to restrict this (and many other) JS APIs to particular context (call it "secure").

storage or crypto (to name a few) are fundamental/core programming blocks and this idea of stripping down (= fragmenting) the JS engine is a fondamental/conceptual change regarding the whole language, its abilities and its evolution.

If I have HTTP website (which is perfectly fine in a lot of scenarios), I still expect to rely on a consistent and predictable Javascript engine ; not a freeware/demo/toy-version because, well, someone thought you mustn't use HTTP and found a way to force HTTPS upon you.

This specification (and browsers implementing it) are exceeding a role that falls on developers themselves: They are the ultimate authority about what they do with a language. And users can already choose not to browse HTTP (whose access has already been made de-incentivized).

No one restricted for-loops to x86-64 because, you know, that's such powerful construct that doesn't fit older CPUs, but that's exactly what's slowly happening to JS.
Would we restrict CSS :visited on *.ru domains, because, who knows, is the *.ru registry actually "secure" or "private" by our standards?

If LetsEncrypt wouldn't be the free worldwide provider of "secure context", I'd question the sincerity of this move, the underlying motivation and possible economic incentive.
The only (far-fetching) interpretation I could still draw is about questioning gTLD because one immediate (side or intended) effect of this spec' is to make impossible for developers to use http://mysite.whatever when developing locally
Since *.local is the de-facto only exception offered by implementers it forces developers to follow and no /etc/hosts will save them.
=> Any relation to *.zip, .dev or other .gTLD usage and future browser policies/plans regarding them?

I request storage (and crypto) API to stay out of this ES-for-kid/DRMized-JS specification project.

(Anyone from @tc39 could come here and see what's being done to our programming language?)

@johannhof
Copy link
Member

Hi @drzraf, this API defines requirements for browsers to safely allow embedded sites access to cross-site information. In the vast majority of cases this cross-site information contains user identifiers or session data that could be abused by an attacker on an insecure network. Therefore it is restricted to a secure context. What constitutes a secure context and how to improve developer ergonomics should be (politely) discussed at https://github.com/w3c/webappsec-secure-contexts or in individual bugs filed with browser engines.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict to secure context
5 participants