Security fixes#219
Conversation
…-between-incompatible-types
WalkthroughAdds an HTML sanitization hook and integrates it into HTML parsing, prevents prototype-pollution in DOM data helpers, adjusts computed disposal option sourcing, refines mapping helpers for undefined roots, updates JSON parsing, and introduces test lifecycle helpers and test-time sanitizer behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/utils/src/dom/html.ts (1)
170-179: Critical: jQuery path bypasses HTML sanitization.The return value from
validateHTMLInput(html)at line 170 is discarded. When the jQuery path is taken (line 179), the original unsanitizedhtmlis passed tojQuery(node).html(html), completely bypassing theoptions.sanitizeHtmlTemplate()sanitization.This defeats the purpose of the new sanitization feature for browsers where
supportsTemplateTagis false and jQuery is present.🔎 Proposed fix
validateHTMLInput(html) + html = validateHTMLInput(html) const jQuery = options.jQuery // If the browser supports <template> tags, prefer that, as // it obviates all the complex workarounds of jQuery.Alternatively, since
parseHtmlFragmentalready sanitizes internally, you could simplify by always using it:- validateHTMLInput(html) - const jQuery = options.jQuery - // If the browser supports <template> tags, prefer that, as - // it obviates all the complex workarounds of jQuery. - // - // However, jQuery contains a lot of sophisticated code to parse arbitrary HTML fragments, - // for example <tr> elements which are not normally allowed to exist on their own. - // If you've referenced jQuery (and template tags are not supported) we'll use that rather than duplicating its code. - if (jQuery && !supportsTemplateTag) { - jQuery(node).html(html) - } else { - // ... otherwise, use KO's own parsing logic. - let parsedNodes : Node[] - if(node.ownerDocument) { - parsedNodes = parseHtmlFragment(html, node.ownerDocument) - } - else { - parsedNodes = parseHtmlFragment(html) - } + // Use KO's parsing logic which includes sanitization + let parsedNodes : Node[] + if(node.ownerDocument) { + parsedNodes = parseHtmlFragment(html, node.ownerDocument) + } + else { + parsedNodes = parseHtmlFragment(html) + } - if (node.nodeType === 8) { - if (html === null) { - virtualElements.emptyNode(node) - } else { - virtualElements.setDomNodeChildren(node, parsedNodes) - } + if (node.nodeType === 8) { + if (html === null) { + virtualElements.emptyNode(node) } else { - for (let i = 0; i < parsedNodes.length; i++) { node.appendChild(parsedNodes[i]) } + virtualElements.setDomNodeChildren(node, parsedNodes) } + } else { + for (let i = 0; i < parsedNodes.length; i++) { node.appendChild(parsedNodes[i]) } }
🧹 Nitpick comments (2)
packages/utils/src/options.ts (1)
95-104: Consider rate-limiting the warning message.The sanitization hook is well-designed with clear documentation. However, the warning will be logged every time HTML is sanitized, which could spam the console during normal application use.
💡 Suggested improvement to log warning only once
+ private _sanitizeWarningLogged: boolean = false + /** * Sanitize HTML templates before parsing them. Default is a no-op. * Please configure something like DOMPurify or validator.js for your environment. * @param html HTML string to be sanitized * @returns Sanitized HTML string */ sanitizeHtmlTemplate(html: string): string { - console.log('WARNING -- You don\'t have a HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate to avoid XSS vulnerabilities.') + if (!this._sanitizeWarningLogged) { + console.warn('WARNING -- You don\'t have a HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate to avoid XSS vulnerabilities.') + this._sanitizeWarningLogged = true + } return html }Also consider using
console.warninstead ofconsole.logfor warnings.builds/knockout/helpers/jasmine.extensions.js (1)
206-214: Consider using const/let instead of var.The logic correctly toggles jQuery usage based on Karma arguments. Minor suggestion: use modern variable declarations.
💡 Optional modernization
const KARMA_STRING = '__karma__' -var disableJQueryUsage = true; +let disableJQueryUsage = true; function switchJQueryState() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)builds/knockout/helpers/jasmine.extensions.js(1 hunks)packages/bind/src/arrayToDomNodeChildren.ts(1 hunks)packages/binding.foreach/spec/eachBehavior.ts(1 hunks)packages/computed/src/computed.ts(1 hunks)packages/observable/src/mappingHelpers.ts(2 hunks)packages/utils/helpers/jasmine-13-helper.ts(1 hunks)packages/utils/src/dom/data.ts(3 hunks)packages/utils/src/dom/html.ts(1 hunks)packages/utils/src/options.ts(2 hunks)packages/utils/src/string.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/binding.foreach/spec/eachBehavior.ts (1)
packages/binding.core/src/options.ts (1)
options(13-172)
packages/bind/src/arrayToDomNodeChildren.ts (1)
packages/utils/src/array.ts (1)
arrayMap(25-28)
builds/knockout/helpers/jasmine.extensions.js (1)
tko.io/src/tko-io.js (1)
window(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, off)
- GitHub Check: testheadless (22.x, on)
🔇 Additional comments (17)
packages/bind/src/arrayToDomNodeChildren.ts (1)
140-140: LGTM! Redundant condition removed.The conditional check for
isFirstExecutionwas redundant since this code is already within theelseblock where!isFirstExecutionis true. The refactored version is cleaner and equivalent.README.md (1)
109-114: LGTM! Clear security documentation added.The documentation clearly explains the sanitization option and provides practical examples using well-known libraries. The recommendation to implement CSP adds an important defense-in-depth layer.
packages/utils/src/string.ts (1)
21-22: LGTM! Simplified and more secure.Removing the Function-based fallback eliminates a potential code injection vector. All modern browsers (including IE8+) support
JSON.parsenatively, so this simplification is safe and improves security.packages/utils/src/dom/data.ts (5)
9-9: LGTM! Safer initialization.Initializing
dataStoreas an empty object instead of potentially undefined adds clarity and safety.
12-15: LGTM! Critical prototype pollution prevention.The
isSafeKeyfunction effectively prevents prototype pollution attacks by blocking dangerous property names (__proto__,constructor,prototype). This is a crucial security fix.
78-83: LGTM! Proper validation in get method.The unsafe key check with clear error message provides good protection and debugging information.
85-92: LGTM! Proper validation in set method.Consistent validation pattern applied to the set method.
94-98: LGTM! Proper validation in getOrSet method.All three data access methods now have consistent protection against prototype pollution.
packages/utils/helpers/jasmine-13-helper.ts (1)
250-253: LGTM! Appropriate test setup.Disabling sanitization in tests with an identity function is appropriate to avoid log pollution during test runs while still exercising the code paths.
packages/binding.foreach/spec/eachBehavior.ts (1)
49-53: LGTM! Consistent test setup.The test setup correctly disables sanitization warnings with an identity function, consistent with other test files in the PR.
builds/knockout/helpers/jasmine.extensions.js (2)
193-200: LGTM! Proper test lifecycle management.The
beforeEachhook appropriately sets up jQuery state and disables sanitization warnings for tests.
202-204: LGTM! Good state verification.The
afterEachassertion helps catch unintended modifications to the jQuery usage flag during tests.packages/utils/src/dom/html.ts (2)
109-125: LGTM! Sanitization is correctly integrated into all parsing paths.The centralized call to
validateHTMLInputensures all three parsing strategies (template, jQuery, simple) receive sanitized HTML consistently.
128-141: LGTM! Validation function correctly returns sanitized HTML.The function now properly returns the sanitized HTML after performing size and script-tag checks. The empty string return for falsy input is a safe default.
packages/observable/src/mappingHelpers.ts (2)
29-29: LGTM: Signature change correctly handles undefined callback results.The addition of
| undefinedto the parameter type allowsmapInputCallbackto return undefined (e.g., when unwrapping observables with undefined values) without type errors. The existing guard at lines 31-34 already handles undefined correctly by returning early whenrootObjectcannot have properties.
40-40: Non-null assertion is safe but relies on control flow guards.The non-null assertion is correct because the guard at lines 31-34 ensures
rootObjectis not undefined before reaching line 40. However, this relies on understanding the control flow between the early return and the property access.The change appears to address CodeQL warnings about potential undefined access while maintaining type safety through the existing guards.
packages/computed/src/computed.ts (1)
125-126: LGTM! Removal of cross-option fallback improves predictability and fixes CodeQL warnings.The changes correctly remove the cross-option defaulting behavior between
disposeWhenNodeIsRemovedanddisposeWhen, making each disposal condition independent and explicit. Previously, settingdisposeWhenNodeIsRemovedwould override thedisposeWhenfunction to combine both conditions. Now, each option is assigned independently and the disposal logic evaluates them with OR semantics (line 343), eliminating confusing control flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/utils/src/options.ts (1)
95-108: Consider usingconsole.warninstead ofconsole.logfor the security warning.The warning about missing HTML sanitizer configuration is a security concern and should be logged at the appropriate level using
console.warnrather thanconsole.log.🔎 Proposed fix
sanitizeHtmlTemplate(html: string): string { if (!this._sanitizeWarningLogged) { - console.log('WARNING -- You don\'t have a HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate to avoid XSS vulnerabilities.') + console.warn('WARNING -- You don\'t have a HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate to avoid XSS vulnerabilities.') this._sanitizeWarningLogged = true } return html }packages/utils/src/dom/html.ts (1)
170-184: Consider the double sanitization in the parseHtmlFragment path.Line 170 validates HTML for the jQuery.html() path (line 179), but when
parseHtmlFragmentis called at line 184, it validates again (line 110 in parseHtmlFragment). This double sanitization is safe but potentially inefficient if the sanitizer is expensive.Consider refactoring to avoid redundant sanitization while maintaining security, such as:
- Creating an internal
_parseHtmlFragmentUnsafethat skips validation for use bysetHtml- Or documenting this as intentional defense-in-depth
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
builds/knockout/helpers/jasmine.extensions.js(1 hunks)packages/utils/src/dom/html.ts(2 hunks)packages/utils/src/options.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- builds/knockout/helpers/jasmine.extensions.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, off)
- GitHub Check: testheadless (22.x, on)
🔇 Additional comments (2)
packages/utils/src/dom/html.ts (2)
109-125: Excellent security improvement with sanitization integration.The restructured parsing logic correctly sanitizes HTML first via
validateHTMLInput, then uses the sanitized result across all parsing strategies. The conditional order (template tag → jQuery → simple parse) is appropriate and well-structured.
128-141: Security improvement with sanitization return path.The function now returns sanitized HTML after validation checks, enabling downstream code to use the safer result. Returning an empty string for falsy input is a safe default that prevents null/undefined edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/utils/src/options.ts (1)
95-108: LGTM! Well-designed sanitization hook with appropriate defaults.The implementation correctly provides a customizable sanitization hook with a one-time warning to alert developers. The no-op default preserves backward compatibility while encouraging security best practices.
Optional: Consider enhancing the warning message
The current warning is clear, but you might consider making it more actionable by mentioning specific libraries:
- console.warn('WARNING -- You don\'t have a HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate to avoid XSS vulnerabilities.') + console.warn('WARNING -- You don\'t have an HTML sanitizer configured. Please configure options.sanitizeHtmlTemplate with a library like DOMPurify to avoid XSS vulnerabilities.')packages/utils/spec/parseHtmlFragmentBehavior.ts (1)
134-147: LGTM! Good test coverage for setHtml sanitization.The test correctly verifies that
setHtmlrejects HTML containing script tags, mirroring the existingparseHtmlFragmenttest pattern. This ensures both parsing paths properly enforce the security policy.Optional: Consider removing debug console.log
Line 140 logs the return value of
setHtml, which is alwaysundefined. This appears to be debug code that could be removed for cleaner test output:try { let ret = setHtml(node, html) - console.log(ret) } catch (e : any) {Note: The same pattern exists on line 125 for
parseHtmlFragment, so you may want to clean up both if desired.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/utils/spec/parseHtmlFragmentBehavior.ts(2 hunks)packages/utils/src/dom/html.ts(3 hunks)packages/utils/src/options.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/utils/src/dom/html.ts (1)
packages/utils/src/options.ts (2)
jQuery(53-57)jQuery(64-75)
packages/utils/spec/parseHtmlFragmentBehavior.ts (1)
packages/utils/src/dom/html.ts (1)
setHtml(156-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, on)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (4)
packages/utils/src/dom/html.ts (3)
109-125: LGTM! Sanitization correctly applied across all parsing paths.The refactored logic ensures that
validateHTMLInputsanitizes the HTML before any parsing strategy is used. The preference order (template tag → jQuery → simple parser) is sensible and all paths use the sanitizedsaferHtml.
128-141: LGTM! Correct validation and sanitization order.The signature change from
voidtostringis appropriate, allowing the sanitized HTML to be used inline. The validation sequence is correct:
- Reject falsy input early
- Check size limits on original input
- Detect script tags on original input
- Sanitize and return
This ordering ensures that malicious content is detected before sanitization potentially masks it.
156-201: LGTM! Consistent sanitization across both setHtml code paths.The function correctly sanitizes HTML in both execution paths:
- jQuery path (line 179-180): Explicitly validates via
validateHTMLInputbefore passing tojQuery(node).html()- parseHtmlFragment path (lines 184-189): Validation happens internally within
parseHtmlFragmentThis ensures HTML is sanitized regardless of which rendering strategy is used.
packages/utils/spec/parseHtmlFragmentBehavior.ts (1)
2-2: LGTM! Correctly imports setHtml for testing.The addition of
setHtmlto the imports enables testing of the sanitization behavior in the jQuery code path.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.