Replace DidCommit's->origin with NavigationRequest::GetOriginToCommit() [chromium/src : main]
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
Related details
- Alex Moshchuk
- Charlie Reis
- Chromium IPC Reviews
- Daniel Cheng
- Dave Risney
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
dch...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Moshchuk
- Charlie Reis
- Daniel Cheng
- Dave Risney
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
Similarly adding Liang to get the initial review!
Related details
- Dave Risney
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
DCHECK(commit_params->origin_to_commit);Should this be CHECK instead of DCHECK?
// TODO(crbug.com/457478023): Disable the test due to navigation origin changes.
// Use this crbug to fix the test accordingly and enable it.Do we still need this test? It seems that the test is trying to set unexpected origin in DidCommit IPC call, with us removing the parameter, should we remove the test?
navigation_request->SetOriginToCommitForTesting(new_origin_);Which tests are still relying on origin getting changed after DidCommitNavigation? If there should be no test relying on this, could we simply remove the support of setting the "committed origin"? If that is what we should do, we could potentially simply ignore new_origin_ in this CL and create separate CL to remove the new_origin_ support from this class.
if (commit_params.origin_to_commit) {Could you add comment on when commit_params.origin_to_commit does not have value and what's the impact of leaving navigation_params->origin_to_commit unset.
Related details
- Dave Risney
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 4 comments![]( "Open in Gerrit")
Should this be CHECK instead of DCHECK?
Done
// TODO(crbug.com/457478023): Disable the test due to navigation origin changes.
// Use this crbug to fix the test accordingly and enable it.Do we still need this test? It seems that the test is trying to set unexpected origin in DidCommit IPC call, with us removing the parameter, should we remove the test?
SecurityExploitBrowserTest.CrossOriginSameDocumentCommitFromAboutBlank - agree, removed the test as renderer no longer supplies the committed origin
navigation_request->SetOriginToCommitForTesting(new_origin_);Which tests are still relying on origin getting changed after DidCommitNavigation? If there should be no test relying on this, could we simply remove the support of setting the "committed origin"? If that is what we should do, we could potentially simply ignore new_origin_ in this CL and create separate CL to remove the new_origin_ support from this class.
SitePerProcessBrowserTest.CommittedOriginIncompatibleWithOriginLock - can be removed as well but additionally it sanity-checks ProcessLock equality for same-origin URLs and inequality for cross-origin. But this isn't the purpose for the test and covered by other tests.
ChromeSecurityExploitBrowserTest.CommitExtensionOriginInWebProces - can be removed as well. This test ensures non-extension process cannot send a about:blank commit with an extension origin.
Do you agree? If so, I can remove the tests in the current cl and later remove new_origin_?
if (commit_params.origin_to_commit) {Could you add comment on when commit_params.origin_to_commit does not have value and what's the impact of leaving navigation_params->origin_to_commit unset.
This is needed for some tests(RenderFrameImplTest/RenderViewImplTest) that creates DummyCommitNavigationParams using blink::CreateCommitNavigationParams().
Also, this check exists before and removed when we changed origin_to_commit was always set but in the current cl I made it optional so added the check back
Related details
- Dave Risney
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 2 comments![]( "Open in Gerrit")
(*params)->origin = url::Origin::Create(GURL("https://example.com/"));lzhao@ Added the feature flag to handle any regressions. Should we also keep this in test files behind the feature flag?
// TODO(https://crbug.com/402272788): Remove `origin` from DidCommitParams,
// making this check unnecessary.
if (navigation_request &&
navigation_request->commit_params().origin_to_commit != params->origin) {
bad_message::ReceivedBadMessage(process,
bad_message::RFH_ORIGIN_TO_COMMIT_MISMATCH);
return false;
}lzhao@ Should we also keep this behind a feature flag?
Related details
- Dave Risney
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 5 comments![]( "Open in Gerrit")
handle edge cases where GetOriginToCommit() cannot be safely called sometimes. As GetOriginToCommit() relies on SandboxFlagsToCommit(),line too long
(*params)->origin = url::Origin::Create(GURL("https://example.com/"));lzhao@ Added the feature flag to handle any regressions. Should we also keep this in test files behind the feature flag?
The normal way is to update the tests to be parameterized tests and tests the behavior for both when the feature is enabled and disabled.
Since the old code is still used when the feature is disabled, we actually should not delete any tests at this moment. We should update them to verify behavior when the feature is enabled and disabled.
// TODO(https://crbug.com/402272788): Remove `origin` from DidCommitParams,
// making this check unnecessary.
if (navigation_request &&
navigation_request->commit_params().origin_to_commit != params->origin) {
bad_message::ReceivedBadMessage(process,
bad_message::RFH_ORIGIN_TO_COMMIT_MISMATCH);
return false;
}lzhao@ Should we also keep this behind a feature flag?
Yes. Since we still have origin_deprecated and that would be used when the feature is disabled. Or can we use NavigationRequest::GetOriginToCommit for the check so that it works for both feature disabled and enabled?
if (everything_except_origin_matches &&
(!ShouldVerify("origin") ||
request->state() < NavigationRequest::WILL_PROCESS_RESPONSE ||
request->GetOriginToCommit() == params.origin)) {
// Don't do a DumpWithoutCrashing if everything matches. Note that we save
// `everything_except_origin_matches` separately so that we can skip doing
// a DumpWithoutCrashing at the end of this function if the origin turns
// out to match (actual origin match checking is done below as it does its
// own DumpWithoutCrashing).
return;We might also want to keep this when the feature is disabled.
navigation_request->SetOriginToCommitForTesting(new_origin_);Monica ChintalaWhich tests are still relying on origin getting changed after DidCommitNavigation? If there should be no test relying on this, could we simply remove the support of setting the "committed origin"? If that is what we should do, we could potentially simply ignore new_origin_ in this CL and create separate CL to remove the new_origin_ support from this class.
SitePerProcessBrowserTest.CommittedOriginIncompatibleWithOriginLock - can be removed as well but additionally it sanity-checks ProcessLock equality for same-origin URLs and inequality for cross-origin. But this isn't the purpose for the test and covered by other tests.
ChromeSecurityExploitBrowserTest.CommitExtensionOriginInWebProces - can be removed as well. This test ensures non-extension process cannot send a about:blank commit with an extension origin.
Do you agree? If so, I can remove the tests in the current cl and later remove new_origin_?
Looks like we now have both old code and new code, so we would need to keep all test code and ensure that the test works properly when the feature is disabled or enabled. We could skip some parts of the tests if some checks are not applicable anymore.
Related details
- Dave Risney
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
if (commit_params.origin_to_commit) {Monica ChintalaCould you add comment on when commit_params.origin_to_commit does not have value and what's the impact of leaving navigation_params->origin_to_commit unset.
This is needed for some tests(RenderFrameImplTest/RenderViewImplTest) that creates DummyCommitNavigationParams using blink::CreateCommitNavigationParams().
Also, this check exists before and removed when we changed origin_to_commit was always set but in the current cl I made it optional so added the check back
Would it be possible to just set the origin_to_commit value on those tests, or are there other cases that won't set this?
Related details
- Dave Risney
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 5 comments![]( "Open in Gerrit")
handle edge cases where GetOriginToCommit() cannot be safely called sometimes. As GetOriginToCommit() relies on SandboxFlagsToCommit(),Monica Chintalaline too long
Done
(*params)->origin = url::Origin::Create(GURL("https://example.com/"));Liang Zhaolzhao@ Added the feature flag to handle any regressions. Should we also keep this in test files behind the feature flag?
The normal way is to update the tests to be parameterized tests and tests the behavior for both when the feature is enabled and disabled.
Since the old code is still used when the feature is disabled, we actually should not delete any tests at this moment. We should update them to verify behavior when the feature is enabled and disabled.
All related browser tests are updated with parameterized tests to run for feature when enabled/disabled. For unit_tests, assignment of origin_deprecated is behind a feature flag.
// TODO(https://crbug.com/402272788): Remove `origin` from DidCommitParams,
// making this check unnecessary.
if (navigation_request &&
navigation_request->commit_params().origin_to_commit != params->origin) {
bad_message::ReceivedBadMessage(process,
bad_message::RFH_ORIGIN_TO_COMMIT_MISMATCH);
return false;
}Liang Zhaolzhao@ Should we also keep this behind a feature flag?
Yes. Since we still have origin_deprecated and that would be used when the feature is disabled. Or can we use NavigationRequest::GetOriginToCommit for the check so that it works for both feature disabled and enabled?
using NavigationRequest::GetOriginToCommit for this check and will delete later when feature flag is removed
if (everything_except_origin_matches &&
(!ShouldVerify("origin") ||
request->state() < NavigationRequest::WILL_PROCESS_RESPONSE ||
request->GetOriginToCommit() == params.origin)) {
// Don't do a DumpWithoutCrashing if everything matches. Note that we save
// `everything_except_origin_matches` separately so that we can skip doing
// a DumpWithoutCrashing at the end of this function if the origin turns
// out to match (actual origin match checking is done below as it does its
// own DumpWithoutCrashing).
return;We might also want to keep this when the feature is disabled.
Done
navigation_request->SetOriginToCommitForTesting(new_origin_);Monica ChintalaWhich tests are still relying on origin getting changed after DidCommitNavigation? If there should be no test relying on this, could we simply remove the support of setting the "committed origin"? If that is what we should do, we could potentially simply ignore new_origin_ in this CL and create separate CL to remove the new_origin_ support from this class.
Liang ZhaoSitePerProcessBrowserTest.CommittedOriginIncompatibleWithOriginLock - can be removed as well but additionally it sanity-checks ProcessLock equality for same-origin URLs and inequality for cross-origin. But this isn't the purpose for the test and covered by other tests.
ChromeSecurityExploitBrowserTest.CommitExtensionOriginInWebProces - can be removed as well. This test ensures non-extension process cannot send a about:blank commit with an extension origin.
Do you agree? If so, I can remove the tests in the current cl and later remove new_origin_?
Looks like we now have both old code and new code, so we would need to keep all test code and ensure that the test works properly when the feature is disabled or enabled. We could skip some parts of the tests if some checks are not applicable anymore.
- Dave Risney
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Monica Chintala
| Commit-Queue | +1 |
1 comment
if (commit_params.origin_to_commit) {Monica ChintalaCould you add comment on when commit_params.origin_to_commit does not have value and what's the impact of leaving navigation_params->origin_to_commit unset.
Rakina Zata AmniThis is needed for some tests(RenderFrameImplTest/RenderViewImplTest) that creates DummyCommitNavigationParams using blink::CreateCommitNavigationParams().
Also, this check exists before and removed when we changed origin_to_commit was always set but in the current cl I made it optional so added the check back
Would it be possible to just set the origin_to_commit value on those tests, or are there other cases that won't set this?
Yes, tests mostly in RenderFrameImplTest/RenderViewImplTest if using blink::CreateCommitNavigationParams() fails when origin_to_commit isn't set
Also Fixed PasswordAutofillAgentTest.SendPasswordFormsTest_ReloadTab and
TranslateAgentBrowserTest.BackToTranslatablePage by passing origin_to_commit in RenderViewTest::GoToOffset and RenderViewTest::Reload
Related details
- Dave Risney
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |