-
Notifications
You must be signed in to change notification settings - Fork 893
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 some webui that doesn't allowed in private window is loaded in private window #1295
Conversation
c56a0bc
to
3db9f6a
Compare
#undef Navigate | ||
#undef IsURLAllowedInIncognito | ||
|
||
void Navigate(NavigateParams* params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is exactly what we don't want to do: try to find every place we need to hook in to map the url. We need to map it once and only once in a place that takes effect before any security checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test this place as a replacement of current mapping not for another hooking with current one. So I inserted NOTREACHED()
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I'll still studying navigation flow to find better place :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, just need to remember that there are many, many different paths and most of the are initiated by the renderer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments below
2e2d7fa
to
83b19b3
Compare
@bridiver With this new mapping timing, previously skipped url check/verification that did for chrome ui are also applied to brave ui for browser-initiated navigation. |
@@ -63,6 +63,12 @@ namespace { | |||
|
|||
bool HandleURLRewrite(GURL* url, | |||
content::BrowserContext* browser_context) { | |||
if (url->SchemeIs(kBraveUIScheme)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this should be a DCHECK
} | ||
|
||
+#if defined(BRAVE_CHROMIUM_BUILD) | ||
+ DCHECK(!url.SchemeIs(content::kBraveUIScheme)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely do not want to patch in a DCHECK here
@@ -13,6 +13,10 @@ | |||
bool FixupBrowserAboutURLBraveImpl(GURL* url, | |||
content::BrowserContext* browser_context) { | |||
if (url->SchemeIs(kBraveUIScheme)) { | |||
NOTREACHED() << "brave url should not be reached here. scheme mapping " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would we put a NOTREACHED() before a bunch of code? This does not seem correct to me
83b19b3
to
e9f0ccc
Compare
@bridiver Rebased onto the master and I think all your comments were resolved with this rebase. |
host != chrome::kChromeUISuggestionsHost; | ||
} | ||
|
||
+#if defined(BRAVE_CHROMIUM_BUILD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for our sync page.
Our sync page is not allowed to load private window.
That causes crash for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a good example of what I brought up in slack today. If we create this one-off patch then someone else will have to come in and create another one if there is some other host we don't want in private tabs. We should have a chromium_src override that defines a method to call here
e9f0ccc
to
d2a5854
Compare
857a3e0
to
3c89b51
Compare
Some brave pages should not be loaded in private window such as brave://settings. Instead, they should be redirected to normal window. Also sync/rewards host are added to not allowed list.
3c89b51
to
205f727
Compare
Some webui should not be loaded in private window.
Instead, they should be redirected to normal window.
This also fixes crash when user tries to load sync page in private window.
Sync page will be also redirected to normal window.
Fix brave/brave-browser#2853
Fix brave/brave-browser#2852
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests
) ongit rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
BraveSchemeLoadBrowserTest.*PageIsNotAllowedInPrivateWindow
Reviewer Checklist: