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

feat(extensions/fetch): Add Origin header to outgoing requests for fetch #11557

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

F3n67u
Copy link
Contributor

@F3n67u F3n67u commented Jul 30, 2021

fix #10992

@F3n67u F3n67u force-pushed the fetch-origin-header branch 4 times, most recently from 0cb7110 to 5663b6c Compare July 30, 2021 16:06
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

@F3n67u Thanks for the patch. LGTM.

@ry ry merged commit f87aa44 into denoland:main Aug 2, 2021
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Looking at the spec again, I am unsure about adding the header now. It is meant for CORS and doesn't really make much sense other than spec compliance.

Comment on lines +434 to +444
const baseURL = getLocationHref();
if (
baseURL &&
(requestObject.method !== "GET" && requestObject.method !== "HEAD")
) {
ArrayPrototypePush(request.headerList, [
"origin",
new URL(baseURL).origin,
]);
}

Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. The header should be added of a different function in the implementation. The spec explains this.

The origin header should also respect request referrer policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, take a look at where it is called.

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Aug 2, 2021
bartlomieju added a commit that referenced this pull request Aug 2, 2021
@F3n67u
Copy link
Contributor Author

F3n67u commented Aug 2, 2021

Looking at the spec again, I am unsure about adding the header now. It is meant for CORS and doesn't really make much sense other than spec compliance.

Is there any use case where origin header should added? CORS is not a problem for deno because deno doesn't follow same-origin policy now.

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

Successfully merging this pull request may close these issues.

Add Origin header to outgoing requests for fetch
3 participants