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 clobbering of overridden canvas methods #2658

Merged
merged 5 commits into from
Aug 17, 2020

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Jul 30, 2020

The method may be overridden again after we override,
as is done in the `hidpi-canvas-polyfill`. So if we
restore the method to the original as we see it, we'll
wipe out the polyfill.

I'd like to write tests for this, but I see very little related to canvas fingerprinting, and what is there is pretty inscrutable (where is fingerprinting.html?)

Fixes #2657, fixes #1226, fixes #1854.

The method may be overridden again after we override,
as is done in the `hidpi-canvas-polyfill`. So if we
restore the method to the original as we see it, we'll
wipe out the polyfill.

Fixes EFForg#2657, EFForg#1226
@ghostwords
Copy link
Member

Hello, and thank you for the report and the PR! I will take a proper look next week.

fingerprinting.html lives in https://github.com/EFForg/privacybadger-test-fixtures. (We should probably document where fixtures live somewhere in our tests doc.)

Let me know what other questions you have. Thanks again!

@STRML
Copy link
Contributor Author

STRML commented Jul 31, 2020

Thanks @ghostwords. Perhaps this should be same-repo, and we just run a local webserver rather than hit the github.io page? Adding or modifying a test requires now coordinating two PRs, which is quite difficult. Regardless, I'll make a PR there that correlates to this one, but it will need to be accepted and the tests re-run here.

STRML added a commit to STRML/privacybadger-test-fixtures that referenced this pull request Jul 31, 2020
STRML added a commit to STRML/privacybadger-test-fixtures that referenced this pull request Jul 31, 2020
@ghostwords
Copy link
Member

ghostwords commented Jul 31, 2020

Yeah, the coordination is a bit of a pain. We used to host test fixtures on various public JS playground/gist hosts, but for reasons like reliability and control over what actually gets served we migrated to the two GH Pages repos we have now. #928 has a bit of history and links to some relevant commits.

A local fixtures server would be nice, but we need support for things like https and distinct-to-Privacy Badger hostnames (to simulate first and third-party interactions). (Localhost domains are probably not a good fit since they should probably be ignored: #817)

@STRML
Copy link
Contributor Author

STRML commented Jul 31, 2020

That's fair enough. So we'd have to run a system-level dns resolver and point the local browsers to it... quite a song & dance. Doubt it's worth the effort.

Should be able to figure out that "Google Chrome" is chrome
@STRML
Copy link
Contributor Author

STRML commented Jul 31, 2020

Build failing, I'll ping when this is ready for merge.

@ghostwords
Copy link
Member

ghostwords commented Aug 7, 2020

I should have some time to look into what's going on with the test next week, if that's helpful.

@STRML
Copy link
Contributor Author

STRML commented Aug 7, 2020

It would be, thanks. When running locally I have no issue.

@ghostwords
Copy link
Member

I think the test doesn't work on Travis because it doesn't run in a HiDPI environment. We could fake it with something like window.devicePixelRatio = 2;. Still looking though.

@ghostwords
Copy link
Member

ghostwords commented Aug 13, 2020

Looks like that was it, the polyfill wasn't kicking in on Travis. I added the devicePixelRatio override, the test should pass now.

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

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

Nice, thank you! Looks good so far outside of some nits. Let me know if I should take care of them myself.

tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
tests/selenium/fingerprinting_canvas_hidpi_test.py Outdated Show resolved Hide resolved
src/js/contentscripts/fingerprinting.js Outdated Show resolved Hide resolved
@STRML
Copy link
Contributor Author

STRML commented Aug 14, 2020

@ghostwords We're green, thanks a lot for the help and for catching the fixture issue, I wouldn't have caught that.

I can clean up the history if you like, or if you're just going to squash it should be no problem.

@ghostwords
Copy link
Member

Thank you for the contribution! I'll finish testing next week, expect to merge then in time for the next Privacy Badger update.

I won't squash merge as I would rather preserve the individual commits. You're welcome to clean up history, but it's not necessary.

@ghostwords ghostwords merged commit 2df3d90 into EFForg:master Aug 17, 2020
@ghostwords
Copy link
Member

Thanks again!

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