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

Jump to bottom

Finish todos of fetching reporting URLs. #732

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Finish todos of fetching reporting URLs. #732

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e39d59c
Finish todos of fetching reporting URLs.
Jul 27, 2023
095931e
Add an issue for "no-cors"
Jul 27, 2023
27ac4aa
Merge branch 'WICG:main' into fetchreport
qingxinwu Aug 15, 2023
17a8eb0
Add a link to github issue for no-cors.
Aug 15, 2023
3f0d37a
Merge branch 'WICG:main' into fetchreport
qingxinwu Aug 18, 2023
f0e5a92
Use manual link instead of issue(github).
Aug 18, 2023
bab9348
Remove unused metadata.
Aug 18, 2023
924befd
Merge branch 'main' into fetchreport
qingxinwu Aug 29, 2023
563a157
Merge branch 'WICG:main' into fetchreport
qingxinwu Aug 31, 2023
e7a94c6
Add no-referrer back for IG update request.
Aug 31, 2023
04e2bb9
Remove extra trailing spaces.
Aug 31, 2023
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
48 changes: 41 additions & 7 deletions spec.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ To <dfn>asynchronously finish reporting</dfn> given a
1. If |buyerMap| is null, set |buyerMap| to an empty [=map=] «[]».
1. [=Finalize a reporting destination=] with |reportingMap|,
{{FenceReportingDestination/buyer}}, and |buyerMap|.
1. TODO: Fetch |leadingBidInfo|'s [=leading bid info/buyer reporting result=]'s
1. [=Send report=] to |leadingBidInfo|'s [=leading bid info/buyer reporting result=]'s
[=reporting result/report url=].
1. Set |buyerDone| to true.
1. If |sellerDone| is false and |leadingBidInfo|'s [=leading bid info/seller reporting result=]
Expand All @@ -731,7 +731,7 @@ To <dfn>asynchronously finish reporting</dfn> given a
1. If |sellerMap| is null, set |sellerMap| to an empty [=map=] «[]».
1. [=Finalize a reporting destination=] with |reportingMap|,
{{FenceReportingDestination/seller}}, and |sellerMap|.
1. TODO: Fetch |leadingBidInfo|'s [=leading bid info/seller reporting result=]'s
1. [=Send report=] to |leadingBidInfo|'s [=leading bid info/seller reporting result=]'s
[=reporting result/report url=].
1. Set |sellerDone| to true.
1. If |componentSellerDone| is false and |leadingBidInfo|'s
Expand All @@ -742,7 +742,7 @@ To <dfn>asynchronously finish reporting</dfn> given a
1. If |componentSellerMap| is null, set |componentSellerMap| to an empty [=map=] «[]».
1. [=Finalize a reporting destination=] with |reportingMap|,
{{FenceReportingDestination/component-seller}}, and |componentSellerMap|.
1. TODO: Fetch |leadingBidInfo|'s [=leading bid info/component seller reporting result=]'s
1. [=Send report=] to |leadingBidInfo|'s [=leading bid info/component seller reporting result=]'s
[=reporting result/report url=].
1. Set |componentSellerDone| to true.

Expand Down Expand Up @@ -1543,7 +1543,10 @@ To <dfn>fetch script</dfn> given a [=URL=] |url|:
:: "`error`"

Issue: One of the side-effects of a `null` client for this subresource request is it neuters all
service worker interceptions, despite not having to set the service workers mode.
service worker interceptions, despite not having to set the service workers mode.

