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

Change default of request's credentials mode #1153

Merged
merged 2 commits into from
Feb 2, 2021
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 30, 2021

This only affects specifications that do not set credentials mode and I'm not aware of any.

Closes #804.


Preview | Diff

This only affects specifications that do not set credentials mode and I'm not aware of any.

Closes #804.
@annevk annevk requested a review from domenic January 30, 2021 11:18
@domenic
Copy link
Member

domenic commented Feb 1, 2021

Many parts of HTML fail to set credentials mode, I think? E.g. a bunch of navigation ones:

Let resource be a new request whose url is url and whose referrer policy is the current state of element's referrerpolicy content attribute.

If destination is not a request, then set destination to a new request whose URL is destination.

Plan to navigate to a new request whose url is parsed action, method is method, header list consists of Content-Type/MIME type, and body is body.

Let request be a new request whose URL is urlRecord.

(and more I skipped over)

importScripts():

Let request be a new request whose url is url, client is settings object, destination is "script", parser metadata is "not parser-inserted", synchronous flag is set, and whose use-URL-credentials flag is set.

Downloads:

Let request be a new request whose url is URL, client is entry settings object, initiator is "download", destination is the empty string, and whose synchronous flag and use-URL-credentials flag are set.

I also found service worker script imports:

Let importRequest be a new request whose url is importUrl, client is job’s client, destination is "script", parser metadata is "not parser-inserted", synchronous flag is set, and whose use-URL-credentials flag is set.

@annevk
Copy link
Member Author

annevk commented Feb 1, 2021

None of those should use "omit".

@domenic
Copy link
Member

domenic commented Feb 1, 2021

Sure, I wasn't really questioning the change, just the commit message. Although it might be worth using this opportunity as a forcing function to fix HTML, at least.

@annevk
Copy link
Member Author

annevk commented Feb 1, 2021

What would we want in HTML? Set credentials mode less I suppose as the default already handles it?

@domenic
Copy link
Member

domenic commented Feb 1, 2021

Well navigate (and downloads?) seems like it should always be include? We could force that inside the navigate algorithm, but that seems hacky; I'd almost prefer updating all the call sites then adding an assert inside the navigate algorithm.

I'm honestly not sure what browsers do for importScripts(). If it's same-origin then omitting it seems OK I guess.

Alternately we could try to remove the default entirely and make all callers consider this? Hmm, not sure...

@annevk
Copy link
Member Author

annevk commented Feb 1, 2021

Oh right, yeah, some of those need "include", at which point that would be a pretty significant undertaking with regards to tests. I don't want to take that on right now.

@annevk annevk merged commit 49085f9 into main Feb 2, 2021
@annevk annevk deleted the annevk/credentials-mode branch February 2, 2021 11:30
bors-servo added a commit to servo/servo that referenced this pull request Apr 20, 2023
…l-mode-for-RequestInit-fix, r=<try>

Use same-origin as default credential mode for RequestInit fix

<!-- Please describe your changes on the following line: -->
- Updated the default credential mode from omit to same-origin as per: whatwg/fetch#1153
- Deleted instances of fallback credentials
---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29633  (GitHub issue number if applicable)
- [X] There are tests for these changes
bors-servo added a commit to servo/servo that referenced this pull request Apr 28, 2023
…l-mode-for-RequestInit-fix, r=<try>

Use same-origin as default credential mode for RequestInit fix

<!-- Please describe your changes on the following line: -->
- Updated the default credential mode from omit to same-origin as per: whatwg/fetch#1153
- Deleted instances of fallback credentials
---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29633  (GitHub issue number if applicable)
- [X] There are tests for these changes
bors-servo added a commit to servo/servo that referenced this pull request May 2, 2023
…l-mode-for-RequestInit-fix, r=<try>

Use same-origin as default credential mode for RequestInit fix

<!-- Please describe your changes on the following line: -->
- Updated the default credential mode from omit to same-origin as per: whatwg/fetch#1153
- Deleted instances of fallback credentials
---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29633  (GitHub issue number if applicable)
- [X] There are tests for these changes
bors-servo added a commit to servo/servo that referenced this pull request May 18, 2023
…l-mode-for-RequestInit-fix, r=<try>

Use same-origin as default credential mode for RequestInit fix

<!-- Please describe your changes on the following line: -->
- Updated the default credential mode from omit to same-origin as per: whatwg/fetch#1153
- Deleted instances of fallback credentials
---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29633  (GitHub issue number if applicable)
- [X] There are tests for these changes
bors-servo added a commit to servo/servo that referenced this pull request May 18, 2023
…l-mode-for-RequestInit-fix, r=mukilan

Use same-origin as default credential mode for RequestInit fix

<!-- Please describe your changes on the following line: -->
- Updated the default credential mode from omit to same-origin as per: whatwg/fetch#1153
- Deleted instances of fallback credentials
---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29633  (GitHub issue number if applicable)
- [X] There are tests for these changes
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.

"Set fallbackCredentials to "same-origin"."
2 participants