Javascript Binding - Finalize JavascriptBindingApiAllowOrigins: Per-browser isolation and origin normalization#5218
Conversation
📝 WalkthroughWalkthroughReplaces a global JS-binding flag with per-browser settings, serializes allowed origins to browser extraInfo, and enforces origin checks during OnContextCreated to conditionally expose CefSharp/CefSharp JS binding objects. Tests and managed settings were added to configure and validate allowed origins. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant MBA as ManagedCefBrowserAdapter
participant BI as BrowserInit (native)
participant CAW as CefAppUnmanagedWrapper
participant BW as BrowserWrapper
participant Frame as CefFrame / JavaScript
App->>MBA: CreateBrowser(with JavascriptBindingSettings)
MBA->>BI: include extraInfo.JavascriptBindingApiAllowOrigins
BI->>CAW: OnBrowserCreated(extraInfo)
CAW->>BW: Set JavascriptBindingApiEnabled/HasAllowOrigins/AllowOrigins
Frame->>CAW: OnContextCreated(frame)
CAW->>BW: Read wrapper.JavascriptBindingApiEnabled
alt Enabled
CAW->>CAW: IsJavascriptBindingApiAllowed(frame)
CAW->>Frame: Parse frame.GetURL() origin & normalize
CAW->>BW: Compare origin against wrapper.JavascriptBindingApiAllowOrigins
alt Origin allowed or no origins configured
CAW->>Frame: Inject cefSharp / CefSharp binding objects
else Not allowed
CAW->>Frame: Skip binding injection
end
else Disabled
CAW->>Frame: Skip binding injection
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
luca-sonntag
changed the title
|
❌ Build CefSharp 136.1.40-CI5434 failed (commit c7297af541 by @) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 348-370: The code currently checks browserWrapper and
JavascriptBindingApiHasAllowOrigins but then dereferences
browserWrapper->JavascriptBindingApiAllowOrigins (used with GetSize/GetAt) which
can be null; add a local smart-pointer/local reference like allowOrigins =
browserWrapper->JavascriptBindingApiAllowOrigins right after the existing flag
check, guard it with if (!allowOrigins.get()) { return false; } (fail closed)
and then replace subsequent direct uses of
browserWrapper->JavascriptBindingApiAllowOrigins with allowOrigins when calling
GetSize/GetAt; keep the existing frameUrl/frameUrlOrigin logic intact.
- Around line 103-113: The JS binding API flags are only being set when the
wrapper is created for non-popup browsers, leaving popup wrappers with defaults
(Enabled=true, HasAllowOrigins=false) which makes
IsJavascriptBindingApiAllowed() return true for popups; move the block that
reads and assigns wrapper->JavascriptBindingApiEnabled,
wrapper->JavascriptBindingApiHasAllowOrigins and the
wrapper->JavascriptBindingApiAllowOrigins population (the
extraInfo->GetBool/GetList logic) out of the if (!browser->IsPopup()) scope so
these settings are always applied to every CefAppUnmanagedWrapper instance,
ensuring IsJavascriptBindingApiAllowed() enforces configured restrictions for
popups as well.
In `@CefSharp/JavascriptBinding/JavascriptBindingSettings.cs`:
- Around line 52-57: The normalization currently calls origin.EndsWith(...)
inside the setter and will throw if any element of the incoming string[] value
is null; update the mapping used to populate javascriptBindingApiAllowOrigins so
it guards against null entries (e.g., map null to null or to an empty string) by
checking origin == null before calling EndsWith, and otherwise apply the
existing trimming logic; modify the conversion that uses Array.ConvertAll (the
lambda that references origin) to perform that null check to avoid
NullReferenceException.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.hCefSharp.BrowserSubprocess.Core/CefBrowserWrapper.hCefSharp/JavascriptBinding/JavascriptBindingSettings.cs
|
❌ Build CefSharp 136.1.40-CI5435 failed (commit e96399376a by @) |
|
❌ Build CefSharp 136.1.40-CI5436 failed (commit 3067020ea6 by @) |
luca-sonntag
force-pushed
the
feature/jsbindingalloworiginlist-completion
branch
from
25d6090 to
6cd5871
Compare
|
✅ Build CefSharp 136.1.40-CI5437 completed (commit a0289d8427 by @) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
351-401: Well-structured origin validation with fail-closed security defaults.The implementation correctly:
- Falls back to permissive behavior when no allow-origins are configured (backward compatibility)
- Fails closed when allow-origins are expected but the list is null (lines 360-364)
- Normalizes origins by removing trailing slashes before comparison
- Uses case-insensitive comparison for origin matching
- Fails closed when URL parsing fails or no match is found
One optional defensive improvement: consider adding a null check for
originStrat line 375 before dereferencing, in case CEF returns inconsistent length/pointer data:🛡️ Optional defensive fix
auto originStr = frameUrlParts.origin.str; auto originLen = frameUrlParts.origin.length; - if (originLen > 0 && originStr[originLen - 1] == L'/') + if (originStr != nullptr && originLen > 0 && originStr[originLen - 1] == L'/') { originLen--; } + + if (originStr == nullptr) + { + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp` around lines 351 - 401, The method IsJavascriptBindingApiAllowed should defensively verify that frameUrlParts.origin.str is not null before dereferencing it; update the logic in IsJavascriptBindingApiAllowed (around the use of frameUrlParts.origin.str and originLen) to check that originStr != nullptr prior to accessing originStr[originLen - 1] or constructing CefString, and if originStr is null treat it as a parse failure (return false) so we maintain the fail-closed behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 351-401: The method IsJavascriptBindingApiAllowed should
defensively verify that frameUrlParts.origin.str is not null before
dereferencing it; update the logic in IsJavascriptBindingApiAllowed (around the
use of frameUrlParts.origin.str and originLen) to check that originStr !=
nullptr prior to accessing originStr[originLen - 1] or constructing CefString,
and if originStr is null treat it as a parse failure (return false) so we
maintain the fail-closed behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/Stdafx.hCefSharp.Core.Runtime/Stdafx.hCefSharp/JavascriptBinding/JavascriptBindingSettings.cs
|
@amaitland I’ve reviewed your comments on PR #5085 and implemented the remaining changes in this PR. I wasn’t sure what the usual process is for finalizing open PRs, so I targeted your branch for these updates. There were problems with the build. The AppVeyor 'Visual Studio 2019' image update introduced Windows SDK 10.0.26100.0, which contains a bug in winnt.h where _CountOneBits64 is called without being defined for the ARM64 v142 toolchain. This caused the build to fail. I’ve added a temporary workaround in commit “Fix ARM64 build error C3861 in winnt.h (Windows SDK 10.0.26100.0)”. Since the master branch already uses Visual Studio 2022 by default on the build machine, this commit should no longer be necessary there and can be removed once everything is fully on VS 2022. |
|
@luca-sonntag Thanks for the PR. As my branch is out of date, can you target directly against master and rebase so it's up to date please. My two commits can probably be squashed into a single commit. |
Once you've rebased on master then please revert the unnecessary changes. Thanks |
|
Additional tests will also need to be written. #5085 (comment) |
luca-sonntag
force-pushed
the
feature/jsbindingalloworiginlist-completion
branch
from
6cd5871 to
b916db1
Compare
luca-sonntag
changed the base branch from
feature/jsbindingalloworiginlist
to
master
Move the JS binding API configuration block out of the !browser->IsPopup() scope in OnBrowserCreated. This ensures that IsJavascriptBindingApiAllowed() enforces configured restrictions for popups as well.
In IsJavascriptBindingApiAllowed, added a null check for JavascriptBindingApiAllowOrigins to prevent a potential null dereference if JavascriptBindingApiHasAllowOrigins is true but the allowOrigins list itself was null.
luca-sonntag
force-pushed
the
feature/jsbindingalloworiginlist-completion
branch
from
b916db1 to
6cefcea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
67-70:⚠️ Potential issue | 🔴 CriticalPopups bypass JavaScript binding origin restrictions.
At line 67,
OnBrowserCreatedreturns early for popups (nullextraInfo), leavingJavascriptBindingApiEnabled=trueandJavascriptBindingApiHasAllowOrigins=false. This causesIsJavascriptBindingApiAllowed()to returntrueregardless of origin, allowing unrestricted JavaScript binding API access in popup windows. The code acknowledges "Popups don't send this information" (line 72-73) but provides no rationale or enforcement of parent browser's binding restrictions.Either populate popup
extraInfowith inherited binding keys from the parent browser (lines 85-100 in ManagedCefBrowserAdapter show the required keys), or implement and document explicit popup-specific binding policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp` around lines 67 - 70, OnBrowserCreated currently returns early when extraInfo is null, leaving JavascriptBindingApiEnabled true and JavascriptBindingApiHasAllowOrigins false which lets popups bypass origin checks; update OnBrowserCreated (and related popup handling) to either inherit the parent's binding metadata into the popup's extraInfo (copy the same binding keys that ManagedCefBrowserAdapter expects so popups get the parent's allowed origins/keys) or enforce an explicit popup-specific policy by setting JavascriptBindingApiEnabled=false or populating JavascriptBindingApiHasAllowOrigins and allowed-origins before returning; ensure IsJavascriptBindingApiAllowed consults the inherited/populated values so popup windows are subject to the same origin restrictions as their parent.
🧹 Nitpick comments (1)
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (1)
133-242: Consider adding popup-specific allow-origin tests to lock this security path.The new tests cover main-frame origin behavior well, but popup enforcement is also part of this change set. A
window.open(...)scenario would reduce regression risk.If helpful, I can draft a focused popup test case pattern for this suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs` around lines 133 - 242, Add popup-specific tests mirroring the existing origin tests (e.g. ShouldDisableJsBindingApiForOrigin, ShouldEnableJsBindingApiForOrigin, ShouldEnableJsBindingApiWhenOriginsListIsEmpty) that open a popup via window.open(...) from the initial page and then evaluate the same checks inside the popup; specifically, use ChromiumWebBrowser with automaticallyCreateBrowser:false, call browser.CreateBrowser(), WaitForInitialLoadAsync(), then EvaluateScriptAsync("window.open('...')") to open the popup and obtain a reference/URL for the new window, wait for that popup to finish loading, and assert typeof window.cefSharp / typeof window.CefSharp in the popup matches the expected allowed-or-not result for the configured JavascriptBindingApiAllowOrigins; place these new tests in JavascriptBindingTests.cs alongside the existing Theory/Fact tests and reuse the same setup (JavascriptObjectRepository.Settings, JavascriptBindingApiEnabled, JavascriptBindingApiAllowOrigins) to ensure popup origin enforcement is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs`:
- Around line 175-176: The test awaits browser.WaitForInitialLoadAsync() but
doesn't verify the result; after each call that assigns the result to
loadResponse (the awaited value from WaitForInitialLoadAsync in
JavascriptBindingTests), add Assert.True(loadResponse.Success) to assert the
initial load succeeded (repeat for the occurrences that currently await but lack
an assertion).
---
Outside diff comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 67-70: OnBrowserCreated currently returns early when extraInfo is
null, leaving JavascriptBindingApiEnabled true and
JavascriptBindingApiHasAllowOrigins false which lets popups bypass origin
checks; update OnBrowserCreated (and related popup handling) to either inherit
the parent's binding metadata into the popup's extraInfo (copy the same binding
keys that ManagedCefBrowserAdapter expects so popups get the parent's allowed
origins/keys) or enforce an explicit popup-specific policy by setting
JavascriptBindingApiEnabled=false or populating
JavascriptBindingApiHasAllowOrigins and allowed-origins before returning; ensure
IsJavascriptBindingApiAllowed consults the inherited/populated values so popup
windows are subject to the same origin restrictions as their parent.
---
Nitpick comments:
In `@CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs`:
- Around line 133-242: Add popup-specific tests mirroring the existing origin
tests (e.g. ShouldDisableJsBindingApiForOrigin,
ShouldEnableJsBindingApiForOrigin,
ShouldEnableJsBindingApiWhenOriginsListIsEmpty) that open a popup via
window.open(...) from the initial page and then evaluate the same checks inside
the popup; specifically, use ChromiumWebBrowser with
automaticallyCreateBrowser:false, call browser.CreateBrowser(),
WaitForInitialLoadAsync(), then EvaluateScriptAsync("window.open('...')") to
open the popup and obtain a reference/URL for the new window, wait for that
popup to finish loading, and assert typeof window.cefSharp / typeof
window.CefSharp in the popup matches the expected allowed-or-not result for the
configured JavascriptBindingApiAllowOrigins; place these new tests in
JavascriptBindingTests.cs alongside the existing Theory/Fact tests and reuse the
same setup (JavascriptObjectRepository.Settings, JavascriptBindingApiEnabled,
JavascriptBindingApiAllowOrigins) to ensure popup origin enforcement is covered.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.hCefSharp.BrowserSubprocess.Core/CefBrowserWrapper.hCefSharp.Core.Runtime/ManagedCefBrowserAdapter.cppCefSharp.Test/JavascriptBinding/JavascriptBindingTests.csCefSharp/JavascriptBinding/JavascriptBindingSettings.cs
|
✅ Build CefSharp 145.0.260-CI5438 completed (commit ab5e7166dd by @) |
luca-sonntag
force-pushed
the
feature/jsbindingalloworiginlist-completion
branch
from
6cefcea to
b54d60f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (1)
218-248: Consider adding case-insensitive origin matching test coverage.The test correctly validates that binding is enabled when a matching origin exists at various positions in the allow list. However, the PR implements case-insensitive comparison using
_wcsicmp, but there's no test validating this behavior (e.g., matching"HTTPS://CEFSHARP.EXAMPLE"against a lowercase origin).Also, minor style nit: missing blank line between line 217 and the
[Theory]attribute at line 218 for consistency with other test methods.🧪 Suggested additional test case for case-insensitive matching
[Fact] public async Task ShouldEnableJsBindingApiForOriginCaseInsensitive() { using (var browser = new ChromiumWebBrowser(CefExample.BindingApiCustomObjectNameTestUrl, automaticallyCreateBrowser: false)) { var settings = browser.JavascriptObjectRepository.Settings; settings.JavascriptBindingApiEnabled = true; // Use different casing than the actual origin settings.JavascriptBindingApiAllowOrigins = new string[] { "HTTPS://CEFSHARP.EXAMPLE" }; browser.CreateBrowser(); var loadResponse = await browser.WaitForInitialLoadAsync(); Assert.True(loadResponse.Success); var response = await browser.EvaluateScriptAsync("typeof window.cefSharp === 'undefined'"); Assert.True(response.Success); Assert.False((bool)response.Result); // Should be enabled due to case-insensitive match } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs` around lines 218 - 248, Add a new test that verifies case-insensitive origin matching: create a test (e.g., ShouldEnableJsBindingApiForOriginCaseInsensitive) that mirrors ShouldEnableJsBindingApiForOrigin but sets settings.JavascriptBindingApiAllowOrigins to a differently-cased origin (for example "HTTPS://CEFSHARP.EXAMPLE"), enables settings.JavascriptBindingApiEnabled, calls browser.CreateBrowser(), waits for load with WaitForInitialLoadAsync, and asserts the CefSharp binding exists via EvaluateScriptAsync; this will validate the _wcsicmp-based case-insensitive comparison. Also add the missing blank line before the existing [Theory] attribute to match style/consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs`:
- Around line 218-248: Add a new test that verifies case-insensitive origin
matching: create a test (e.g., ShouldEnableJsBindingApiForOriginCaseInsensitive)
that mirrors ShouldEnableJsBindingApiForOrigin but sets
settings.JavascriptBindingApiAllowOrigins to a differently-cased origin (for
example "HTTPS://CEFSHARP.EXAMPLE"), enables
settings.JavascriptBindingApiEnabled, calls browser.CreateBrowser(), waits for
load with WaitForInitialLoadAsync, and asserts the CefSharp binding exists via
EvaluateScriptAsync; this will validate the _wcsicmp-based case-insensitive
comparison. Also add the missing blank line before the existing [Theory]
attribute to match style/consistency.
3 participants
Fixes:
#5001
Summary:
This PR completes and optimizes the implementation of the
JavascriptBindingApiAllowOriginsfeature. The primary focus is shifting binding settings from a global state to a per-browser scope for better isolation and introducing robust origin normalization to ensure consistent security validation across different URL formats.Changes:
JavascriptBindingApiEnabled,JavascriptBindingApiAllowOrigins, etc.) from the globalCefAppUnmanagedWrappertoCefBrowserWrapper. This allows each browser instance to have its own independent list of allowed origins.JavascriptBindingSettings.csto automatically trim trailing slashes from strings added to theJavascriptBindingApiAllowOriginslist, preventing common configuration errors.CefAppUnmanagedWrapper::IsJavascriptBindingApiAllowed._wcsicmp) to align with standard origin matching behavior.How Has This Been Tested?
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.csJavascriptBindingApiAllowOriginscorrectly blocks or allows the cefSharp object based on the frame's URL.CefBrowserWrapper).Screenshots (if appropriate): N/A
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests