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

Bug 1812797 - Add referrerUrl to Request for download features #2535

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

jackyzy823
Copy link
Contributor

@jackyzy823 jackyzy823 commented Jun 19, 2023

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1812797

@github-actions github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Jun 19, 2023
@mavduevskiy
Copy link
Contributor

mavduevskiy commented Jun 29, 2023

Hey, jackyzy823! First of all, thank you so much for your contributions. Really appreciate it.

I tested the patch, and it actually makes the given use case, but passing the url that might contain sensitive information into the referrer field imposes a lot of security risks. We might need a different approach here.

Previous discussions resulted in some follow ups for GeckoView, seeing @jonalmeida comment I get a sense that's the direction we were going for. I also see that the geckoview bug got higher priority, so I assume there might be some work going on during this nightly.

I don't see anybody assigned to it though. And a separate question is, what kind of data we would like to pass with the request in this case? I don't have a lot of context on requests, but after some research we might want use different header like origin or we could add policies to the referrer field via Referrer-Policy header.

@jackyzy823
Copy link
Contributor Author

Hi, @mavduevskiy. I think GeckoViewFetchClient has ability to handle referrer properly according to referer policy.

The reason why i think so:

So , no need to worry about information leak, Gecko will take care of it.

I also explain this in Matrix channel for someone asking GeckoSession.Loader's referrer issue: link , You can check that if you want more detailed explanations.

@gunir
Copy link

gunir commented Oct 19, 2023

I tested the patch, and it actually makes the given use case, but passing the url that might contain sensitive information into the referrer field imposes a lot of security risks. We might need a different approach here.

I know that Firefox is a privacy browser, but it really doesn't matter a lot when downloading images, all we need to do is just strip the path and keep only the domain like the way we're currently doing in desktop.

We rarely even download images using Firefox Android.

But well this bug needs to be sorted out soon because users are suffering.

Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

Awesome work 👏🏽
Thank you so much for this patch!

@Amejia481 Amejia481 added 🛬 needs landing PRs that are ready to land approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Oct 23, 2023
@mergify mergify bot merged commit 9947b70 into mozilla-mobile:main Oct 23, 2023
109 checks passed
@jackyzy823 jackyzy823 deleted the bug-1812797-referrer branch October 24, 2023 02:46
@jackyzy823 jackyzy823 restored the bug-1812797-referrer branch October 24, 2023 15:44
@jackyzy823 jackyzy823 deleted the bug-1812797-referrer branch February 27, 2024 14:16
@jackyzy823 jackyzy823 restored the bug-1812797-referrer branch February 27, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants