Same site query filter #6609
Same site query filter #6609
Conversation
fmarier
force-pushed
the
same-site-query-filter-9020
branch
2 times, most recently
from
df6c3a3
to
709d20b
Compare
fmarier
force-pushed
the
same-site-query-filter-9020
branch
2 times, most recently
from
7ca8158
to
e7ed618
Compare
fmarier
force-pushed
the
same-site-query-filter-9020
branch
from
e7ed618
to
0a6800d
Compare
| } | ||
|
|
||
| if (ctx->redirect_source.is_valid()) { | ||
| if (ctx->internal_redirect) { |
There was a problem hiding this comment.
isn't ctx->internal_redirect always false if we have good redirect_source?
There was a problem hiding this comment.
No, because we generate 307s as a result of the query filter for example. If we don't filter those out here, we end up looking at these requests twice.
There was a problem hiding this comment.
I mean that redirect_source is set to a non-empty value only if ctx->internal_redirect is false, and it seems that ctx->internal_redirect cannot change to true afterwards without changing ctx->redirect_source. This way, if ctx->redirect_source is non-empty, ctx->internal_redirect should be false
Am I wrong?
There was a problem hiding this comment.
I believe you're right, as long as we keep these in sync.
| <body> | ||
| <p>Waiting for JS redirect...</p> | ||
| <script> | ||
| setTimeout(redirectToDestination, 100); |
There was a problem hiding this comment.
curious why do we need this?
There was a problem hiding this comment.
That's to simulate a navigation, as opposed to a redirect. Another way I could have done this would have been to include a link and then inject JS to click the link programmatically.
There was a problem hiding this comment.
100ms is pretty noticeable dealy even for browsertests, so I'd suggest to change if we can avoid it
browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
Outdated
Show resolved
Hide resolved
browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
Outdated
Show resolved
Hide resolved
browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc
Outdated
Show resolved
Hide resolved
browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc
Outdated
Show resolved
Hide resolved
browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc
Outdated
Show resolved
Hide resolved
|
LGTM with small nits. I'll rebase my #6674 on top of this one after it gets merged |
fmarier
force-pushed
the
same-site-query-filter-9020
branch
from
0a6800d
to
7ebccaf
Compare
fmarier
force-pushed
the
same-site-query-filter-9020
branch
from
7ebccaf
to
cc3e7cf
Compare
| // Same-site redirects are exempted. | ||
| return; | ||
| } | ||
| } else if (net::registry_controlled_domains::SameDomainOrHost( |
There was a problem hiding this comment.
maybe we also can shorten the code by chosing something like source_url conditionally?
if (...) {source_url = ctx->redirect_source;}
else if (...) {source_url = ctx->initiator_url;}
if (net::registry_controlled_domains::SameDomainOrHost(source_url, ctx->request_url, ...) {
return;
}
|
I updated the documentation on the wiki: https://github.com/brave/brave-browser/wiki/Query-String-Filter/_compare/ac55a85c8e44c91e16bdbe5fb2a82ceab1af271c...9ea0443624df53f34b0265568ee2c82f8c4e6acf |
|
For some reason, GitHub didn't show me your comments and only showed me your approval (I found out catching up on emails). Sorry for merging without addressing these first. |
Resolves brave/brave-browser#9020
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
All of the test cases on https://fmarier.github.io/brave-testing/query-filter.html should work as described on there.
Reviewer Checklist:
After-merge Checklist:
changes has landed on.