fix: Don't ignore the referer header in net.request #23386

zeeker999 · 2020-05-02T14:41:58Z

Description of Change

Don't ignore the referrer header for a request, fixes #22411 #23102.

@zcbenz @MarshallOfSound

Checklist

PR description included and stakeholders cc'd
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Don't ignore the referrer header in net.request

nornagon

Hey, thanks for this! Would you mind adding a test in spec-main/api-net-spec.ts? Thanks!

zeeker999 · 2020-05-11T23:41:08Z

Done. Could please review my changes again.

zeeker999 · 2020-05-17T03:44:08Z

@nornagon @zcbenz any comment for my latest change?

MarshallOfSound · 2020-05-17T08:46:34Z

shell/browser/api/electron_api_url_loader.cc

@@ -352,6 +352,7 @@ gin::Handle<SimpleURLLoaderWrapper> SimpleURLLoaderWrapper::Create(
  request->force_ignore_site_for_cookies = true;
  opts.Get("method", &request->method);
  opts.Get("url", &request->url);
+  opts.Get("referrer", &request->referrer);


Is there a reason this has to be a new option and we can't just read it off the headers object if it's defined? That would make it easier to use I think / be more expected that the header just works ™️

En, this way works too and less burden for user, but the current approach matches the window.fetch() better and even better if we'll introduce the referrer policy later. I don't have opinion which one is better.

I think we should probably go with what @MarshallOfSound said and read this out of the headers option.

This API is designed to be similar to the NodeJS http API (though it doesn't match it in every respect). In the NodeJS API, setting the Referer header is done through the headers object (or setHeader), so it would be more consistent with NodeJS for us to respect that here. Additionally, the two linked bugs in the PR description show two examples of how people expect the API to work, which is similarly to the NodeJS API. I think we can avoid confusion by matching NodeJS here.

Ok, I just pushed the branch forcely and changed the code to read referrer url from headers instead.

jkleinsc

LGTM - just need to remove the unneeded ts change

typings/internal-ambient.d.ts

0xdeadbae · 2020-05-20T01:10:15Z

I have the same issue on Windows 1909. Electron 8.2.5.

Does this fix also solve the issue for Windows user?

zeeker999 · 2020-05-20T01:45:18Z

I have the same issue on Windows 1909. Electron 8.2.5.

Does this fix also solve the issue for Windows user?

Yes, this pull request fixes the issue for all platforms.

release-clerk · 2020-05-20T17:28:42Z

Release Notes Persisted

Don't ignore the referrer header in net.request

trop · 2020-05-20T19:24:50Z

I was unable to backport this PR to "7-3-x" cleanly;
you will need to perform this backport manually.

trop · 2020-05-20T19:24:53Z

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

trop · 2020-05-20T19:24:56Z

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

nornagon · 2020-05-20T19:25:11Z

welp

zeeker999 · 2020-05-20T21:16:18Z

Should we backport the 7e841ce to stable branches first? If we don't want backport it, I think I have to backport this pull manually.

nornagon · 2020-05-20T21:34:05Z

I don't think we should backport 7e841ce to older branches, please go ahead and create manual backports.

(there's no need to backport to 7.x since this was only an issue since the 8.x branch)

zeeker999 · 2020-05-20T21:37:33Z

Ok. I'm going to backport this manually.

trop · 2020-05-20T21:48:07Z

@LuoJinghua has manually backported this PR to "9-x-y", please check out #23685

trop · 2020-05-20T21:49:05Z

@LuoJinghua has manually backported this PR to "master", please check out #23686

trop · 2020-05-20T21:55:49Z

@LuoJinghua has manually backported this PR to "8-x-y", please check out #23688

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 2, 2020

zeeker999 force-pushed the net-referrer-master branch from a240622 to 7b488f6 Compare May 2, 2020 15:00

zeeker999 changed the title ~~feat: Add an referrer option to net.request~~ feat: Add a referrer option to net.request May 2, 2020

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 3, 2020

zcbenz approved these changes May 11, 2020

View reviewed changes

zcbenz added the api-review/requested 🗳 label May 11, 2020

zcbenz requested a review from a team May 11, 2020 05:42

nornagon suggested changes May 11, 2020

View reviewed changes

zeeker999 force-pushed the net-referrer-master branch from 7b488f6 to 6543833 Compare May 11, 2020 23:39

zeeker999 requested a review from nornagon May 12, 2020 01:27

MarshallOfSound reviewed May 17, 2020

View reviewed changes

zeeker999 force-pushed the net-referrer-master branch from 6543833 to 7b3e418 Compare May 19, 2020 04:44

zeeker999 changed the title ~~feat: Add a referrer option to net.request~~ feat: Don't ignore the referrer header in net.request May 19, 2020

zeeker999 force-pushed the net-referrer-master branch from 7b3e418 to 528d622 Compare May 19, 2020 05:08

zeeker999 changed the title ~~feat: Don't ignore the referrer header in net.request~~ feat: Don't ignore the referer header in net.request May 19, 2020

jkleinsc added api-review/approved ✅ and removed api-review/requested 🗳 labels May 19, 2020

nornagon approved these changes May 19, 2020

View reviewed changes

jkleinsc requested changes May 19, 2020

View reviewed changes

typings/internal-ambient.d.ts Show resolved Hide resolved

zeeker999 force-pushed the net-referrer-master branch from 528d622 to 1ac1779 Compare May 19, 2020 23:06

net: Don't ignore the referer header in net.request

1924398

zeeker999 force-pushed the net-referrer-master branch from 1ac1779 to 1924398 Compare May 20, 2020 01:48

zeeker999 changed the title ~~feat: Don't ignore the referer header in net.request~~ fix: Don't ignore the referer header in net.request May 20, 2020

jkleinsc approved these changes May 20, 2020

View reviewed changes

jkleinsc merged commit 9d851b8 into electron:master May 20, 2020

trop bot added the needs-manual-bp/7-3-x label May 20, 2020

trop bot added needs-manual-bp/8-x-y and removed target/8-x-y labels May 20, 2020

trop bot added needs-manual-bp/9-x-y and removed target/9-x-y labels May 20, 2020

nornagon added target/9-x-y and removed needs-manual-bp/7-3-x labels May 20, 2020

zeeker999 mentioned this pull request May 20, 2020

fix: Don't ignore the referer header in net.request #23685

Merged

trop bot added in-flight/9-x-y and removed target/9-x-y labels May 20, 2020

zeeker999 mentioned this pull request May 20, 2020

fix: Don't ignore the referer header in net.request #23686

Closed

trop bot added the in-flight/master label May 20, 2020

zeeker999 mentioned this pull request May 20, 2020

fix: Don't ignore the referer header in net.request #23688

Merged

trop bot added in-flight/8-x-y and removed needs-manual-bp/8-x-y labels May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't ignore the referer header in net.request #23386

fix: Don't ignore the referer header in net.request #23386

zeeker999 commented May 2, 2020 •

edited

Loading

nornagon left a comment

zeeker999 commented May 11, 2020 •

edited

Loading

zeeker999 commented May 17, 2020

MarshallOfSound May 17, 2020

zeeker999 May 17, 2020

nornagon May 18, 2020

zeeker999 May 19, 2020 •

edited

Loading

jkleinsc left a comment

0xdeadbae commented May 20, 2020 •

edited

Loading

zeeker999 commented May 20, 2020

release-clerk bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

nornagon commented May 20, 2020

zeeker999 commented May 20, 2020 •

edited

Loading

nornagon commented May 20, 2020

zeeker999 commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

fix: Don't ignore the referer header in net.request #23386

fix: Don't ignore the referer header in net.request #23386

Conversation

zeeker999 commented May 2, 2020 • edited Loading

Description of Change

Checklist

Release Notes

nornagon left a comment

Choose a reason for hiding this comment

zeeker999 commented May 11, 2020 • edited Loading

zeeker999 commented May 17, 2020

MarshallOfSound May 17, 2020

Choose a reason for hiding this comment

zeeker999 May 17, 2020

Choose a reason for hiding this comment

nornagon May 18, 2020

Choose a reason for hiding this comment

zeeker999 May 19, 2020 • edited Loading

Choose a reason for hiding this comment

jkleinsc left a comment

Choose a reason for hiding this comment

0xdeadbae commented May 20, 2020 • edited Loading

zeeker999 commented May 20, 2020

release-clerk bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

nornagon commented May 20, 2020

zeeker999 commented May 20, 2020 • edited Loading

nornagon commented May 20, 2020

zeeker999 commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

trop bot commented May 20, 2020

zeeker999 commented May 2, 2020 •

edited

Loading

zeeker999 commented May 11, 2020 •

edited

Loading

zeeker999 May 19, 2020 •

edited

Loading

0xdeadbae commented May 20, 2020 •

edited

Loading

zeeker999 commented May 20, 2020 •

edited

Loading