Don't reveal chrome-untrusted:// and other parents in ancestorOrigins. #20792
Conversation
goodov
force-pushed
the
fix-ancestors-origins-for-player
branch
from
4c97c40 to
2f16868
Compare
goodov
force-pushed
the
fix-ancestors-origins-for-player
branch
from
2f16868 to
224ecbb
Compare
goodov
added
the
CI/run-upstream-tests
goodov
force-pushed
the
fix-ancestors-origins-for-player
branch
from
224ecbb to
7493df7
Compare
goodov
added
CI/run-linux-arm64
goodov
force-pushed
the
fix-ancestors-origins-for-player
branch
from
7493df7 to
ab6f3fd
Compare
goodov
removed
the
CI/run-upstream-tests
| auto* raw_origins = ancestorOrigins_ChromiumImpl(); | ||
| for (uint32_t i = 0; i < raw_origins->length(); ++i) { | ||
| const String raw_origin = raw_origins->item(i); | ||
| const scoped_refptr<SecurityOrigin> origin = | ||
| SecurityOrigin::CreateFromString(raw_origin); | ||
| if (is_chrome_untrusted(origin.get())) { | ||
| break; |
There was a problem hiding this comment.
Sorry for missing that during the review, but shouldn't we use continue here instead?
I think we want to remove chrome-untrusted from the list, but if there are multiple nesting levels, then we should probably leave the other ones there.
There was a problem hiding this comment.
Sorry for missing that during the review, but shouldn't we use
continuehere instead? I think we want to removechrome-untrustedfrom the list, but if there are multiple nesting levels, then we should probably leave the other ones there.
we explicitly don't want the player iframe to know its nested level (how many iframes deep it's running).
I don't think we want to reveal this info to any iframe inside chrome-untrusted://.
There was a problem hiding this comment.
Just to be clear, I was not suggesting adding "null" entries in this case, just omitting the chrome-untrusted entries instead.
There was a problem hiding this comment.
Just to be clear, I was not suggesting adding
"null"entries in this case, just omitting thechrome-untrustedentries instead.
yep, but this way the ancestorOrigins will still contain the top frame, which is chrome://player. We also don't want this.
There was a problem hiding this comment.
And it would be fine to remove chrome:// URLs in a similar way IMO.
So basically I think we should do one of two things:
- Remove
chrome-untrustedandchromeuntrusted entries fromancestorOriginsin all cases, but leave the other entries alone. This would apply all of the time. - Clear
ancestorOriginsentirely, but only forchrome://player.
With #1, it would mean that if we have nested iframes like:
chrome://player => chrome-untrusted://whatever => https://example.com => https://targetsite.com
then targetsite.com would see the same ancestorOrigins as in the normal web case:
https://example.com => https://targetsite.com
#2 on the other hand would solve the immediate problem without affecting anything else.
There was a problem hiding this comment.
With #1, it would mean that if we have nested iframes like:
chrome://player => chrome-untrusted://whatever => https://example.com => https://targetsite.comthen targetsite.com would see the same ancestorOrigins as in the normal web case:
https://example.com => https://targetsite.com
This example should already work like you described. The chain will be left intact until chrome-untrusted:// is encountered, so https://example.com => https://targetsite.com chain will stay. The order in the chain list is reversed: first we get https://targetsite.com, then https://example.com, then chrome-untrusted://. We stop at chrome-untrusted:// item, this means the "good" https:// chain is there.
Resolves brave/brave-browser#34033
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: