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 inability to open Gmail when using reduce-tracking-mode. #3403

Closed

Conversation

shamazmazum
Copy link
Contributor

This PR fixes #3402. Fix strip-tracking-parameters which was damaging some kind of URLs.

@shamazmazum shamazmazum changed the title Fix inability to open Gmail when using reduce-tracking-mode. [DO NOT MERGE]Fix inability to open Gmail when using reduce-tracking-mode. May 23, 2024
@shamazmazum
Copy link
Contributor Author

Oops, I am sorry. Gmail opens fine, but search queries which contain spaces are now causing an endless loop. Looking for another solution.

Copy link
Member

@aadcg aadcg left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @shamazmazum. I've left some comments.

Please add a changelog entry as a separate commit.

source/mode/reduce-tracking.lisp Outdated Show resolved Hide resolved
@@ -56,15 +56,36 @@ still being less noticeable in the crowd.")
:export nil
:documentation "The timezone the system had before enabling this mode.")))

;; NB: QURI:URL-ENCODE-PARAMS is not applicable for filtering a
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest moving the comment from the codebase to the commit message. Also, the specific case of gmail could be used as a test for reduce-tracking-mode. The commit message should mention, in more general terms, the root of the issue instead of focusing in this concrete case.

@shamazmazum shamazmazum changed the title [DO NOT MERGE]Fix inability to open Gmail when using reduce-tracking-mode. Fix inability to open Gmail when using reduce-tracking-mode. May 23, 2024
@aadcg
Copy link
Member

aadcg commented May 23, 2024

@shamazmazum I took a brief look at the recent changes. Would it make sense to add a variant of quri:url-encode-params or refactor it in such a way that percent-encoding is optional? I am a maintainer of quri.

@shamazmazum
Copy link
Contributor Author

shamazmazum commented May 23, 2024

@aadcg Where to put a new entry in the changelog? 4.0.0? 3.11.7? What is the next release? 3.11.7, as I presume.

Few comments on code:

  • The comment should be in the code, as it is tempting for someone not familiar with the situation to change it back to using quri. It should be clear why one should not do so.
  • url-filter-params is good at least for debugging. It does not require a string to be wrapped in two layers of abstraction.

I agree with the rest.

@aadcg
Copy link
Member

aadcg commented May 23, 2024

@shamazmazum 3.11.7, thanks.

Your argumentation makes sense.

@shamazmazum
Copy link
Contributor Author

@shamazmazum I took a brief look at the recent changes. Would it make sense to add a variant of quri:url-encode-params or refactor it in such a way that percent-encoding is optional? I am a maintainer of quri.

Yes, it would be better. I'll take a look at it. It can take some time, though :)

@aadcg
Copy link
Member

aadcg commented May 23, 2024

Yes, it would be better. I'll take a look at it. It can take some time, though :)

No pressure! Thanks.

@shamazmazum
Copy link
Contributor Author

OK, I've added a PR to quri, waiting for approval. After it is merged, I'll modify this PR to use quri for correct handling of parameters.

@aadcg
Copy link
Member

aadcg commented May 28, 2024

Sounds good @shamazmazum, thanks.

STRIP-TRACKING-PARAMETERS added an extra level of percent encoding to
a request which resulted in broken redirect and inability to open
some pages like Google Mail.
@shamazmazum
Copy link
Contributor Author

I think, it's done :)

@aadcg
Copy link
Member

aadcg commented May 31, 2024

@shamazmazum I've merged with PR with minor changes via commit 6a7afab, thanks!

@aadcg aadcg closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Unable to navigate to gmail.com
2 participants