feat: Enhance security by implementing least-privilege permissions and improving HTML sanitization#233
Conversation
…d improving HTML sanitization
📝 WalkthroughWalkthroughReplaces fragile regex/heuristics with safer parsers, adds crypto-backed ID generation, introduces centralized HTML sanitization (DOMPurify + parser fallback) and safe renderer, tightens domain and redirect_uri validation, redacts sensitive headers in logs, and restricts GitHub Actions workflow permissions. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SafeHtmlRenderer
participant Sanitizer
participant DOM
Client->>SafeHtmlRenderer: render(html)
SafeHtmlRenderer->>Sanitizer: sanitizeHtml(html)
Sanitizer-->>SafeHtmlRenderer: sanitizedHtml
SafeHtmlRenderer->>DOM: set innerHTML = sanitizedHtml
DOM-->>Client: content rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/uipack/src/runtime/index.ts (1)
75-93:⚠️ Potential issue | 🟡 MinorAdd
sanitizeHtmlContentto the runtime API reference.The function is exported from the public runtime barrel but not documented in
docs/frontmcp/ui/api-reference/runtime.mdx. Add it to the "Input Sanitization" section with usage examples and description, similar to the existingsanitizeInputdocumentation.libs/auth/src/jwks/jwks.utils.ts (1)
15-27:⚠️ Potential issue | 🟡 MinorAdd base64url padding before decode for browser compatibility.
Browser
atob()uses forgiving-base64 decode and rejects base64 input with length not divisible by 4 (length % 4 == 1). JWT payloads in base64url format often omit padding, causing failures in browser environments. Add padding to make length a multiple of 4.🛠️ Proposed fix
- const b64 = parts[1].replace(/-/g, '+').replace(/_/g, '/'); + let b64 = parts[1].replace(/-/g, '+').replace(/_/g, '/'); + const pad = b64.length % 4; + if (pad) b64 += '='.repeat(4 - pad);
🤖 Fix all issues with AI agents
In `@libs/auth/src/utils/www-authenticate.utils.ts`:
- Around line 224-241: The key parsing in parseQuotedParams currently uses /\w/
which misses valid RFC token characters; update the key character test to accept
the full RFC token (tchar) set (e.g. A-Za-z0-9 and ! # $ % & ' * + - . ^ _ ` |
~) so keys like "realm-1" or "token.value" are preserved; replace the while loop
condition that uses /\w/.test(input[i]) with a test against the RFC token char
class (or a small helper isTokenChar(char) used inside the loop) so the loop
advances over all valid token characters and then slice the key as before.
In `@libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts`:
- Around line 966-978: The OAuth authorize flow currently performs redirects
without enforcing http/https, causing open-redirect risk; update the production
flow to enforce protocol validation by either adding a Zod refinement on the
redirect_uri schema to only accept url strings whose parsed URL.protocol is
'http:' or 'https:' or by invoking the existing isValidRedirectUri(uri: string)
helper immediately before any call to httpRespond.redirect(...) (e.g., in the
authorize handling code paths that call httpRespond.redirect) and rejecting the
request if validation fails; also update the tests for oauth.authorize.flow to
assert the production behavior rejects javascript:, data:, vbscript: schemes
rather than only testing the local helper.
In `@libs/sdk/src/server/adapters/express.host.adapter.ts`:
- Around line 8-48: Update the docs/draft to document the new public interfaces
ExpressCorsOptions and ExpressHostAdapterOptions: replace or augment references
to the generic CorsOptions in the SDK server docs (the core server CORS section)
to explicitly list the ExpressCorsOptions fields (origin, credentials, maxAge),
document allowed origin value types, note that CORS is disabled by default and
that credentials cannot be used with origin: '*', and show how to pass an
ExpressHostAdapterOptions.cors object when constructing the adapter.
In `@libs/ui/src/universal/renderers/html.renderer.ts`:
- Around line 105-114: The decodeHtmlEntities function performs entity
replacements in the wrong order causing double-unescaping; update
decodeHtmlEntities so that the & replacement occurs before replacing <
and > (i.e., keep numeric and hex decodes, then decode '&' first, then
decode '<', '>', '"' and ''') to prevent inputs like
'&lt;script&gt;' from becoming actual tags; locate the function named
decodeHtmlEntities and reorder the .replace(...) calls accordingly.
In `@libs/uipack/src/runtime/sanitizer.ts`:
- Around line 457-501: sanitizeHtmlViaDom triggers a CodeQL DOM-text-as-HTML
finding and Biome flags implicit returns in forEach callbacks; add a CodeQL
suppression comment/justification above the DOMParser.parseFromString call
referencing the CodeQL report query id (e.g. // CodeQL suppression:
<CODEQL_QUERY_ID> - justified because input is sanitized by this function) and
rewrite all arrow forEach callbacks to use block bodies with explicit statements
(e.g. elements.forEach(el => { el.remove(); }); and attrsToRemove.forEach(name
=> { el.removeAttribute(name); });) so no implicit return values are used;
reference function sanitizeHtmlViaDom and constants HTML_DANGEROUS_TAGS,
HTML_EVENT_HANDLERS, and HTML_DANGEROUS_SCHEMES in your change.
🧹 Nitpick comments (4)
libs/ui/src/bridge/adapters/gemini.adapter.ts (1)
12-26: Consider extracting shared domain validation utility.The validation logic is identical to
isValidClaudeDomaininclaude.adapter.ts. A shared helper could reduce duplication:// Potential shared utility in base-adapter.ts or a utils file function isValidDomain(hostname: string, allowedDomains: string[]): boolean { const lowerHost = hostname.toLowerCase(); return allowedDomains.some(domain => lowerHost === domain || lowerHost.endsWith('.' + domain) ); }This is a minor improvement and can be deferred since the current implementation is correct and secure.
libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts (1)
985-999: Test names imply flow behavior verification, but only test local helper.The test name "should identify javascript: URI as malicious" suggests the OAuth flow identifies these URIs as malicious, but the test only validates the local helper. Consider renaming to clarify scope (e.g., "isValidRedirectUri helper should reject javascript: URIs") or, better yet, add flow-level tests that verify the actual authorize endpoint behavior.
Example: Add flow-level test for malicious URIs
it('should return error page for javascript: redirect_uri', async () => { const scope = flowScenarios.orchestratedLocal(); const metadata = createFlowMetadata(); const params = createValidOAuthRequest({ redirect_uri: 'javascript:alert(1)' }); const input = createOAuthInput(params); const flow = new OauthAuthorizeFlow(metadata, input, scope, jest.fn(), new Map()); const { output } = await runFlowStages(flow, ['parseInput', 'validateInput']); expectOAuthHtmlPage(output, { status: 400, contains: ['Authorization Error'], }); });libs/ui/src/universal/renderers/html.renderer.ts (2)
77-77: Consider addingsvgandmathto dangerous tags.SVG and MathML elements can contain executable content (e.g.,
<svg onload="...">,<svg><script>...</script></svg>, or<math><maction actiontype="statusline">with event handlers). The current attribute filtering catcheson*handlers, but allowing these elements could still pose risks via nested dangerous content or browser-specific vectors.♻️ Proposed enhancement
-const DANGEROUS_TAGS = ['script', 'style', 'iframe', 'object', 'embed', 'applet', 'base']; +const DANGEROUS_TAGS = ['script', 'style', 'iframe', 'object', 'embed', 'applet', 'base', 'svg', 'math', 'template'];Note: If SVG support is needed, consider using a proper allowlist approach or DOMPurify as suggested in line 89.
306-317: Redundant event handler check.Lines 314-317 are unreachable since all event handlers start with
onand are already blocked by the check on lines 310-312.🧹 Proposed cleanup
function isAttributeSafe(name: string, value: string): boolean { const nameLower = name.toLowerCase(); // Block event handlers (including variations) if (nameLower.startsWith('on')) { return false; } - - // Block known event handlers with entity encoding - if (EVENT_HANDLERS.some(h => nameLower === h)) { - return false; - } // Check URL attributes for dangerous schemes
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-02-04T21:32:34.121Z |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui/src/components/card.ts (1)
25-66:⚠️ Potential issue | 🟡 MinorAdd the
sanitizeoption to the CardOptions API Reference in the documentation.The
sanitize?: booleanoption added toCardOptionsis missing from the API Reference table indocs/frontmcp/ui/components/card.mdx(lines 201-214). Add a row to the CardOptions table documenting the newsanitizeoption and its behavior:| `sanitize` | `boolean` | `false` | Sanitizes HTML to prevent XSS (removes script tags, event handlers, dangerous attributes) |
🤖 Fix all issues with AI agents
In `@libs/ui/src/universal/renderers/html.renderer.ts`:
- Around line 294-314: The closing-tag branch in sanitizeTagAttributes returns a
closing tag without the trailing '>' because closeBracket is computed
incorrectly; change the logic so that closing tags include '>' (e.g., compute
closeBracket = tagStr.startsWith('</') ? '>' : '') or simply return
`</${tagName}>` in the closing-tag branch; keep parseAndSanitizeAttributes usage
and existing selfClose handling intact.
- Around line 164-170: The new public APIs sanitizeHtml and safeHtmlRenderer
need documentation added; confirm the correct docs directory with the team
(since docs/draft/docs/ is absent) and create corresponding docs pages there
describing usage, parameters and examples for sanitizeHtml and safeHtmlRenderer;
ensure the new docs are referenced in the project sidebar/navigation and that
any public export lists (e.g., barrel files or package README) include entries
for sanitizeHtml and safeHtmlRenderer so consumers can find them.
In `@libs/uipack/src/runtime/sanitizer.ts`:
- Around line 401-406: Add a new API doc entry for the runtime export
sanitizeHtmlContent in the same runtime API reference that already documents
sanitizeInput and sanitizeCSPDomains: describe the function signature,
parameters, return type, simple usage example, and the intended purpose
(sanitizing HTML content and removing unsafe attributes/tags such as event
handlers), and include a short security note about remaining
restrictions/assumptions; reference the exact export name sanitizeHtmlContent so
readers can find the runtime function.
- Around line 514-522: Add a new test file that exercises sanitizeHtmlContent
and its helpers: write tests that (1) simulate a browser environment by mocking
global DOMParser and document to exercise sanitizeHtmlViaDom and assert
dangerous tags/attributes are removed and safe text is preserved; (2) simulate a
non-browser environment by deleting or undefined-ing DOMParser/document to force
sanitizeHtmlViaParser and assert the same sanitization outcomes; (3) include
edge cases and invalid inputs (empty string, null/undefined cast to string,
malformed HTML, long input) and verify proper escaping/neutralization of XSS
payloads; and (4) assert expected outputs for both paths so coverage hits 95%+
across statements, branches, functions, and lines, referencing
sanitizeHtmlContent, sanitizeHtmlViaDom, and sanitizeHtmlViaParser in the tests.
… DOMPurify and validating redirect URIs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/auth/flows/oauth.authorize.flow.ts (1)
87-121:⚠️ Potential issue | 🔴 CriticalHarden error redirects to use the same scheme allowlist.
safeRedirectUriSchemaprotects the happy path, butrespondWithErrorstill redirects using the rawredirect_uri. Ajavascript:/data:URI will parse and be redirected even when validation fails, leaving an open-redirect/XSS vector on error paths. Reuse the allowlist check before any redirect inrespondWithError, and fall back to the HTML error page if it fails.🔒 Suggested fix (apply in respondWithError)
- if (redirectUri) { - try { - const url = new URL(redirectUri); - url.searchParams.set('error', 'invalid_request'); - url.searchParams.set('error_description', errorDescription); - if (state) { - url.searchParams.set('state', state); - } - this.respond(httpRespond.redirect(url.toString())); - return; - } catch { - // Invalid redirect_uri, fall through to error page - } - } + if (redirectUri) { + const safe = safeRedirectUriSchema.safeParse(redirectUri); + if (safe.success) { + const url = new URL(safe.data); + url.searchParams.set('error', 'invalid_request'); + url.searchParams.set('error_description', errorDescription); + if (state) { + url.searchParams.set('state', state); + } + this.respond(httpRespond.redirect(url.toString())); + return; + } + // Unsafe redirect_uri, fall through to error page + }
🤖 Fix all issues with AI agents
In `@libs/ui/src/universal/renderers/html.renderer.ts`:
- Around line 154-205: The parser currently rejects hyphens in custom element
names; update parseTag and sanitizeTagAttributes to allow '-' in tag names by
expanding the tag-name character class from /[a-zA-Z0-9]/ to include the hyphen
(e.g., /[a-zA-Z0-9-]/) and update the opening regex in sanitizeTagAttributes
from /^<\/?([a-zA-Z0-9]+)/ to accept hyphens as well (e.g.,
/^<\/?([a-zA-Z0-9-]+)/) so tags like <my-widget> are parsed and matched
correctly; no other logic changes required (isClosing/isSelfClosing handling
remains the same).
In `@libs/uipack/src/runtime/sanitizer.ts`:
- Around line 499-507: The fallback sanitizer currently only blocks a small tag
list (HTML_DANGEROUS_TAGS) which misses SVG/MathML vectors; update the non-DOM
fallback to treat SVG and MathML namespaces/tags as dangerous by adding common
SVG/MathML root and element names (e.g., svg, foreignObject, math, mrow, path,
circle, etc.) to HTML_DANGEROUS_TAGS or by introducing a separate
HTML_DANGEROUS_NAMESPACES set and ensuring the parsing path that uses
HTML_DANGEROUS_TAGS checks those names/namespaces as well; ensure the same
blocking logic that removes script/style/iframe also removes any element
matching the added SVG/MathML names during the fallback sanitizer in
sanitizer.ts so the non-DOMPurify path does not allow SVG/MathML XSS payloads.
🧹 Nitpick comments (1)
libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts (1)
964-1025: Prefer flow-level assertions over a duplicated helper.The new helper mirrors production logic, but it doesn’t verify the actual flow behavior. To avoid drift, add test cases that run the flow with
javascript:,data:, andvbscript:redirect URIs and assert an error response (HTML 400).🧪 Example test addition
+ it.each([ + 'javascript:alert(1)', + 'data:text/html,<script>alert(1)</script>', + 'vbscript:msgbox(1)', + ])('should reject unsafe redirect_uri scheme: %s', async (redirect_uri) => { + const scope = flowScenarios.orchestratedLocal(); + const metadata = createFlowMetadata(); + const params = createValidOAuthRequest({ redirect_uri }); + const input = createOAuthInput(params); + + const flow = new OauthAuthorizeFlow(metadata, input, scope, jest.fn(), new Map()); + const { output } = await runFlowStages(flow, ['parseInput', 'validateInput']); + + expectOAuthHtmlPage(output, { status: 400 }); + });
…ctUriSchema to prevent open-redirect/XSS vulnerabilities
…D validation failures and improving tag name parsing for custom elements
…id-cols classes and re-exporting sanitizeHtml for backward compatibility
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)
libs/ui/src/components/form.ts (1)
44-106:⚠️ Potential issue | 🟠 MajorAdd
sanitizeoption to input API documentation.The new
sanitizeoption inInputOptionsis not documented in the input API reference table at docs/frontmcp/ui/components/form.mdx (lines 355-368). Update the table to include:|
sanitize|boolean|false| Sanitize icon HTML content to prevent XSS |This is a public API addition to the
@frontmcp/uiSDK and must be documented.
…ementing DoS prevention for HTML sanitization
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/uipack/src/runtime/sanitizer.ts`:
- Around line 548-564: The loop that handles tags currently pushes the raw '<'
into result when findTagEnd(html, i) returns -1 or parseTagContent(tagContent)
returns falsy; change this to emit an escaped '<' (e.g., '<') instead of the
raw character so malformed tags are rendered safely. Update the branches in the
while loop that check tagEnd === -1 and !tagInfo to push '<' (or the
appropriate HTML-escaped sequence) to result and advance i accordingly,
referencing the existing variables and functions findTagEnd, parseTagContent,
html, result, and i.
- Line 38: Replace the direct DOMPurify import with the isomorphic-dompurify
entry so module initialization won't fail in Node/SSR; specifically update the
import at the top of sanitizer.ts (where DOMPurify is currently imported) to use
isomorphic-dompurify and keep all references to DOMPurify.sanitize unchanged
(e.g., in functions that check for DOM availability), ensuring the sanitizer
module loads safely in both browser and server environments.
🧹 Nitpick comments (1)
libs/uipack/src/runtime/sanitizer.ts (1)
723-729: Consider treatingsrcsetas a URL attribute.
srcsetcan carry URL lists and should be scheme‑checked likesrc/hreffor consistency.♻️ Suggested tweak
- if (['href', 'src', 'action', 'formaction', 'data', 'poster', 'codebase'].includes(nameLower)) { + if (['href', 'src', 'srcset', 'action', 'formaction', 'data', 'poster', 'codebase'].includes(nameLower)) {
1 participant
Summary by CodeRabbit
New Features
Security