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

Sec-Fetch-User is not detailed enough #23

Closed
annevk opened this issue Mar 26, 2019 · 6 comments
Closed

Sec-Fetch-User is not detailed enough #23

annevk opened this issue Mar 26, 2019 · 6 comments
Labels
bug Something isn't working

Comments

@annevk
Copy link
Member

annevk commented Mar 26, 2019

E.g., if I change the src of an <img> element in response to a click, does that count? What if I invoked fetch() instead? (Note that in case of the latter putting state on requests isn't the way to go.)

@mikewest
Copy link
Member

Indeed. It's quite hand-wavy at the moment. The requests I care most about are navigational requests, and the approach sketched out in the spec assumes that we'd check the user activation status in HTML's navigation algorithm, and pass it on to Fetch.

For subresource requests, the implementation behind a flag in Chrome is currently hard-coded to ?F, as we hadn't made a decision about whether it would be useful enough on the server side to expose whether or not the user agent thought that the request was triggered by user activation. If it turns out to be useful, I expect our implementation would rely upon LocalFrame::HasTransientUserActivation, which is being reimplemented on top of https://github.com/mustaqahmed/user-activation-v2.

In the spec, I expect we'd continue to rely on triggered by user activation, but we'd need to examine it explicitly in every case where we construct a request object (as I expect it would be unreasonable to blindly apply that test from inside Fetch).

/cc @arturjanc, as we've talked about this a little bit in the past.

@arturjanc
Copy link
Contributor

As a bit of a tangent: after Chrome implements Sec-Fetch-Site: none we're going to gather data about the number of cross-site navigations that don't set Sec-Fetch-User: ?T and review what causes them. Depending on how frequently this happens, we'll have an idea about the cases where it's tractable to block/redirect requests without a user-activation (to protect against some XS-Search attacks).

@mikewest mikewest added the bug Something isn't working label Mar 27, 2019
mikewest added a commit that referenced this issue Mar 29, 2019
This patch:

*   Locks `Sec-Fetch-User` to navigation requests.
*   Drops the header when it's `false`.
*   Sketches out monkeypatches to Fetch and HTML in a little more
    detail so that we can have a more focused discussion around the
    integration points we'll want in the future.

Fixes #19.
Partially addresses #23.
@mikewest
Copy link
Member

mikewest commented Mar 29, 2019

In 79938e0, I've locked the header to navigation requests (in https://mikewest.github.io/sec-metadata/#sec-fetch-user-header), and done a little work to explain the integration between Fetch and HTML in (https://mikewest.github.io/sec-metadata/#fetch-integration).

Do those look like patches you'd consider accepting against those documents, @annevk?

@annevk
Copy link
Member Author

annevk commented Mar 29, 2019

That is probably reasonable, yes. Assuming navigate hasn't gone in parallel by the time you do the "triggered by user activation" check.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
@mikewest
Copy link
Member

Ok. Tests are being updated in https://chromium-review.googlesource.com/c/chromium/src/+/1545871 (WPT: web-platform-tests/wpt#16148). I'll close this out once that lands, and punt the subresource discussion to #26.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 30, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 30, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 30, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}
@mikewest
Copy link
Member

mikewest commented Apr 1, 2019

web-platform-tests/wpt#16148 was merged over the weekend.

@mikewest mikewest closed this as completed Apr 1, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 3, 2019
…vated, navigational requests., a=testonly

Automatic update from web-platform-tests
Send `Sec-Fetch-User` only for user-activated, navigational requests.

As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}

--

wpt-commits: b93752a06f9498f774aed288663259cd738f1a7c
wpt-pr: 16148
mykmelez pushed a commit to mykmelez/gecko that referenced this issue May 6, 2019
…vated, navigational requests., a=testonly

Automatic update from web-platform-tests
Send `Sec-Fetch-User` only for user-activated, navigational requests.

As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}

--

wpt-commits: b93752a06f9498f774aed288663259cd738f1a7c
wpt-pr: 16148
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#646086}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…vated, navigational requests., a=testonly

Automatic update from web-platform-tests
Send `Sec-Fetch-User` only for user-activated, navigational requests.

As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <alexmoschromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#646086}

--

wpt-commits: b93752a06f9498f774aed288663259cd738f1a7c
wpt-pr: 16148

UltraBlame original commit: 595f9f3aa36e9e4e5c8f120d7349c2965613814d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…vated, navigational requests., a=testonly

Automatic update from web-platform-tests
Send `Sec-Fetch-User` only for user-activated, navigational requests.

As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <alexmoschromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#646086}

--

wpt-commits: b93752a06f9498f774aed288663259cd738f1a7c
wpt-pr: 16148

UltraBlame original commit: 595f9f3aa36e9e4e5c8f120d7349c2965613814d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…vated, navigational requests., a=testonly

Automatic update from web-platform-tests
Send `Sec-Fetch-User` only for user-activated, navigational requests.

As per the conversation in w3c/webappsec-fetch-metadata#23 and
w3c/webappsec-fetch-metadata#19, this patch drops the `Sec-Fetch-User` header
for non-navigational requests, and for navigational requests that are
not user-activated.

Bug: 947444
Change-Id: Ica4846bda6ccf4e8bce1323803954f4fef9c18a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545871
Reviewed-by: Alex Moshchuk <alexmoschromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#646086}

--

wpt-commits: b93752a06f9498f774aed288663259cd738f1a7c
wpt-pr: 16148

UltraBlame original commit: 595f9f3aa36e9e4e5c8f120d7349c2965613814d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants