Javascript Binding - Add ability to limit access to JavaScript Bound objects to specific origins #5085
Conversation
| cefSharpObj->SetValue(kRenderProcessId, CefV8Value::CreateInt(processId), CefV8Value::PropertyAttribute::V8_PROPERTY_ATTRIBUTE_NONE); | ||
|
|
||
| global->SetValue(_jsBindingPropertyName, cefSharpObj, CefV8Value::PropertyAttribute::V8_PROPERTY_ATTRIBUTE_READONLY); | ||
| createObjects = false; |
There was a problem hiding this comment.
Extract this into a helper method would probably make this a lot cleaner
| @@ -33,6 +35,24 @@ public bool JavascriptBindingApiEnabled | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// When <see cref="JavascriptBindingApiEnabled"/> is set to true, set a collection | |||
| @@ -95,5 +115,17 @@ public bool AlwaysInterceptAsynchronously | |||
| alwaysInterceptAsynchronously = value; | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// HasJavascriptBindingApiAllowOrigins | |||
|
Could probably use some more tests, specifically for multiple origins in the list |
|
❌ Build CefSharp 135.0.170-CI5207 failed (commit bcf0a561b5 by @amaitland) |
|
❌ Build CefSharp 135.0.170-CI5209 failed (commit 8a395dfd16 by @amaitland) |
amaitland
force-pushed
the
feature/jsbindingalloworiginlist
branch
2 times, most recently
from
218de9c to
60203e5
Compare
|
|
||
| auto clrOrigin = StringUtils::ToClr(origin); | ||
|
|
||
| auto originEqual = String::Compare(clrframeUrlOrigin, clrOrigin, StringComparison::InvariantCultureIgnoreCase); |
There was a problem hiding this comment.
Need to find a more efficient string comparison, ideally compare the native strings
|
✅ Build CefSharp 135.0.170-CI5213 completed (commit 0ac4c7d444 by @amaitland) |
| /// <remarks> | ||
| /// If you wish to create the CefSharp object for a limited set of origins then set this property | ||
| /// </remarks> | ||
| public string[] JavascriptBindingApiAllowOrigins |
There was a problem hiding this comment.
The origin returned from CEF when it's parsed always has a trailing /, we should ensure that there's always a trailing slash programmatically
| @@ -29,6 +29,8 @@ namespace CefSharp | |||
| bool _focusedNodeChangedEnabled; | |||
| bool _legacyBindingEnabled; | |||
| bool _jsBindingApiEnabled = true; | |||
There was a problem hiding this comment.
Storing these on a per browser basis would provide greater flexibility, we could create a class that has a lookup by browser id.
amaitland
force-pushed
the
feature/jsbindingalloworiginlist
branch
from
60203e5 to
9aeb5cb
Compare
WalkthroughThe changes implement origin-based restrictions for JavaScript binding APIs in CefSharp. New settings and logic allow specifying allowed origins, and JavaScript binding objects are only created if the current page's origin matches these. Tests are added to verify this behavior, and internal state is updated to support the new restrictions. Changes
Sequence Diagram(s)sequenceDiagram
participant BrowserProcess
participant RendererProcess
participant CefAppUnmanagedWrapper
BrowserProcess->>RendererProcess: Create browser (with extraInfo: allowed origins)
RendererProcess->>CefAppUnmanagedWrapper: OnContextCreated (frame URL)
CefAppUnmanagedWrapper->>CefAppUnmanagedWrapper: Parse frame URL, extract origin
CefAppUnmanagedWrapper->>CefAppUnmanagedWrapper: Check if origin is in allowed list
alt Origin allowed or no restrictions
CefAppUnmanagedWrapper->>RendererProcess: Create JS binding objects
else Origin not allowed
CefAppUnmanagedWrapper-->>RendererProcess: Do not create JS binding objects
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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: 1
♻️ Duplicate comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (1)
165-195: 🛠️ Refactor suggestionAvoid round-tripping through managed strings for origin comparison
The loop currently converts every allowed origin and the frame origin to managed
System::Stringbefore doing an invariant-culture compare:auto clrframeUrlOrigin = StringUtils::ToClr(frameUrlOrigin); // … auto clrOrigin = StringUtils::ToClr(origin); auto originEqual = String::Compare(clrframeUrlOrigin, clrOrigin, StringComparison::InvariantCultureIgnoreCase);This adds two allocations per iteration, crosses the managed/native boundary,
and duplicates previous logic flagged in an earlier review.
A more efficient/native-only approach would be:- auto frameUrlOrigin = CefString(frameUrlParts.origin.str, - frameUrlParts.origin.length); + // frameUrlParts.origin already is a |cef_string_t| + const CefString& frameUrlOrigin = frameUrlParts.origin; - auto originEqual = String::Compare(clrframeUrlOrigin, clrOrigin, - StringComparison::InvariantCultureIgnoreCase); + auto originEqual = + frameUrlOrigin.Compare(origin, /*case_insensitive=*/true) == 0;This:
- Eliminates managed allocations and GC pressure.
- Keeps comparison purely in the native layer.
- Leaves behaviour (case-insensitive compare) unchanged.
Consider extracting the whole “origin matches allow-list” logic into a small
helper to makeOnContextCreatedeasier to read and unit-test.
🧹 Nitpick comments (4)
CefSharp.Test/CefSharpFixture.cs (1)
67-67: Consider adding a comment explaining this line's purpose.This commented-out line adds a command-line argument to disable the "SpareRendererForSitePerProcess" feature. Since it's commented out and not explained, it's unclear why it was added or under what conditions it might be enabled.
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
32-33: Consider implementing a per-browser approach instead of global state.The implementation currently stores allowed origins as global state in the
CefAppUnmanagedWrapper. This works for the current use case but may be less flexible when different browsers need different origin restrictions.A more flexible approach would be to store these settings on a per-browser basis using a lookup by browser ID, which would allow different origin restrictions for different browser instances.
CefSharp/JavascriptBinding/JavascriptBindingSettings.cs (1)
119-129: Improve XML documentation for the HasJavascriptBindingApiAllowOrigins method.The current XML documentation only repeats the method name without providing additional context. Consider expanding the documentation to explain when and why this method would be used.
/// <summary> -/// HasJavascriptBindingApiAllowOrigins +/// Checks if there are any allowed origins specified for JavaScript binding API. /// </summary> +/// <remarks> +/// This method is used internally to determine if origin-based restrictions +/// should be applied when creating JavaScript binding API objects. +/// </remarks> /// <returns>bool true if <see cref="JavascriptBindingApiAllowOrigins"/> is non empty collection.</returns>CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (1)
160-185: Assert the load result for consistencyIn the preceding tests you verify
loadResponse.Successafter the initial
WaitForInitialLoadAsynccall. Doing the same here guards against silent HTTP
errors that would otherwise surface only as “undefined” object checks.- await browser.WaitForInitialLoadAsync(); + var loadResponse = await browser.WaitForInitialLoadAsync(); + Assert.True(loadResponse.Success);(Not critical for correctness of the feature, but keeps the test suite’s style
and diagnostics consistent.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp(3 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h(2 hunks)CefSharp.Core.Runtime/ManagedCefBrowserAdapter.cpp(1 hunks)CefSharp.Test/CefSharpFixture.cs(1 hunks)CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs(1 hunks)CefSharp/JavascriptBinding/JavascriptBindingSettings.cs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs (5)
CefSharp.Test/JavascriptBinding/JavaScriptObjectRepositoryTests.cs (3)
Fact(32-50)Fact(52-60)Fact(63-89)CefSharp.Test/JavascriptBinding/JavascriptBindingSimpleTest.cs (1)
Fact(27-53)CefSharp/Internals/JavascriptObjectRepository.cs (4)
Task(419-422)Task(424-568)JavascriptObjectRepository(35-795)JavascriptObjectRepository(114-118)CefSharp.Test/WebBrowserTestExtensions.cs (2)
Task(57-119)Task(121-149)CefSharp.Example/CefExample.cs (1)
CefExample(15-274)
🪛 Cppcheck (2.10-2)
CefSharp.Core.Runtime/ManagedCefBrowserAdapter.cpp
[error] 90-90: syntax error
(syntaxError)
🔇 Additional comments (5)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
75-75: Good cleanup of the CefRefPtr.Properly setting the
_jsBindingApiAllowOriginsto nullptr in the destructor prevents potential memory leaks.CefSharp.Core.Runtime/ManagedCefBrowserAdapter.cpp (2)
87-102: Implementation correctly propagates origin restrictions.The code properly checks for allowed origins and adds them to the extra info dictionary along with a flag indicating their presence. This allows the native code to enforce origin-based restrictions for JavaScript binding.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 90-90: syntax error
(syntaxError)
90-90: False positive from static analysis.The static analysis tool reported a syntax error here, but the code is syntactically correct. This is a false positive and can be ignored.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 90-90: syntax error
(syntaxError)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (2)
103-114: Extraction and lifetime management look correctThe flag/array are read once and copied, mirroring the handling of other settings.
No correctness, ownership, or thread-safety issues spotted here.
198-245: Conditional creation path integrates cleanlyMoving the binding-object creation behind the
createObjectsgate keeps existing
behaviour when no restrictions are configured while avoiding object exposure
for disallowed origins. 👍
| /// <summary> | ||
| /// When <see cref="JavascriptBindingApiEnabled"/> is set to true, set a collection | ||
| /// of origins to limit which origins CefSharp will create it's global (window) object. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If you wish to create the CefSharp object for a limited set of origins then set this property | ||
| /// </remarks> | ||
| public string[] JavascriptBindingApiAllowOrigins | ||
| { | ||
| get { return javascriptBindingApiAllowOrigins; } | ||
| set | ||
| { | ||
| ThrowIfFrozen(); | ||
|
|
||
| javascriptBindingApiAllowOrigins = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure origins have trailing slashes.
The origins returned by CEF always include a trailing slash when parsed. To ensure consistent matching, the code should normalize all origins by adding trailing slashes if they don't exist.
public string[] JavascriptBindingApiAllowOrigins
{
get { return javascriptBindingApiAllowOrigins; }
set
{
ThrowIfFrozen();
- javascriptBindingApiAllowOrigins = value;
+ // Ensure all origins have trailing slashes to match CEF's format
+ if (value != null)
+ {
+ javascriptBindingApiAllowOrigins = Array.ConvertAll(value, origin =>
+ origin.EndsWith("/") ? origin : origin + "/");
+ }
+ else
+ {
+ javascriptBindingApiAllowOrigins = null;
+ }
}
}You'll need to add using System; at the top of the file for this change.
🏁 Script executed:
#!/bin/bash
# Locate the settings file
file=$(fd JavascriptBindingSettings.cs)
echo "File located at: $file"
echo
echo "=== Using directives ==="
grep -n "^using" "$file"
echo
echo "=== File header (lines 1–30) ==="
sed -n '1,30p' "$file"
echo
echo "=== Property snippet (lines 30–90) ==="
sed -n '30,90p' "$file"Length of output: 4593
🏁 Script executed:
#!/bin/bash
# Search for references to javascriptBindingApiAllowOrigins and origin matching logic
echo "References to javascriptBindingApiAllowOrigins:"
rg -n 'javascriptBindingApiAllowOrigins' -A3 -B3 CefSharp/JavascriptBinding/JavascriptBindingSettings.cs
echo
echo "Search for origin matching in repository:"
rg -n 'AllowOrigins' -A3 -B3 -t cs .Length of output: 6462
🏁 Script executed:
#!/bin/bash
# Inspect how AllowOrigins is used in the JavascriptBinding tests
echo "=== References to AllowOrigins in tests ==="
rg -n 'AllowOrigins' -A5 -B5 CefSharp.Test/JavascriptBinding/JavascriptBindingTests.cs
echo
echo "=== Test file snippet (lines 120–200) ==="
sed -n '120,200p' CefSharp.Test/JavascriptBinding/JavascriptBindingTests.csLength of output: 5139
Normalize JavascriptBindingApiAllowOrigins to Always Include a Trailing Slash
To prevent mismatches against CEF’s parsed origins (which always end with “/”), normalize each entry in the setter so that it always has a trailing slash:
File: CefSharp/JavascriptBinding/JavascriptBindingSettings.cs
public string[] JavascriptBindingApiAllowOrigins
{
get { return javascriptBindingApiAllowOrigins; }
set
{
ThrowIfFrozen();
- javascriptBindingApiAllowOrigins = value;
+ if (value != null)
+ {
+ javascriptBindingApiAllowOrigins = Array.ConvertAll(
+ value,
+ origin => origin.EndsWith("/") ? origin : origin + "/"
+ );
+ }
+ else
+ {
+ javascriptBindingApiAllowOrigins = null;
+ }
}
}Don’t forget to add the namespace at the top of the file:
using System;This change ensures that origins supplied without a trailing slash will correctly match CEF’s internal origin strings.
|
✅ Build CefSharp 136.1.40-CI5256 completed (commit f728ec3ccf by @amaitland) |
Fixes:
#5001
Summary:
Changes:
How Has This Been Tested?
Types of changes
Checklist:
Summary by CodeRabbit