Skip to content

Javascript Binding - Finalize JavascriptBindingApiAllowOrigins: Per-browser isolation and origin normalization#5218

Merged
amaitland merged 14 commits intocefsharp:masterfrom
luca-sonntag:feature/jsbindingalloworiginlist-completion
Mar 21, 2026
Merged

Javascript Binding - Finalize JavascriptBindingApiAllowOrigins: Per-browser isolation and origin normalization#5218
amaitland merged 14 commits intocefsharp:masterfrom
luca-sonntag:feature/jsbindingalloworiginlist-completion

Conversation

@luca-sonntag
Copy link
Contributor

@luca-sonntag luca-sonntag commented Feb 27, 2026

Fixes:
#5001

Summary:
This PR completes and optimizes the implementation of the JavascriptBindingApiAllowOrigins feature. 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:

  • Per-Browser Settings Isolation:
    • Relocated JavaScript binding settings (JavascriptBindingApiEnabled, JavascriptBindingApiAllowOrigins, etc.) from the global CefAppUnmanagedWrapper to CefBrowserWrapper. This allows each browser instance to have its own independent list of allowed origins.
  • Origin Normalization (C#):
    • Updated JavascriptBindingSettings.cs to automatically trim trailing slashes from strings added to the JavascriptBindingApiAllowOrigins list, preventing common configuration errors.
  • Optimized C++ Validation Logic:
    • Centralized the security check into CefAppUnmanagedWrapper::IsJavascriptBindingApiAllowed.
    • Implemented normalization in the browser subprocess to strip trailing slashes from the frame origin string before comparison.
    • Switched to case-insensitive comparison (_wcsicmp) to align with standard origin matching behavior.
    • Integrated null checks for origin string pointers to improve stability.

How Has This Been Tested?

  • Executed unit tests in CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs
  • Verified that JavascriptBindingApiAllowOrigins correctly blocks or allows the cefSharp object based on the frame's URL.
  • Confirmed that settings changed in one browser instance do not affect other instances (testing the move to CefBrowserWrapper).
  • OS: Windows.

Screenshots (if appropriate): N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

Summary by CodeRabbit

  • New Features

    • Origin-based access control for the JavaScript binding API (per-browser enablement and allowlist)
    • Automatic trailing-slash normalization for consistent origin matching
    • New settings to configure enablement and allowed origins; allowlist can be empty to allow all
  • Tests

    • Added tests for allowed/blocked origins, empty-origin behavior, trailing-slash handling, and cross-origin navigation impacts on bindings

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 27, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaces a global JS-binding flag with per-browser settings, serializes allowed origins into browser extraInfo, and enforces per-frame origin checks in OnContextCreated to conditionally expose CefSharp JS binding objects. Managed settings and tests added to configure and validate allow-origins behavior.

Changes

Cohort / File(s) Summary
Native browser subprocess
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp, CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
Removed global _jsBindingApiEnabled; added IsJavascriptBindingApiAllowed(CefBrowserWrapper^, CefRefPtr<CefFrame>); parse extraInfo in OnBrowserCreated to populate per-browser JS binding flags and allow-origins; gate binding injection in OnContextCreated using browser-wrapper flags + origin check.
Browser wrapper state
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
Added private MCefRefPtr<CefListValue> _javascriptBindingApiAllowOrigins; new public properties JavascriptBindingApiEnabled, JavascriptBindingApiHasAllowOrigins, JavascriptBindingApiAllowOrigins; initialize/clear these in ctor/dtor.
Managed settings & adapter serialization
CefSharp/JavascriptBinding/JavascriptBindingSettings.cs, CefSharp.Core.Runtime/ManagedCefBrowserAdapter.cpp
Added JavascriptBindingApiAllowOrigins property with trailing-slash normalization and HasJavascriptBindingApiAllowOrigins(); adapter serializes JavascriptBindingApiHasAllowOrigins and JavascriptBindingApiAllowOrigins into browser extraInfo during browser creation.
Tests
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs
Added multiple tests covering allow-origins matching, trailing-slash handling, empty-list behavior, and cross-origin navigation effects; adjusted existing test to assert initial load success before JS evaluation.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant MBA as ManagedCefBrowserAdapter
    participant BI as Browser Init (native)
    participant CAW as CefAppUnmanagedWrapper
    participant BW as BrowserWrapper
    participant Frame as CefFrame / JS

    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(BW, 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 binding objects
        else Not allowed
            CAW->>Frame: Skip binding injection
        end
    else Disabled
        CAW->>Frame: Skip binding injection
    end
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly Related PRs

Poem

🐰 I hopped through frames and trimmed each trailing slash,
I kept the bindings safe from every stray cache.
Origins checked, the whitelist in place,
Only trusted domains may now embrace.
Hop, hop — secure bindings, steady and brash!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving JavaScript binding settings to per-browser scope and adding origin normalization.
Description check ✅ Passed The PR description follows the template structure with all key sections: fixes reference (#5001), detailed summary, specific changes, testing details, and completed checklist.
Linked Issues check ✅ Passed The PR implements solution #3 from #5001: introducing allowed origins settings with automatic CefSharp-enforced validation, enabling per-browser origin-based access restriction.
Out of Scope Changes check ✅ Passed All changes directly support the linked issue objectives: per-browser isolation, origin normalization, and origin-based access control for JavaScript-bound objects.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can customize the tone of the review comments and chat replies.

Configure the tone_instructions setting to customize the tone of the review comments and chat replies. For example, you can set the tone to Act like a strict teacher, Act like a pirate and more.

@luca-sonntag luca-sonntag changed the title Feature/jsbindingalloworiginlist completion Javascript Binding - Finalize JavascriptBindingApiAllowOrigins: Per-browser isolation and origin normalization Feb 27, 2026
@AppVeyorBot
Copy link

Build CefSharp 136.1.40-CI5434 failed (commit c7297af541 by @)

@luca-sonntag luca-sonntag marked this pull request as draft February 27, 2026 13:34
@luca-sonntag luca-sonntag marked this pull request as ready for review March 2, 2026 07:54
@luca-sonntag
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 2, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9aeb5cb and 5d445bb.

📒 Files selected for processing (4)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
  • CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
  • CefSharp/JavascriptBinding/JavascriptBindingSettings.cs

@luca-sonntag luca-sonntag marked this pull request as draft March 2, 2026 08:17
@AppVeyorBot
Copy link

Build CefSharp 136.1.40-CI5435 failed (commit e96399376a by @)

@AppVeyorBot
Copy link

Build CefSharp 136.1.40-CI5436 failed (commit 3067020ea6 by @)

@luca-sonntag luca-sonntag force-pushed the feature/jsbindingalloworiginlist-completion branch from 25d6090 to 6cd5871 Compare March 2, 2026 13:22
@AppVeyorBot
Copy link

Build CefSharp 136.1.40-CI5437 completed (commit a0289d8427 by @)

@luca-sonntag
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 2, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 originStr at 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5d445bb and 6cd5871.

📒 Files selected for processing (4)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
  • CefSharp.BrowserSubprocess.Core/Stdafx.h
  • CefSharp.Core.Runtime/Stdafx.h
  • CefSharp/JavascriptBinding/JavascriptBindingSettings.cs

@luca-sonntag luca-sonntag marked this pull request as ready for review March 2, 2026 13:54
@luca-sonntag
Copy link
Contributor Author

@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.

@amaitland
Copy link
Member

@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.

@amaitland
Copy link
Member

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.

Once you've rebased on master then please revert the unnecessary changes. Thanks

@amaitland
Copy link
Member

Additional tests will also need to be written. #5085 (comment)

@luca-sonntag luca-sonntag force-pushed the feature/jsbindingalloworiginlist-completion branch from 6cd5871 to b916db1 Compare March 3, 2026 09:18
@luca-sonntag luca-sonntag changed the base branch from feature/jsbindingalloworiginlist to master March 3, 2026 09:19
amaitland and others added 7 commits March 3, 2026 10:21
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.
Luca Sonntag added 2 commits March 3, 2026 10:21
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 luca-sonntag force-pushed the feature/jsbindingalloworiginlist-completion branch from b916db1 to 6cefcea Compare March 3, 2026 10:24
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Popups bypass JavaScript binding origin restrictions.

At line 67, OnBrowserCreated returns early for popups (null extraInfo), leaving JavascriptBindingApiEnabled=true and JavascriptBindingApiHasAllowOrigins=false. This causes IsJavascriptBindingApiAllowed() to return true regardless 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 extraInfo with 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd5871 and 6cefcea.

📒 Files selected for processing (6)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
  • CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
  • CefSharp.Core.Runtime/ManagedCefBrowserAdapter.cpp
  • CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs
  • CefSharp/JavascriptBinding/JavascriptBindingSettings.cs

@AppVeyorBot
Copy link

Build CefSharp 145.0.260-CI5438 completed (commit ab5e7166dd by @)

@luca-sonntag luca-sonntag force-pushed the feature/jsbindingalloworiginlist-completion branch from 6cefcea to b54d60f Compare March 3, 2026 11:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6cefcea and b54d60f.

📒 Files selected for processing (1)
  • CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs

@AppVeyorBot
Copy link

Build CefSharp 145.0.260-CI5439 completed (commit fc2929530d by @)

@luca-sonntag
Copy link
Contributor Author

luca-sonntag commented Mar 3, 2026

@amaitland Branch is rebased and targeted to master.

Improved test coverage for JavaScript binding origin validation:

  • Added tests for origins with and without trailing slashes.
  • Expanded validation for multiple origins in the allowlist.
  • Added assertions to verify successful page load before script execution.

@amaitland
Copy link
Member

Nice one, thanks! Will review in detail on the weekend

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks solid, thanks for the hard work 👍

Could of questions/comments inline.

Test wise we probably need some cross origin navigation tests to confirm that's working correctly.

Luca Sonntag added 4 commits March 18, 2026 09:29
Refactored IsJavascriptBindingApiAllowed to accept a CefBrowserWrapper^ parameter directly, eliminating redundant lookups and improving clarity. Updated all call sites to pass the browser wrapper explicitly.
Clarify documentation for HasJavascriptBindingApiAllowOrigins method to specify zero or more origins
Refactored code to directly compare wide string origins using
_wcsicmp, removing unnecessary pointer casts and null checks.
This streamlines the logic and improves readability.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)

350-393: The behavior for special URLs like about:blank appears intentional, but clarification and testing are recommended.

When JavascriptBindingApiAllowOrigins is not configured, the function correctly returns true at lines 352–355, allowing binding on all frames including about:blank. However, when origin restrictions are set and CefParseURL fails to parse a URL (e.g., about:blank, data:, file:// schemes), the function returns false, blocking the binding API.

This fail-closed behavior is appropriate for a security feature restricting the binding API to configured origins. However, there are no test cases covering the scenario where allow-origins is configured and a frame with an unparseable URL attempts to use the binding API. Consider adding a test case to document this behavior explicitly, or add a comment explaining the design choice for clarity.

🤖 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 350
- 393, IsJavascriptBindingApiAllowed currently treats unparseable frame URLs
(e.g., about:blank, data:, file:) as disallowed when
JavascriptBindingApiAllowOrigins is configured because CefParseURL fails and the
function returns false; add an explicit unit test covering the case where
JavascriptBindingApiAllowOrigins is set and a frame with an unparseable URL
attempts to use the binding API to document and lock in this fail-closed
behavior, and/or add a concise in-code comment in
CefAppUnmanagedWrapper::IsJavascriptBindingApiAllowed explaining that
CefParseURL failures (unparseable schemes) intentionally deny access when
allow-origins is configured to ensure security.
🤖 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 350-393: IsJavascriptBindingApiAllowed currently treats
unparseable frame URLs (e.g., about:blank, data:, file:) as disallowed when
JavascriptBindingApiAllowOrigins is configured because CefParseURL fails and the
function returns false; add an explicit unit test covering the case where
JavascriptBindingApiAllowOrigins is set and a frame with an unparseable URL
attempts to use the binding API to document and lock in this fail-closed
behavior, and/or add a concise in-code comment in
CefAppUnmanagedWrapper::IsJavascriptBindingApiAllowed explaining that
CefParseURL failures (unparseable schemes) intentionally deny access when
allow-origins is configured to ensure security.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b5583cb2-bd7a-4269-8823-8cf9b24045b6

📥 Commits

Reviewing files that changed from the base of the PR and between b54d60f and fa32b5e.

📒 Files selected for processing (4)
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
  • CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
  • CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs
  • CefSharp/JavascriptBinding/JavascriptBindingSettings.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • CefSharp/JavascriptBinding/JavascriptBindingSettings.cs

@AppVeyorBot
Copy link

Build CefSharp 145.0.260-CI5440 completed (commit 68a619693e by @)

@amaitland amaitland merged commit 00d57a5 into cefsharp:master Mar 21, 2026
2 checks passed
@amaitland amaitland added this to the 146.0.x milestone Mar 21, 2026
@amaitland
Copy link
Member

Thanks for the contribution 👍 Should be available in the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants