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

Add a destination type for FedCM #1495

Merged
merged 1 commit into from Oct 4, 2022
Merged

Add a destination type for FedCM #1495

merged 1 commit into from Oct 4, 2022

Conversation

cbiesinger
Copy link
Contributor

@cbiesinger cbiesinger commented Sep 27, 2022

We think it makes sense to define a destination type specific to FedCM requests (https://fedidcg.github.io/FedCM/)

This would replace the Sec-FedCM-CSRF header that is currently in the FedCM spec.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
    • I will add WPT tests once I update Chrome's implementation to use this destination
  • Implementation bugs are filed:
    • Chrome: https://crbug.com/1368382
    • Firefox: n/a, fedcm impl still in progress
    • Safari: n/a, does not support fedcm
    • Deno (not for CORS changes): n/a

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


Preview | Diff

@cbiesinger
Copy link
Contributor Author

xref fedidcg/FedCM#353

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

When are implementations planning to ship this? If that's soon, will it move out of the CG soon as well?

This also requires a PR to CSP I think, to make it use connect-src.

fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Sep 27, 2022

The Build script discovered you need to update <td rowspan=19> to account for this new row.

We think it makes sense to define a destination type specific to
FedCM requests (https://fedidcg.github.io/FedCM/)

This would replace the Sec-FedCM-CSRF header that is currently in the
FedCM spec.
@cbiesinger
Copy link
Contributor Author

To answer the questions:

@annevk
Copy link
Member

annevk commented Sep 30, 2022

The way it works is that Fetch calls out to CSP (to ensure all fetches are governed by CSP). With what you have now we'd end up checking CSP twice and slightly different CSP policies at that.

@cbiesinger
Copy link
Contributor Author

Oh OK, I will work on that. Can you elaborate on how it is a slightly different policy...?

@annevk
Copy link
Member

annevk commented Sep 30, 2022

Just because of the code path it goes down in CSP which doesn't currently know about this destination.

cbiesinger added a commit to cbiesinger/webappsec-csp that referenced this pull request Sep 30, 2022
@cbiesinger
Copy link
Contributor Author

I have sent w3c/webappsec-csp#567

Is there anything else that's blocking merging this PR?

@cbiesinger
Copy link
Contributor Author

Tests are in web-platform-tests/wpt#36230

@bvandersloot-mozilla
Copy link

* Not sure about Firefox's plans, maybe @bvandersloot-mozilla can advise?

We are supportive and implementing FedCM. It is harder for us to be too precise at this point. Current ballpark is early next year.

@annevk annevk merged commit 30d462d into whatwg:main Oct 4, 2022
@annevk
Copy link
Member

annevk commented Oct 4, 2022

Thanks @cbiesinger!

aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 4, 2022
See whatwg/fetch#1495

Bug: 1368382
Change-Id: I298988f78fb10e21ccc83dde9f218f1371c676d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3931069
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1054725}
mikewest pushed a commit to w3c/webappsec-csp that referenced this pull request Oct 5, 2022
@@ -1517,6 +1517,7 @@ the empty string,
"<code>style</code>",
"<code>track</code>",
"<code>video</code>",
"<code>webidentity</code>",

Choose a reason for hiding this comment

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

Shouldn't this have been added to the dictionary too, or how is https://fetch.spec.whatwg.org/#dom-request-destination supposed to work?

Copy link
Member

Choose a reason for hiding this comment

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

It depends a bit on whether it's exposed to service workers, but if it is then yes. I filed #1500 on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. It is not exposed to service workers but I'll let Anne handle the question of whether it should be added to the enum anyway, since they have a patch already.

@cbiesinger cbiesinger deleted the fedcm branch October 6, 2022 16:51
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
See whatwg/fetch#1495

Bug: 1368382
Change-Id: I298988f78fb10e21ccc83dde9f218f1371c676d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3931069
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1054725}
NOKEYCHECK=True
GitOrigin-RevId: 2f45baa7de4e77ab72424192e7b4f9f659455ae6
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2022
… (FedCM), r=freddyb,webdriver-reviewers,whimboo

This is to keep up with WHATWG Fetch whatwg/fetch#1495 .
Also revised to not include the new destination type in the RequestDestination enum, per whatwg/fetch#1500 .

I added an element to nsIContentPolicy::nsContentPolicyType as my starting point and
proceeded from there, following the instructions at the end of the internal enum.

Differential Revision: https://phabricator.services.mozilla.com/D158657
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 19, 2022
… (FedCM), r=freddyb,webdriver-reviewers,whimboo

This is to keep up with WHATWG Fetch whatwg/fetch#1495 .
Also revised to not include the new destination type in the RequestDestination enum, per whatwg/fetch#1500 .

I added an element to nsIContentPolicy::nsContentPolicyType as my starting point and
proceeded from there, following the instructions at the end of the internal enum.

Differential Revision: https://phabricator.services.mozilla.com/D158657
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants