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

Fetching should be defined in terms of Fetch #261

Closed
annevk opened this issue May 19, 2022 · 21 comments
Closed

Fetching should be defined in terms of Fetch #261

annevk opened this issue May 19, 2022 · 21 comments
Labels
compatibility risk Issues that may lead to backwards compatibility problems editorial

Comments

@annevk
Copy link

annevk commented May 19, 2022

Requirements such as

This request MUST NOT follow HTTP redirects and instead abort with an error if there are any.

are not needed if this specification would build on top of Fetch. That would also address a number of other things that are unclear, such as referrer handling, etc.

@cbiesinger
Copy link
Collaborator

cbiesinger commented May 19, 2022

duplicate of issue #188. (what is unclear about referer handling?)

@annevk
Copy link
Author

annevk commented May 19, 2022

How is it clear? :-)

@cbiesinger
Copy link
Collaborator

Heh. Well each endpoint specifies whether it is sent or not, e.g.:
https://fedidcg.github.io/FedCM/#idp-api-client-id-metadata-endpoint

c) with a Referer header indicating the RP's origin (as if Referer-Policy: strict-origin was in use)

I don't disagree with the fetch suggestion, but is that line not clear?

@annevk
Copy link
Author

annevk commented May 19, 2022

Ah, fair, that's actually pretty clear (though can a referrer policy override it?), though the algorithm I was looking at https://fedidcg.github.io/FedCM/#browser-api-rp-sign-in does not seem to specify anything.

@cbiesinger
Copy link
Collaborator

Ah yes. That should be better integrated with the other section, thanks for pointing that out

@cbiesinger
Copy link
Collaborator

I'm going to reopen this issue and narrow the scope to clarifying that issue

@npm1

@cbiesinger cbiesinger reopened this May 20, 2022
@cbiesinger
Copy link
Collaborator

(I can't figure out how to change the title of a github issue...)

@annevk
Copy link
Author

annevk commented May 20, 2022

(next to the green "New issue" button there's a gray "Edit" button, both to the right of the heading)

@cbiesinger
Copy link
Collaborator

I must be missing some permission to do that :(

@npm1
Copy link
Collaborator

npm1 commented May 20, 2022

It may be only @samuelgoto has the permissions to edit...

@npm1
Copy link
Collaborator

npm1 commented Jul 20, 2022

We integrated one of the fetch calls in here https://fedidcg.github.io/FedCM/#check-the-root-manifest. Does this look OK? If so then we can move ahead and integrate the others similarly, if not let's improve the integration of that one more and then model the other ones on top of this one.

@annevk
Copy link
Author

annevk commented Aug 24, 2022

Set rootUrl’s path to ".well-known/web-identity".

This needs to be a list, no? You might want to assert provider’s configURL's scheme for clarity here.

If response’s Content-Type Metadata is not a JSON MIME Type, return false.

You want to use https://fetch.spec.whatwg.org/#concept-header-extract-mime-type here.

You also need to set many more fields of the request, as discussed in some of the other issues.

@annevk
Copy link
Author

annevk commented Aug 24, 2022

Also, what you have attempted to define is that "check the root manifest" returns a boolean but instead it returns early and then the callback passed to processResponseConsumeBody returns a boolean which is then ignored.

@npm1 npm1 mentioned this issue Aug 26, 2022
@cbiesinger
Copy link
Collaborator

for reference, whatwg/fetch#1495 (if accepted) spec's a "web-identity" value for Sec-Fetch-Dest

@npm1 npm1 mentioned this issue Sep 27, 2022
@samuelgoto samuelgoto added the compatibility risk Issues that may lead to backwards compatibility problems label Oct 3, 2022
@npm1
Copy link
Collaborator

npm1 commented Oct 3, 2022

Closing but feel free to provide feedback if there's something off with how we integrated with Fetch.

@npm1 npm1 closed this as completed Oct 3, 2022
@annevk
Copy link
Author

annevk commented Oct 4, 2022

"check the root manifest" is still broken as it expects fetch to block or some such.

@npm1
Copy link
Collaborator

npm1 commented Oct 4, 2022

@annevk help me understand how to actually use the fetch result? We have an algorithm alg that wants to call fetch and return something based on processResponseConsumeBody. Currently we have

alg:
...
1. Create |var|
2. Fetch and in processResponseConsumeBody set |var| to something
3. Return |var|

Is there a notion of 'await' in the spec so this becomes correct? Would it be ok to have step 3 be Wait until |var| is no longer null or some such? Otherwise using fetch on algorithms that return something based on it seems really complicated. Any example of correct usage? I was browsing the HTML spec and most fetch usages are not in returning algorithms. One that did is broken in the same way.

@annevk
Copy link
Author

annevk commented Oct 5, 2022

I mean in general you cannot really invoke fetch from "in parallel" without a lot of work so you'll have to account for it "calling you back" at some point. Otherwise you'd have to snapshot a bunch of state from the global object and take that with you somehow (because Fetch will need it for enforcing CSP and other things). And none of that is really worked out in detail. (And yeah, some of HTML is indeed broken in this way, but hopefully for new specs we don't repeat those mistakes.)

@npm1
Copy link
Collaborator

npm1 commented Oct 5, 2022

Ok let me reopen for now. What's missing here is rewriting everything so that we need to call an algorithm from fetch instead of returning a variable updated by fetch. This requires a lot of updates to the algorithms but I'll try to get to it soon.

@domfarolino
Copy link

Is this done now that #378 has landed?

@npm1
Copy link
Collaborator

npm1 commented Jan 19, 2023

I think so, the remaining piece is the Sec-Fetch-Dest which is being tracked in #320.

@npm1 npm1 closed this as completed Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility risk Issues that may lead to backwards compatibility problems editorial
Projects
None yet
Development

No branches or pull requests

5 participants