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

Viewport Protection site setting #2175

Closed
wants to merge 18 commits into from
Closed

Conversation

uazo
Copy link
Collaborator

@uazo uazo commented Jul 6, 2022

Description

this is the patch I was talking about in #2128 but that I haven't tried on a real device yet.
this is a second version, much more simplified than a first draft which operated at a lower level, but which I deleted because it was too complex.
basically the idea is to change the size of the viewport on each page by exploiting the override of the initial-scale metatag with a site setting that allows you to select any exceptions.
in parallel, the patch acts on the information of the screen and on the device-width of the css.

All submissions

  • there are no other open Pull Requests for the same update/change
  • Bromite can be built with these changes
  • I have tested that the new change works as intended (AVD or physical device will do)

Format

  • patch subject and filename match (e.g. Subject: Alternative cache (NIK-based) -> Alternative-cache-NIK-based.patch)
  • patch description contains explanation of changes
  • no unnecessary whitespace or unrelated changes

+
+ if (settings->AllowViewportChange(false)) {
+ if (page->PageWidthOverride() == 0) {
+ page->SetPageWidthOverride(base::RandInt(-5, 15));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is where the random scale is decided.
the factor I have set for now is -5% to +15% compared to the current size.
we probably need to check if it is a correct range.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-5 was too annoying, I changed to -3 ... +15 with a factor of 10

@csagan5 csagan5 marked this pull request as draft July 9, 2022 10:02
@csagan5 csagan5 changed the title [draft] Viewport Protection site setting Viewport Protection site setting Jul 9, 2022
@uazo
Copy link
Collaborator Author

uazo commented Jul 21, 2022

I think i finished the patch, let me know if you find it interesting.
I'm testing this patch on my device with the normal navigation I do, I'll let you know if I find any bugs.

@csagan5
Copy link
Contributor

csagan5 commented Jul 25, 2022

Does this mitigation supersede any pre-existing one?

If it is ready you can remove draft status.

@uazo
Copy link
Collaborator Author

uazo commented Jul 25, 2022

boring little thing that i noticed.
because of this patch, the distiller often and willingly suggests me to view the page in lite mode.

being written in gwt I don't really want to learn a new framework (to understand how to fix it), can I propose to add a flag to deactivate it (with a new flag in accessibility), considering that the actual flag chrome://flags/#reader-mode-heuristics does not work?

aaaaaa, it came to my mind now.
here's the way to bypass that protection! damn I have to study gwt :(

@uazo uazo marked this pull request as ready for review July 25, 2022 08:33
@uazo uazo marked this pull request as draft July 25, 2022 09:24
fix incorrect site setting value at startup
remove code from visual_viewport
remove devicePixelRatio override
@uazo
Copy link
Collaborator Author

uazo commented Jul 25, 2022

I have to study gwt

there was no need, the value was in blink

@uazo uazo marked this pull request as ready for review July 25, 2022 15:46
@uazo uazo marked this pull request as draft July 26, 2022 19:13
@uazo
Copy link
Collaborator Author

uazo commented Jul 27, 2022

sorry if i keep changing the patch status.
but I'm testing the changes with the privacytests.org test result and fail miserably.
then I discovered that the same author is proposing a similar change to brave, maybe it's better I wanted to check

@uazo
Copy link
Collaborator Author

uazo commented Aug 9, 2022

but I'm testing the changes with the privacytests.org test result and fail miserably.

some quick updates.
the tests fail because they are "customized" for that patch, ie made as indicated by the purpose of that pull, which unfortunately only works for desktops and, in my opinion, does not eliminate the problem at its root.
what I am going to propose to you (and that I rewrote for the third time, and now I like it better :) will fail those tests but it should be a more radical approach (if it works, I hope, you will tell me).

Does this mitigation supersede any pre-existing one?

I checked this too. unfortunately not, at least not as I thought.
the problem is that that type of script uses css absolute positioning and looks for insignificant decimals, probably because depending on the device, the value changes imperceptibly.
to solve that it would perhaps be better to delete the values after the second decimal, because, in fact, they are useless to a site and are easily suitable for fingerprinting. the point of code is unique in all blink, so a possible patch could also be simple to implement
but i will let you know better about that too (as soon as i get back :)

@uazo
Copy link
Collaborator Author

uazo commented Aug 26, 2022

I close this one and do another one, I messed with the rebase ...

@uazo uazo closed this Aug 26, 2022
@uazo uazo mentioned this pull request Aug 26, 2022
6 tasks
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.

4 participants