Javascript Binding - Finalize JavascriptBindingApiAllowOrigins: Per-browser isolation and origin normalization#5218
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes consolidate JavaScript binding API configuration from CefAppUnmanagedWrapper to CefBrowserWrapper, and implement origin-based access validation for JavaScript bound objects. The system now checks if a frame's origin is in the allowed list before enabling bindings, with proper origin normalization across C# and C++ layers. Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript<br/>Context Creation
participant CefApp as CefAppUnmanagedWrapper<br/>(OnContextCreated)
participant BrowserWrap as CefBrowserWrapper
participant Validator as IsJavascriptBinding<br/>ApiAllowed
participant BindingObj as Binding Object<br/>Creation
JS->>CefApp: OnContextCreated(frame)
CefApp->>BrowserWrap: Retrieve browser wrapper
CefApp->>BrowserWrap: Check JavascriptBindingApiEnabled
alt API Enabled
CefApp->>Validator: IsJavascriptBindingApiAllowed(frame)
Validator->>BrowserWrap: Get JavascriptBindingApiHasAllowOrigins
alt Has Allow Origins Configured
Validator->>Validator: Parse & normalize frame origin
Validator->>BrowserWrap: Get JavascriptBindingApiAllowOrigins
Validator->>Validator: Compare origin (case-insensitive)
alt Origin Matches
Validator-->>CefApp: true
else Origin Not Allowed
Validator-->>CefApp: false
end
else No Origins Configured
Validator-->>CefApp: true (allow all)
end
alt Origin Allowed
CefApp->>BindingObj: Create & assign to global scope
BindingObj-->>JS: Binding available
else Origin Not Allowed
CefApp-->>JS: No binding
end
else API Disabled
CefApp-->>JS: Skip binding
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
❌ Build CefSharp 136.1.40-CI5434 failed (commit c7297af541 by @) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
| wrapper->JavascriptBindingApiEnabled = extraInfo->GetBool("JavascriptBindingApiEnabled"); | ||
| wrapper->JavascriptBindingApiHasAllowOrigins = extraInfo->GetBool("JavascriptBindingApiHasAllowOrigins"); | ||
|
|
||
| if (_jsBindingApiHasAllowOrigins) | ||
| if (wrapper->JavascriptBindingApiHasAllowOrigins) | ||
| { | ||
| auto allowOrigins = extraInfo->GetList("JavascriptBindingApiAllowOrigins"); | ||
| if (allowOrigins.get() && allowOrigins->IsValid()) | ||
| { | ||
| _jsBindingApiAllowOrigins = allowOrigins->Copy(); | ||
| wrapper->JavascriptBindingApiAllowOrigins = allowOrigins->Copy(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "CefAppUnmanagedWrapper.cpp" -type fRepository: cefsharp/CefSharp
Length of output: 121
🏁 Script executed:
# Search for the file and check its size
git ls-files | grep -i "CefAppUnmanagedWrapper"Repository: cefsharp/CefSharp
Length of output: 176
🏁 Script executed:
# Search for the code section and broader context
rg -n "JavascriptBindingApiEnabled" --type=cpp --type=h -B 10 -A 15Repository: cefsharp/CefSharp
Length of output: 13992
🏁 Script executed:
# Look for popup-related creation and IsPopup checks
rg -n "IsPopup\|OnBeforePopup" --type=cpp --type=h -B 3 -A 5Repository: cefsharp/CefSharp
Length of output: 43
🏁 Script executed:
# Find CefBrowserWrapper class definition and defaults
rg -n "class CefBrowserWrapper|JavascriptBindingApiEnabled\s*=" --type=cpp --type=h -B 2 -A 2Repository: cefsharp/CefSharp
Length of output: 2067
🏁 Script executed:
# Read larger context around lines 103-113 in CefAppUnmanagedWrapper.cpp
sed -n '75,130p' ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppRepository: cefsharp/CefSharp
Length of output: 3062
🏁 Script executed:
# Find the OnBrowserCreated function to see full context
rg -n "OnBrowserCreated" --type=cpp -B 5 -A 40 | head -100Repository: cefsharp/CefSharp
Length of output: 10877
🏁 Script executed:
# Look for the if (!browser->IsPopup()) block
rg -n "IsPopup" ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp -B 5 -A 5Repository: cefsharp/CefSharp
Length of output: 519
🏁 Script executed:
# Check IsJavascriptBindingApiAllowed implementation
rg -n "IsJavascriptBindingApiAllowed" --type=cpp -A 20Repository: cefsharp/CefSharp
Length of output: 7643
🏁 Script executed:
# Check if popups receive extraInfo with JS binding keys from parent process
rg -n "OnBeforePopup" --type=cpp -B 5 -A 20 | head -80Repository: cefsharp/CefSharp
Length of output: 8706
🏁 Script executed:
# Check CefRenderProcessHandler to understand OnBrowserCreated contract
rg -n "CefRenderProcessHandler\|OnBrowserCreated" --type=cpp --type=h | head -20Repository: cefsharp/CefSharp
Length of output: 43
🏁 Script executed:
# Verify if popups get extraInfo passed in OnBrowserCreated
rg -n "extraInfo" ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp -B 2 -A 2Repository: cefsharp/CefSharp
Length of output: 1941
Popup browsers can bypass origin restrictions with current assignment scope.
Lines 103–113 set JavascriptBindingApi* properties only inside the if (!browser->IsPopup()) block. Popup wrappers retain constructor defaults (Enabled=true, HasAllowOrigins=false). In IsJavascriptBindingApiAllowed() (line 348–351), when HasAllowOrigins=false, the function returns true unconditionally, allowing popup contexts to bypass origin restrictions regardless of parent settings.
Move the JS binding API settings outside the popup check to apply configured restrictions to all browser wrappers:
Suggested fix
if (!browser->IsPopup())
{
_legacyBindingEnabled = extraInfo->GetBool("LegacyBindingEnabled");
if (_legacyBindingEnabled)
{
auto objects = extraInfo->GetList("LegacyBindingObjects");
if (objects.get() && objects->IsValid())
{
auto javascriptObjects = DeserializeJsObjects(objects, 0);
for each (JavascriptObject ^ obj in Enumerable::OfType<JavascriptObject^>(javascriptObjects))
{
if (_javascriptObjects->ContainsKey(obj->JavascriptName))
{
_javascriptObjects->Remove(obj->JavascriptName);
}
_javascriptObjects->Add(obj->JavascriptName, obj);
}
}
}
+ wrapper->JavascriptBindingApiEnabled = extraInfo->GetBool("JavascriptBindingApiEnabled");
+ wrapper->JavascriptBindingApiHasAllowOrigins = extraInfo->GetBool("JavascriptBindingApiHasAllowOrigins");
+
+ if (wrapper->JavascriptBindingApiHasAllowOrigins)
+ {
+ auto allowOrigins = extraInfo->GetList("JavascriptBindingApiAllowOrigins");
+ if (allowOrigins.get() && allowOrigins->IsValid())
+ {
+ wrapper->JavascriptBindingApiAllowOrigins = allowOrigins->Copy();
+ }
+ }
+
+ if (!browser->IsPopup())
+ {
if (extraInfo->HasKey("JsBindingPropertyName") || extraInfo->HasKey("JsBindingPropertyNameCamelCase"))
{
_jsBindingPropertyName = extraInfo->GetString("JsBindingPropertyName");
_jsBindingPropertyNameCamelCase = extraInfo->GetString("JsBindingPropertyNameCamelCase");
}
}
-
- wrapper->JavascriptBindingApiEnabled = extraInfo->GetBool("JavascriptBindingApiEnabled");
- wrapper->JavascriptBindingApiHasAllowOrigins = extraInfo->GetBool("JavascriptBindingApiHasAllowOrigins");
-
- if (wrapper->JavascriptBindingApiHasAllowOrigins)
- {
- auto allowOrigins = extraInfo->GetList("JavascriptBindingApiAllowOrigins");
- if (allowOrigins.get() && allowOrigins->IsValid())
- {
- wrapper->JavascriptBindingApiAllowOrigins = allowOrigins->Copy();
- }
- }🤖 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 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.
| if (browserWrapper == nullptr || !browserWrapper->JavascriptBindingApiHasAllowOrigins) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| auto frameUrl = frame->GetURL(); | ||
|
|
||
| CefURLParts frameUrlParts; | ||
|
|
||
| if (CefParseURL(frameUrl, frameUrlParts)) | ||
| { | ||
| auto originStr = frameUrlParts.origin.str; | ||
| auto originLen = frameUrlParts.origin.length; | ||
|
|
||
| if (originLen > 0 && originStr[originLen - 1] == L'/') | ||
| { | ||
| originLen--; | ||
| } | ||
|
|
||
| auto frameUrlOrigin = CefString(originStr, originLen); | ||
|
|
||
| auto size = static_cast<int>(browserWrapper->JavascriptBindingApiAllowOrigins->GetSize()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CefAppUnmanagedWrapper.cpp" -type fRepository: cefsharp/CefSharp
Length of output: 121
🏁 Script executed:
sed -n '340,380p' ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp | cat -nRepository: cefsharp/CefSharp
Length of output: 1906
🏁 Script executed:
rg "JavascriptBindingApiAllowOrigins" --type cpp --type h -B 2 -A 2 | head -100Repository: cefsharp/CefSharp
Length of output: 4816
🏁 Script executed:
sed -n '200,250p' ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp | cat -nRepository: cefsharp/CefSharp
Length of output: 3388
🏁 Script executed:
rg "JavascriptBindingApiAllowOrigins\s*=" --type cpp -B 5 -A 5 | head -80Repository: cefsharp/CefSharp
Length of output: 3230
🏁 Script executed:
rg -A 10 "JavascriptBindingApiHasAllowOrigins\s*=" ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp | head -30Repository: cefsharp/CefSharp
Length of output: 587
🏁 Script executed:
rg "JavascriptBindingApiAllowOrigins" ./CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppRepository: cefsharp/CefSharp
Length of output: 451
Add null-safety guard before dereferencing JavascriptBindingApiAllowOrigins pointer.
The flag JavascriptBindingApiHasAllowOrigins and the pointer JavascriptBindingApiAllowOrigins can become inconsistent: if the IPC payload validation fails during initialization, the flag stays true while the pointer remains nullptr. Lines 369 and 371 dereference the pointer without null checks, causing a render process crash.
Add a null check after the flag check:
if (browserWrapper == nullptr || !browserWrapper->JavascriptBindingApiHasAllowOrigins)
{
return true;
}
auto allowOrigins = browserWrapper->JavascriptBindingApiAllowOrigins;
if (!allowOrigins.get())
{
return false; // fail closed when allow-list is expected but unavailable
}Then replace direct accesses to browserWrapper->JavascriptBindingApiAllowOrigins with allowOrigins.
🤖 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 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.
| if (value != null) | ||
| { | ||
| javascriptBindingApiAllowOrigins = Array.ConvertAll( | ||
| value, | ||
| origin => origin.EndsWith("/") ? origin.Substring(0, origin.Length - 1) : origin | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "JavascriptBindingSettings.cs" -type fRepository: cefsharp/CefSharp
Length of output: 118
🏁 Script executed:
wc -l CefSharp/JavascriptBinding/JavascriptBindingSettings.csRepository: cefsharp/CefSharp
Length of output: 120
🏁 Script executed:
sed -n '40,70p' CefSharp/JavascriptBinding/JavascriptBindingSettings.csRepository: cefsharp/CefSharp
Length of output: 1455
🏁 Script executed:
rg "JavascriptBindingApiAllowOrigins" --type csharp -B 2 -A 2Repository: cefsharp/CefSharp
Length of output: 2575
🏁 Script executed:
grep -r "JavascriptBindingApiAllowOrigins" --include="*.cs" | head -20Repository: cefsharp/CefSharp
Length of output: 840
🏁 Script executed:
grep -B 10 -A 5 'settings.JavascriptBindingApiAllowOrigins = origins' CefSharp.Test/JavascriptBinding/JavascriptBindingTests.csRepository: cefsharp/CefSharp
Length of output: 823
🏁 Script executed:
sed -n '1,100p' CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs | grep -A 20 "origins"Repository: cefsharp/CefSharp
Length of output: 43
🏁 Script executed:
sed -n '1,150p' CefSharp.Test/JavascriptBinding/JavascriptBindingTests.csRepository: cefsharp/CefSharp
Length of output: 5846
🏁 Script executed:
ast-grep --pattern 'JavascriptBindingApiAllowOrigins = $_'Repository: cefsharp/CefSharp
Length of output: 175
🏁 Script executed:
sed -n '45,70p' CefSharp/JavascriptBinding/JavascriptBindingSettings.csRepository: cefsharp/CefSharp
Length of output: 1116
🏁 Script executed:
rg "ConvertAll" --type csharp -B 2 -A 2Repository: cefsharp/CefSharp
Length of output: 602
🏁 Script executed:
cat <<'EOF' > /tmp/test.cs
// Test to understand Array.ConvertAll behavior with null elements
using System;
class Test {
static void Main() {
string[] test = new string[] { "http://example.com/", null, "test" };
try {
var result = Array.ConvertAll(test,
origin => origin.EndsWith("/") ? origin.Substring(0, origin.Length - 1) : origin);
} catch (NullReferenceException ex) {
Console.WriteLine("NullReferenceException thrown: " + ex.Message);
}
}
}
EOF
cat /tmp/test.csRepository: cefsharp/CefSharp
Length of output: 563
Handle null entries in allow-origins normalization.
Line 56 dereferences origin unconditionally. The property accepts string[] which can contain null elements—a single null entry will throw a NullReferenceException at EndsWith(), breaking settings assignment.
Suggested fix
if (value != null)
{
javascriptBindingApiAllowOrigins = Array.ConvertAll(
value,
- origin => origin.EndsWith("/") ? origin.Substring(0, origin.Length - 1) : origin
+ origin => origin?.TrimEnd('/')
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (value != null) | |
| { | |
| javascriptBindingApiAllowOrigins = Array.ConvertAll( | |
| value, | |
| origin => origin.EndsWith("/") ? origin.Substring(0, origin.Length - 1) : origin | |
| ); | |
| if (value != null) | |
| { | |
| javascriptBindingApiAllowOrigins = Array.ConvertAll( | |
| value, | |
| origin => origin?.TrimEnd('/') | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CefSharp/JavascriptBinding/JavascriptBindingSettings.cs` around lines 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.
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.
|
❌ Build CefSharp 136.1.40-CI5435 failed (commit e96399376a by @) |
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
Release Notes