Fix websockets bypass in Brave filtering #7121
Conversation
test/components/braveryPanelTest.js
Outdated
| }) | ||
| // Assume blocking websockets is working if the time difference between | ||
| // onerror firing with adblock on vs off is sufficiently high. | ||
| assert(time2 - time1 > 100) |
There was a problem hiding this comment.
fwiw this test would be nicer if there were a way in webdriver to inspect the page console output. if the connection is blocked in app/filtering.js, it will show a CONNECTION_BLOCKED message. it shows a different error message when the connection is attempted but fails.
There was a problem hiding this comment.
shouldn't it succeed if it isn't blocked?
There was a problem hiding this comment.
@bridiver no because "ws://ag.innovid.com/dv/sync?tid=2" doesn't support websockets. it fails either way: either because it's blocked by adblock (the fast case) or because the server response is incorrect (the slow case)
There was a problem hiding this comment.
that seems really fragile to me. I think we should mock up or add something that should be blocked. For now maybe we could even do it through site hacks in test only
test/fixtures/websockets.html
Outdated
| <div id='errorTime'></div> | ||
| <script> | ||
| // This should get blocked by adblock if it is on | ||
| var exampleSocket = new WebSocket("ws://ag.innovid.com/dv/sync?tid=2"); |
There was a problem hiding this comment.
can we use webpack for the blocked ws to prevent a false positive for the error?
There was a problem hiding this comment.
not sure i understand. the webpack ws URL is not in adblock, so it shouldn't get blocked?
There was a problem hiding this comment.
see comment above about mocking, but are there any patterns for ad block that have a wildcard domain? Another options would be to have a separate adblock file for testing or add a localhost match with a uuid path (wildcard port)
test/components/braveryPanelTest.js
Outdated
| let time1 = 0 | ||
| let time2 = 0 | ||
| yield this.app.client | ||
| .waitForDataFile('adblock') |
There was a problem hiding this comment.
we should really add a way to mock the adblock data so we don't have to wait for the download or worry about connection errors and changes in the adblock file contents
|
talked to brianj on slack. the underlying issue causing this PR to be fragile is that the tabId for websockets is -1 because [1] https://chromium.googlesource.com/chromium/src/+/4a593833d44a457f177f99b2c907bd0f6ae397f7 |
Fix #6997. Requires brave/muon#160 Auditors: @bridiver Test Plan: covered by automated test
145d84b to
f91f678
Compare
|
@bridiver i fixed the websockets tabid issue in brave/muon#160 so the test is no longer fragile |
|
muon changes are now in master |
|
++ |
Fix #6997.
Requires brave/muon#160
Auditors: @bridiver
Test Plan: covered by automated test
git rebase -ito squash commits (if needed).