Issue: Stop using "`no-cors`" mode where possible
(<a href="https://github.com/WICG/turtledove/issues/667">WICG/turtledove#667</a>).
1. Let |script| be null.
1. [=Fetch=] |request| with [=fetch/useParallelQueue=] set to true, and
[=fetch/processResponseConsumeBody=] set to the following steps given a [=response=] |response|
Expand Down Expand Up @@ -1575,7 +1578,10 @@ To <dfn>fetch WebAssembly</dfn> given a [=URL=] |url|:
:: "`error`"

Issue: One of the side-effects of a `null` client for this subresource request is it neuters all
service worker interceptions, despite not having to set the service workers mode.
service worker interceptions, despite not having to set the service workers mode.

Issue: Stop using "`no-cors`" mode where possible
(<a href="https://github.com/WICG/turtledove/issues/667">WICG/turtledove#667</a>).
1. Let |moduleObject| be null.
1. [=Fetch=] |request| with [=fetch/processResponseConsumeBody=] set to the following steps given
a [=response=] |response| and null, failure, or a [=byte sequence=] |responseBody|:
Expand Down Expand Up @@ -1615,7 +1621,10 @@ To <dfn>fetch trusted signals</dfn> given a [=URL=] |url|, and a [=boolean=] |is
:: "`error`"

Issue: One of the side-effects of a `null` client for this subresource request is it neuters all
service worker interceptions, despite not having to set the service workers mode.
service worker interceptions, despite not having to set the service workers mode.

Issue: Stop using "`no-cors`" mode where possible
(<a href="https://github.com/WICG/turtledove/issues/667">WICG/turtledove#667</a>).
1. Let |signals| be null.
1. Let |dataVersion| be null.
1. Let |formatVersion| be null.
Expand Down Expand Up @@ -1714,6 +1723,28 @@ Note: When trusted scoring signals fetches are not batched, |renderURLs|'s [=lis

</div>

<div algorithm>
To <dfn>send report</dfn> given a [=URL=] |url|:

1. Let |request| be a new [=request=] with the following properties:
: [=request/URL=]
:: |url|
: [=request/client=]
:: `null`
: [=request/mode=]
:: "`no-cors`"
qingxinwu marked this conversation as resolved.
Show resolved Hide resolved
: [=request/referrer=]
:: "`no-referrer`"
Copy link
Collaborator Author

@qingxinwu qingxinwu Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The implementation uses the default 'NEVER_CLEAR'.
What does it map to in fetch's spec?

Copy link
Collaborator

Choose a reason for hiding this comment

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

These aren't the same concepts. In spec you're pointing to https://fetch.spec.whatwg.org/#concept-request-referrer but in the implementation you're pointing to https://fetch.spec.whatwg.org/#concept-request-referrer-policy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I'd need to see the actual place in the implementation where this request is being set up.

Copy link
Collaborator Author

@qingxinwu qingxinwu Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh right, sorry about that. It seems "unsafe-url" is the mapped value to the implementation's "NEVER_CLEAR"
I'll ask Matt to review the whole request settings in the spec, since they're back and have the best knowledge about our request implementation here. I'll tag them under the issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, so if you're using that referrer policy, what is the referrer you're using in the implementation? Are you explicitly providing one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding redirects, the spec already uses the "error" redirect mode, right? So maybe that's fine.

Can you explain the rationale for saying the "referrer policy doesn't currently matter"? I don't understand that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Referrer policy is not useful for the initial request, but for redirects only, if I'm not understanding it wrong? And our requests do not follow redirects.

Also I checked our implementation and there's no referrer being set (confirmed with Matt). Matt said "Seems like we should probably grab the referrer and add it, for consistency, at least for the URLs from the page (Seller URLs)". But before that happens, do I spec referrer as no-referrer (then referrer policy really does not matter) for now? I can add a note about some requests' referrer would be set.
As Matt commented in the other github issue, there's also a plan of thinking about changing the request's initiator, which would affect some other fields as well, but that's still not happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

after reading more about referrer policy, seems it does affect the initial request as well (Matt and I thought it does not), so the original statement (referrer policy does not matter when we assumed referrer was set) seems wrong. But turns out our current implementation does not set a referrer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like the implementation does not manually set a referrer, but does one still get computed and sent anyways based on the URL? My understanding is that the behavior can vary depending on the implementation path taken, and since there are somewhat unique requests, I'd like to know way one way or another for sure if we send any referrer or not (regardless of if we manually set one). A WPT here would be great if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I used chrome://net-export/ to get network logs for the publisher page of our demo, and non of the requests (bidder and seller script, report url) have referrer. Didn't find signal requests in the log, but it uses the same code as script requests. Matt confirmed that network stack cannot add referrer for a request if it's not manually set, since it doesn't know what a page is.

Request header for seller script (bidder script and report request have very similar headers)
:method: GET
:authority: protected-audience-demo-ssp.web.app
:scheme: https
:path: /decision-logic.js
accept: application/javascript
sec-fetch-site: cross-site
sec-fetch-mode: no-cors
sec-fetch-dest: empty
user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)

Created a task in backlog to add WPTs for referrer.

: [=request/credentials mode=]
:: "`omit`"
: [=request/redirect mode=]
:: "`error`"

Issue: Stop using "`no-cors`" mode where possible
(<a href="https://github.com/WICG/turtledove/issues/667">WICG/turtledove#667</a>).
1. [=Fetch=] |request| with [=fetch/useParallelQueue=] set to true.
</div>

<div algorithm>

To <dfn>serialize an integer</dfn>, represent it as a string of the shortest possible decimal
Expand Down Expand Up @@ -2642,7 +2673,10 @@ The <dfn for=Navigator method>updateAdInterestGroups()</dfn> method steps are:
:: "`error`"

Issue: One of the side-effects of a `null` client for this subresource request is it neuters
all service worker interceptions, despite not having to set the service workers mode.
all service worker interceptions, despite not having to set the service workers mode.

Issue: Stop using "`no-cors`" mode where possible
(<a href="https://github.com/WICG/turtledove/issues/667">WICG/turtledove#667</a>).
1. Let |update| be null.
1. [=Fetch=] |request| with [=fetch/useParallelQueue=] set to true, and
[=fetch/processResponseConsumeBody=] set to the following steps given a [=response=] |response|
Expand Down