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

Relax the login status requirement from same-origin to same-site #538

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Jan 17, 2024

Some IDPs have their login on one subdomain but the FedCM endpoint on a different subdomain, and this change lets them set the login status on the correct origin.

Bug: #537


Preview | Diff

@cbiesinger cbiesinger requested a review from npm1 January 17, 2024 23:03
@npm1
Copy link
Collaborator

npm1 commented Jan 18, 2024

Maybe edit your description so that is actually links the issue?

Some IDPs have their login on one subdomain but the FedCM endpoint on
a different subdomain, and this change lets them set the login status
on the correct origin.

Bug: w3c-fedid#537
@cbiesinger
Copy link
Collaborator Author

Maybe edit your description so that is actually links the issue?

Could've sworn I already did that... anyway, done now.

@cbiesinger
Copy link
Collaborator Author

@bvandersloot-mozilla , does this look reasonable to you?

@npm1 npm1 added the agenda+ Regular CG meeting agenda items label Jul 25, 2024
<div algorithm="process the login status header">
1. Let |origin| be the response's [=response/URL=]'s [=/origin=].
1. Let |client| be the [=/request=]'s [=request/client=].
1. If the request's [=request/destination=] is not `"document"`:
1. If |client| is null, return.
1. If |origin| is not [=same origin=] with the [=/request=]'s
1. If |origin| is not [=/same site=] with the [=/request=]'s

Choose a reason for hiding this comment

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

what is the significance of / in the dfn syntax? I don't see any reference in the bikeshed docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See https://speced.github.io/bikeshed/#autolink-inside -- it links to a dfn that has no for attribute (here, used to disambiguate what it links to)

Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

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

Overall this is reasonable due to the scoping of cookies to site anyway.

I wonder if we should consider using a permission policy rather than a strict ancestor check though.

@npm1
Copy link
Collaborator

npm1 commented Aug 1, 2024

Overall this is reasonable due to the scoping of cookies to site anyway.

Thanks for the review! Will merge this PR once the repo is migrated to the WG.

I wonder if we should consider using a permission policy rather than a strict ancestor check though.

It would not be very useful for the IdP to be allowed to change its login status if it does not have cookie access, hence why we did not consider adding permissions policy.

@npm1 npm1 removed the agenda+ Regular CG meeting agenda items label Aug 2, 2024
@npm1 npm1 merged commit 548e7b2 into w3c-fedid:main Aug 16, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Aug 16, 2024
SHA: 548e7b2
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@cbiesinger cbiesinger deleted the samesitestatus branch August 22, 2024 17:58
npm1 pushed a commit that referenced this pull request Sep 18, 2024
Some IDPs have their login on one subdomain but the FedCM endpoint on
a different subdomain, and this change lets them set the login status
on the correct origin.

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

Successfully merging this pull request may close these issues.

5 participants