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

[Chrome extension] Add Referer request header if needed #5823

Merged
merged 1 commit into from
Jun 5, 2015

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Mar 10, 2015

This patch adds the Referer request header to PDF requests if the original PDF request included the Referer header.

Fixes #5676.

Review on Reviewable

@timvandermeij
Copy link
Contributor

Do you have steps on how to verify this feature? Most importantly a website that requires a referrer.

Perhaps @Hengjie could also test this?

@Rob--W
Copy link
Member Author

Rob--W commented Mar 17, 2015

@timvandermeij

Steps to verify:

  1. Load extension: node make chromium and chromium --user-data-dir=/tmp/whatever --load-extension=build/chromium
  2. Visit a page that links to a PDF where the referrer will be sent (e.g. https -> http pdf = no referrer).
    For example, https://encrypted.google.com/search?q=file%3Apdf+bitcoin.
  3. Open chrome://net-internals/#events
  4. Click on the link to the PDF from step 2.
  5. Go back to chrome://net-internals/#events, click on the URL_REQUEST for the PDF (type "pdf" in the imput box to filter the displayed requests) and make sure that the Referer header has been added.
  6. Refresh the PDF viewer and verify that the referer header is still added (using the same method as step 5).

verify-referer

@Rob--W
Copy link
Member Author

Rob--W commented Apr 10, 2015

Ping, could anyone review this?

@timvandermeij

@timvandermeij
Copy link
Contributor

I'm sorry to put this on @yurydelendik's review stack, but I do not know enough about this part of the codebase to properly review this (other than testing it of course).

There does appear to be a merge conflict. Perhaps that should be resolved first?

@Rob--W Rob--W force-pushed the preserve-http-referer branch from 56d9623 to 457a076 Compare April 10, 2015 20:37
@Rob--W
Copy link
Member Author

Rob--W commented Apr 10, 2015

Rebased PR.

@Hengjie
Copy link
Contributor

Hengjie commented May 10, 2015

@timvandermeij @Rob--W I can confirm it's fixed, thanks! I tested it by seting up a nginx server locally that prevented "hotlinking" by checking the referral.

@TimothyGu
Copy link
Contributor

Ping.

@yurydelendik
Copy link
Contributor

@Rob--W looks good, can you merge this in?

@Rob--W
Copy link
Member Author

Rob--W commented Jun 4, 2015

@yurydelendik Sure, I'll do this over the weekend.

@timvandermeij timvandermeij assigned Rob--W and unassigned yurydelendik Jun 4, 2015
This patch adds the Referer request header to PDF requests if
the original PDF request included the Referer header.
@Rob--W Rob--W force-pushed the preserve-http-referer branch from 457a076 to adb2f8a Compare June 5, 2015 21:28
@Rob--W
Copy link
Member Author

Rob--W commented Jun 5, 2015

Rebased and resolved trivial merge conflict (caused by an intermediate refactor of web/pdf_history.js that affected the indentation of the whole file).

Using the steps in #5823 (comment), I also verified that the patch still works in Chromium 42.0.2311.90 and 45.0.2416.0.

Rob--W added a commit that referenced this pull request Jun 5, 2015
[Chrome extension] Add Referer request header if needed
@Rob--W Rob--W merged commit 8aa1c81 into mozilla:master Jun 5, 2015
@yurydelendik
Copy link
Contributor

Thank you for the patch.

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

Successfully merging this pull request may close these issues.

Not sending referrals in Chrome
5 participants