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.

Sign up for GitHub

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

Jump to bottom

Credentials on scripts imported by importScripts() #1497

Open
makotoshimazu opened this issue Jan 15, 2020 · 4 comments
Open

Credentials on scripts imported by importScripts() #1497

makotoshimazu opened this issue Jan 15, 2020 · 4 comments

Comments

@makotoshimazu
Copy link

This is similar to whatwg/html#2557.
Chrome now has a bug around the credentials mode (issue), and I'm now wondering what is the correct value for the request's credentials mode.

As far as I read the spec, importScripts() uses fetch a classic worker-imported script which doesn't set any value to the credentials mode, while the main script is set to "same-origin". The default value of the request's credentials mode is "omit", so I think it's "omit".

I implemented the byte-for-byte checking in that way recently, but found that the existing implementation seems to use "include".
What would be the best value for that?

So far, I'm feeling that using "same-origin" would be less confusing.
@mfalken
Copy link
Member

mfalken commented Jan 17, 2020

It looks like we changed fetch() to use "same-origin" credentials by default (whatwg/fetch#585), probably "importScripts()" in workers should also do that?

It'd be useful to see what other browsers do here. cc/ @jakearchibald @annevk

@annevk
Copy link
Member

annevk commented Jan 17, 2020

I would expect importScripts() to use "include", similar to classic scripts. Or is importScripts() different from classic scripts in service workers?

@annevk
Copy link
Member

annevk commented Jan 17, 2020

cc @domenic

@domenic
Copy link
Contributor

domenic commented Jan 17, 2020

I agree with @annevk that I would expect it to use "include". I suspect this was lost in a refactoring. Some research:

  • The earliest HTML commit snapshot, before integration with the Fetch Standard, uses a "fetch" algorithm defined in HTML. That algorithm sends cookies by default unless invoked with the "block cookies" flag, which is not the case here.
  • This old revision explicitly sets credentials mode to "include".

I would guess this got lost when introducing module scripts, but I didn't spend the time to confirm.

"same-origin" would be a nice tightening, but probably we should just go back to what the old spec says.

A spec pull request and web platform tests would be very welcome, if you have the time.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 31, 2020
@makotoshimazu @chromium-wpt-export-bot
ServiceWorkerSingleScriptUpdateChecker has credentials
1697c36 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 31, 2020
@makotoshimazu @chromium-wpt-export-bot
ServiceWorkerSingleScriptUpdateChecker has credentials
fb7c257 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 31, 2020
@makotoshimazu @chromium-wpt-export-bot
ServiceWorkerSingleScriptUpdateChecker has credentials
fc8a4be 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}
pull bot pushed a commit to FreddyZeng/chromium that referenced this issue Jan 31, 2020
@makotoshimazu
ServiceWorkerSingleScriptUpdateChecker has credentials
20aa9a3 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 31, 2020
@makotoshimazu @chromium-wpt-export-bot
ServiceWorkerSingleScriptUpdateChecker has credentials
acc51c1 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 11, 2020
@makotoshimazu @moz-wptsync-bot
Bug 1612485 [wpt PR 21528] - ServiceWorkerSingleScriptUpdateChecker h…
2050493 
…as credentials, a=testonly

Automatic update from web-platform-tests
ServiceWorkerSingleScriptUpdateChecker has credentials

Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}

--

wpt-commits: acc51c1f9cb98db4419132c4cf8510c13e958f0f
wpt-pr: 21528
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Feb 12, 2020
@marco-c
Bug 1612485 [wpt PR 21528] - ServiceWorkerSingleScriptUpdateChecker h…
0d40320 
…as credentials, a=testonly

Automatic update from web-platform-tests
ServiceWorkerSingleScriptUpdateChecker has credentials

Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhirokichromium.org>
Commit-Queue: Makoto Shimazu <shimazuchromium.org>
Cr-Commit-Position: refs/heads/master{#737253}

--

wpt-commits: acc51c1f9cb98db4419132c4cf8510c13e958f0f
wpt-pr: 21528

UltraBlame original commit: e52a0f3b0f4edb3b614ad50b400d26bbcdd3f20e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Feb 12, 2020
@marco-c
Bug 1612485 [wpt PR 21528] - ServiceWorkerSingleScriptUpdateChecker h…
9b6fb31 
…as credentials, a=testonly

Automatic update from web-platform-tests
ServiceWorkerSingleScriptUpdateChecker has credentials

Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhirokichromium.org>
Commit-Queue: Makoto Shimazu <shimazuchromium.org>
Cr-Commit-Position: refs/heads/master{#737253}

--

wpt-commits: acc51c1f9cb98db4419132c4cf8510c13e958f0f
wpt-pr: 21528

UltraBlame original commit: e52a0f3b0f4edb3b614ad50b400d26bbcdd3f20e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Feb 12, 2020
@marco-c
Bug 1612485 [wpt PR 21528] - ServiceWorkerSingleScriptUpdateChecker h…
4777548 
…as credentials, a=testonly

Automatic update from web-platform-tests
ServiceWorkerSingleScriptUpdateChecker has credentials

Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhirokichromium.org>
Commit-Queue: Makoto Shimazu <shimazuchromium.org>
Cr-Commit-Position: refs/heads/master{#737253}

--

wpt-commits: acc51c1f9cb98db4419132c4cf8510c13e958f0f
wpt-pr: 21528

UltraBlame original commit: e52a0f3b0f4edb3b614ad50b400d26bbcdd3f20e
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Feb 12, 2020
@makotoshimazu @moz-wptsync-bot
Bug 1612485 [wpt PR 21528] - ServiceWorkerSingleScriptUpdateChecker h…
e990148 
…as credentials, a=testonly

Automatic update from web-platform-tests
ServiceWorkerSingleScriptUpdateChecker has credentials

Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}

--

wpt-commits: acc51c1f9cb98db4419132c4cf8510c13e958f0f
wpt-pr: 21528
pdigennaro pushed a commit to washezium/washezium that referenced this issue May 3, 2020
@makotoshimazu
ServiceWorkerSingleScriptUpdateChecker has credentials
a737e40 
Before the new byte-for-byte checking, credentials_mode was set to
kInclude for imported scripts. However,
ServiceWorkerSingleScriptUpdateChecker sets the flag to kOmit because
it's missed from the current spec. This CL is to correct the mode to
fix the unintentional change.
Spec issue: w3c/ServiceWorker#1497

Bug: 1042159
Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032689
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737253}
(cherry picked from commit 20aa9a3)

TBR=shimazu@chromium.org

Change-Id: I553d7bec015eb4cb80f7f59c640f26491fa02b75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037842
Reviewed-by: Krishna Govind <govind@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/branch-heads/4044@{chromium#45}
Cr-Branched-From: a6d9daf-refs/heads/master@{#737173}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@domenic @annevk @mfalken @makotoshimazu