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

Finish todos of fetching reporting URLs. #732

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Jul 27, 2023

@domfarolino PTAL. Thanks.


Preview | Diff

@qingxinwu qingxinwu added the spec Relates to the spec label Jul 27, 2023
: [=request/mode=]
:: "`no-cors`"
: [=request/referrer=]
:: "`no-referrer`"
Copy link
Collaborator Author

@qingxinwu qingxinwu Jul 27, 2023

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

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.

spec.bs Show resolved Hide resolved
@JensenPaul JensenPaul merged commit 581cad8 into WICG:main Sep 5, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Sep 5, 2023
SHA: 581cad8
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the fetchreport branch September 6, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants