fix: replace eval-based JS interop with dedicated module (fixes #216)#810
fix: replace eval-based JS interop with dedicated module (fixes #216)#810
Conversation
…s module
Replace ~46 instances of JS.InvokeVoidAsync("eval", ...) across 7 Razor
files with named functions in a dedicated JS module. This eliminates XSS
risks from string interpolation in eval calls, removes prompt injection
surface, and improves maintainability.
Changes:
- Create wwwroot/js/polypilot-interop.js with all named JS functions
- Add script tag to index.html (loads before Blazor)
- Replace all eval calls in MainLayout, Settings, Dashboard,
ExpandedSessionView, SessionSidebar, and DiffView
- Update structural tests to search JS module instead of Razor files
- Add integration test verifying all functions are registered
Fixes #216
Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #810Build Results
✅ All platforms verified Triggered by: verify-build run |
🧪 Integration Test Report — PR #810
✅ All platforms passed |
Expert Code Review — PR #810Design-Level Findings (outside diff hunks)🟢 MINOR — <span data-trigger="log-`@EscapeForJs`(Session.SessionId ?? "")" ...>
🟢 MINOR — 🟢 MINOR — i️ Discarded finding — Warning
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — JS Interop Module Migration
Overall: Well-executed security improvement. Eliminating ~46 eval calls removes the string-interpolation XSS vector. The old popup code already used safe DOM APIs (createElement/textContent for user data, server-side EscapeHtml() before innerHTML for pre-built HTML), and these protections carry over correctly. No regressions in the security model.
🟡 MODERATE — Integration test missing 2 functions
JsInteropModuleTests.cs, InteropModule_AllCoreFunctionsRegistered function array
removeSettingsContentActiveClass and clearSettingsRef are both called from Settings.razor (dispose path) but are not in the integration test's function list. The test claims to verify "all named functions" but misses these 2. If either is accidentally removed from the JS module, Settings dispose fails silently (fire-and-forget _ =).
Fix: Add both to the functions array:
"removeSettingsContentActiveClass", "clearSettingsRef",🟢 MINOR — Fragile function boundary detection in PopupThemeTests
PopupThemeTests.cs, Razor_PromptsPopup_UsesCssClasses
The end-of-function boundary uses js.IndexOf("\nwindow.", startIdx + 10) — if a comment or string containing \nwindow. appears inside the function body, methodBody gets truncated prematurely. Consider asserting against the full JS file content (the CSS class names are unique enough) or using brace-depth counting.
🟢 MINOR — Weakened draft-restore safety assertion
ChatExperienceSafetyTests.cs, DraftRestore_Source_PreservesLiveTyping
The old test asserted window.__liveDrafts in both Dashboard.razor and index.html, forming a two-layer safety check (Razor captures → JS restores). Now only the JS module is checked. Consider adding Assert.Contains("captureDrafts", dashboard) to verify Dashboard.razor still calls the JS function, preserving the cross-layer invariant.
No critical findings. Security improvement is sound — Blazor JSON serialization handles parameter escaping, and the existing EscapeHtml()/textContent patterns protect against innerHTML-based XSS in popup rendering.
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
api.nuget.orgdc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.nuget.org"
- "dc.services.visualstudio.com"See Network Configuration for more information.
Generated by Expert Code Review · ● 94.2M
| var functions = new[] | ||
| { | ||
| "setDataTheme", "setDataPlatform", "setAppFontSize", | ||
| "startSidebarResize", "blurActiveElement", | ||
| "clearSettingsSearchInput", "scrollToSettingsCategory", | ||
| "wireSettingsSearch", "setupCategoryIntersectionObserver", | ||
| "ensureDashboardKeyHandlers", "clearDashRef", | ||
| "ensureTextareaAutoResize", "setDashboardScrollTop", | ||
| "getDashboardScrollTop", "ensureLoadMoreObserver", | ||
| "captureDrafts", "scrollMessagesToBottom", | ||
| "focusAndSelect", "saveDraftsAndCursor", | ||
| "setInputValueAndCursor", "showPopup", | ||
| "showAgentsPopup", "showPromptsPopup", | ||
| "wireSessionNameInputEnter", "clearSidebarRef", | ||
| "invokeDashboardCollapseToGrid", "scrollAndFocusCommentBox", | ||
| "clearPromptRef", | ||
| }; | ||
|
|
||
| var checkExpr = "JSON.stringify([" + | ||
| string.Join(",", functions.Select(f => $"typeof window.{f} === 'function'")) + | ||
| "])"; | ||
|
|
||
| var result = await CdpEvalAsync(checkExpr); |
There was a problem hiding this comment.
🟡 MODERATE · 2/2 reviewers
Integration test is missing several functions from its verification list. The functions array here contains 28 entries, but removeSettingsContentActiveClass and clearSettingsRef (both called fire-and-forget from Settings.razor disposal) are absent. The ref-setter functions (__setNavRef, __setDashRef, __setSettingsRef, __setSidebarRef, __ppSetRef) are also missing — their absence from polypilot-interop.js would cause component initialization failures that this test wouldn't catch.
Scenario: A future refactor accidentally removes removeSettingsContentActiveClass from polypilot-interop.js. Settings page disposal silently throws JSException on every navigation away. This test — the only guard — doesn't catch it.
Suggestion: Add the missing functions to the array:
"removeSettingsContentActiveClass", "clearSettingsRef",
"__setNavRef", "__setDashRef", "__setSettingsRef", "__setSidebarRef", "__ppSetRef",| document.body.setAttribute('data-platform', platform); | ||
| }; | ||
|
|
||
| window.setAppFontSize = function (fontSize) { |
There was a problem hiding this comment.
🟡 MODERATE — Duplicate setAppFontSize definition (2/3 reviewers + independent verification)
This function is also defined in index.html (line ~401), which loads after polypilot-interop.js. The index.html version silently overwrites this one, making this definition dead code.
Both implementations are functionally identical today, but if either is updated without the other (e.g., adding Math.round() or input validation), the behavior will diverge silently — the index.html version always wins.
Failing scenario: A developer updates this definition thinking it's the canonical one (since it's in the interop module), but the change has no effect because index.html overwrites it at load time.
Fix: Remove the duplicate from index.html (since this module is now the canonical location for interop functions), or remove it from here and add a comment pointing to the index.html definition.
| popup.style.left = left + 'px'; | ||
| popup.innerHTML = headerHtml + contentHtml; | ||
| popup.onclick = function (e) { e.stopPropagation(); }; | ||
| ov.appendChild(popup); |
There was a problem hiding this comment.
🟢 MINOR — innerHTML sink requires caller discipline (2/3 reviewers)
popup.innerHTML = headerHtml + contentHtml is safe today because all callers pass pre-escaped content (EscapeHtml() on all dynamic values, static strings for headers). However, this is a latent XSS surface — a future caller forgetting EscapeHtml() would create an injection vector.
Consider adding a brief safety comment here:
// SAFETY: callers MUST EscapeHtml() all user-supplied content before passing to this function.
popup.innerHTML = headerHtml + contentHtml;No code change needed — just documentation to prevent future regressions.
| }); | ||
| if (active && active.id) result['__focused'] = active.id; | ||
| if (active) { result['__selStart'] = active.selectionStart || 0; result['__selEnd'] = active.selectionEnd || 0; } |
There was a problem hiding this comment.
🟡 Pre-existing bug, now trivially fixable (3/3 reviewers)
Two issues in captureDrafts that existed in the old eval code and are faithfully reproduced here:
-
selectionStartthrows on non-text inputs. Ifdocument.activeElementis an<input type="number">(Dashboard.razor has at least two), Chromium throwsInvalidStateErroronselectionStartaccess. The|| 0fallback only handles falsy values—it does not catch exceptions. The exception propagates through Blazor interop asJSException, the C#catch { }swallows it, and all draft data for that render cycle is silently lost. (saveDraftsAndCursorat line 401 correctly gates this behindfocused.matches(sel).) -
JSON number →
Dictionary<string, string>mismatch.selectionStart || 0produces a JS integer.JSON.stringifyemits{"__selStart":0}. The C# caller deserializes viaDeserialize<Dictionary<string, string>>which throwsJsonExceptionon JSON integers (strict by default in System.Text.Json). Same silent-discard via the outercatch { }.
Neither is a regression from this PR. But since the code is now centralized, it's the ideal time to fix:
if (active && active.id) result['__focused'] = active.id;
try {
if (active && typeof active.selectionStart === 'number') {
result['__selStart'] = String(active.selectionStart);
result['__selEnd'] = String(active.selectionEnd ?? 0);
}
} catch (_) { /* non-text input type */ }| "setDataTheme", "setDataPlatform", "setAppFontSize", | ||
| "startSidebarResize", "blurActiveElement", | ||
| "clearSettingsSearchInput", "scrollToSettingsCategory", | ||
| "wireSettingsSearch", "setupCategoryIntersectionObserver", | ||
| "ensureDashboardKeyHandlers", "clearDashRef", | ||
| "ensureTextareaAutoResize", "setDashboardScrollTop", | ||
| "getDashboardScrollTop", "ensureLoadMoreObserver", | ||
| "captureDrafts", "scrollMessagesToBottom", | ||
| "focusAndSelect", "saveDraftsAndCursor", | ||
| "setInputValueAndCursor", "showPopup", | ||
| "showAgentsPopup", "showPromptsPopup", | ||
| "wireSessionNameInputEnter", "clearSidebarRef", | ||
| "invokeDashboardCollapseToGrid", "scrollAndFocusCommentBox", | ||
| "clearPromptRef", | ||
| }; | ||
|
|
||
| var checkExpr = "JSON.stringify([" + | ||
| string.Join(",", functions.Select(f => $"typeof window.{f} === 'function'")) + |
There was a problem hiding this comment.
🟢 Minor: Two functions missing from completeness check (1/3 reviewers, objectively verified)
The module defines removeSettingsContentActiveClass (line 123) and clearSettingsRef (line 46) — both called from Settings.razor's Dispose — but they're not in this list. Consider adding them for full coverage:
"clearSettingsRef",
"removeSettingsContentActiveClass",
Review-Fix Loop — Round 1 of 3Findings Addressed: 6 of 8
Test Results
Next Steps
Warning
|
Cross-Platform Verification — PR #810Build Results
✅ All platforms verified Previous Review HistoryFound 4 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
1 participant
Summary
Replaces ~46 instances of
JS.InvokeVoidAsync("eval", ...)across 7 Razor files with named functions in a dedicatedpolypilot-interop.jsmodule. This eliminates XSS risks from string interpolation in eval calls, removes prompt injection surface, and improves maintainability.Changes
New file:
wwwroot/js/polypilot-interop.jsA ~660-line JS module containing all named interop functions, organized by component:
focusAndSelect,blurActiveElement__setNavRef,__setDashRef,__setSidebarRef,__ppSetRef)setDataTheme,setDataPlatform,setAppFontSize,startSidebarResizeclearSettingsSearchInput,scrollToSettingsCategory,wireSettingsSearch,setupCategoryIntersectionObserverensureDashboardKeyHandlers(230-line keyboard handler),ensureTextareaAutoResize,ensureLoadMoreObserver,captureDrafts,scrollMessagesToBottom,saveDraftsAndCursor,setInputValueAndCursorwireSessionNameInputEnter,clearSidebarRef,invokeDashboardCollapseToGridshowPopup,showAgentsPopup,showPromptsPopup,clearPromptRefscrollAndFocusCommentBoxModified Razor files (7 files, -669 lines of inline JS)
MainLayout.razor— 8 eval calls replacedSettings.razor— 10 eval calls replacedDashboard.razor— 13 eval calls replacedExpandedSessionView.razor— 8 eval calls replaced (3 for prompts popup consolidated into 1)SessionSidebar.razor— 6 eval calls replacedDiffView.razor— 1 eval call replacedUpdated tests
PopupThemeTests.cs— Updated to search JS module for popup CSS classesPromptManagementPopupTests.cs— Updated to search JS module for popup UI stringsChatExperienceSafetyTests.cs— Updated draft-restore test to check JS moduleNew integration test
JsInteropModuleTests.cs— Verifies all 28 named functions are registered onwindow, testssetDataThemeandsetAppFontSizework correctly end-to-endSecurity improvement
eval()— any unescaped user input could execute arbitrary JSTest results
✅ 3661 unit tests pass
✅ Integration tests build successfully
Fixes Replace eval-based JS interop with dedicated JS module #216
Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.