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

Fix percent encoding errors for param stripping #1988

Merged
merged 9 commits into from May 19, 2023
Merged

Conversation

SlayterDev
Copy link
Contributor

@SlayterDev SlayterDev commented May 18, 2023

Reviewer: @jonathanKingston @kzar

Description:

This fixes url param stripping when parameters aren't properly percent decoded. Note I've also removed code that deals with Regex parameters as we no longer have these in our lists.

Steps to test this PR:

Make sure to enable queryParameters in your local config.

Check the tests here: https://privacy-test-pages.glitch.me/privacy-protections/query-parameters/

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@kzar
Copy link
Collaborator

kzar commented May 19, 2023

Looks good @SlayterDev , I've left some comments.

At a higher level though, I wonder if this should be two changes.

  1. First change for rethinking uses of searchParams throughout the codebase. There might be other places where we use the API to add/remove parameters, and therefore adjust the encoding of parameters by mistake.
  2. Second change for removing support for regular expression parameter stripping.

What do you think?

@jonathanKingston
Copy link
Collaborator

First change for rethinking uses of searchParams throughout the codebase. There might be other places where we use the API to add/remove parameters, and therefore adjust the encoding of parameters by mistake.

That sounds like a follow up investigation. I'd concentrate on this issue first to not add further risk.

SlayterDev and others added 2 commits May 19, 2023 09:26
Co-authored-by: Dave Vandyke <kzar@kzar.co.uk>
Copy link
Collaborator

@kzar kzar left a comment

Choose a reason for hiding this comment

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

Some failing unit tests to fix, but otherwise LGTM

@SlayterDev SlayterDev merged commit 3430286 into main May 19, 2023
19 checks passed
SlayterDev added a commit to duckduckgo/privacy-configuration that referenced this pull request May 25, 2023
sammacbeth pushed a commit to duckduckgo/privacy-configuration that referenced this pull request May 25, 2023
* Re-enable URL parameter stripping in the extension

This has been fixed in duckduckgo/duckduckgo-privacy-extension#1988

* Add min version
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.

None yet

3 participants