Skip to content

🛡️ Sentinel: Security hardening - #5241

Merged
diegosouzapw merged 6 commits into
diegosouzapw:release/v3.8.40from
iamedwardngo:main
Jun 28, 2026
Merged

diegosouzapw merged 6 commits into
diegosouzapw:release/v3.8.40from
iamedwardngo:main

Conversation

@iamedwardngo

Copy link
Copy Markdown

Summary

  • Describe the user-facing or operational change.

Related Issues

  • Closes #
  • Related to #

Validation

  • npm run lint
  • npm run test:unit
  • npm run test:coverage
  • Coverage is still >= 60% for statements, lines, functions, and branches
  • SonarQube PR analysis is green or any remaining issues are explicitly documented below

Tests Added Or Updated

  • List every changed or added automated test file.
  • If no production code changed, state that here.

Coverage Notes

  • If this PR changes src/, open-sse/, electron/, or bin/, explain which tests cover the change.
  • If coverage moved down in any touched file, explain why and what follow-up task will recover it.

Reviewer Notes

  • Call out any risky areas, migrations, feature flags, or manual validation that reviewers should know about.

diegosouzapw and others added 4 commits June 28, 2026 10:23
…uzapw#5228)

apt-get upgrade -y in the base stage pulls security-patched trixie
packages at build time, and npm install -g npm@latest refreshes the
globally-bundled undici/tar inside the npm CLI. Together these clear the
subset of GitHub container-scan CVE alerts that have an upstream fix
available.

None of the flagged CVEs are in the application dependency tree (app
already resolves undici@8.5.0 / tar@7.5.16, both fixed); they live in
the node:24-trixie-slim base layer and npm's own internals, and none are
reachable from the proxy request surface at runtime. CVEs without a
published fix (local-only TOCTOU, etc.) remain until the distro patches
them and the image is rebuilt.
…se) (diegosouzapw#5234)

The advisory Trivy image scan uploaded every HIGH/CRITICAL into the
Security tab without ignore-unfixed, flooding it with ~150 unfixable
base-image OS CVEs (Debian trixie packages with no upstream patch,
overwhelmingly local-only and not reachable from the proxy request
surface). Operators cannot act on those, so they are pure noise.

Add ignore-unfixed:true to the advisory step so it mirrors the existing
CRITICAL blocking gate and surfaces only actionable, fixable
vulnerabilities. Wire trivyignores to a new repo-root .trivyignore that
documents the accepted-risk policy and is the single auditable home for
the rare fixable CVE we must temporarily accept (none at present).

Takes effect on the next release image build (Trivy only runs on tag
builds, not main pushes); fixed CVEs drop out of the SARIF and GitHub
auto-resolves the corresponding alerts.
- Implement server-side HTML sanitization for documentation content.
- Harden path traversal prevention in i18n docs fallback logic.
- Add unit tests for XSS and path traversal protection.

Co-authored-by: iamedwardngo <129360513+iamedwardngo@users.noreply.github.com>
…ing-2394592656372058115

🛡️ Sentinel: Security hardening

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces server-side HTML sanitization for documentation rendering using DOMPurify and JSDOM, along with corresponding unit tests for sanitization and path traversal regexes. The review feedback highlights a security risk in the sanitizer configuration where allowing <input> tags and associated attributes could lead to phishing or UI redressing, as well as the inclusion of non-standard HTML tags. Additionally, the path traversal tests incorrectly validate local regex definitions rather than production code, violating the repository style guide which mandates using Zod schemas from src/shared/validation/schemas.ts for input validation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/lib/docsSanitizer.ts Outdated
Comment on lines +25 to +34
ALLOWED_TAGS: [
"h1", "h2", "h3", "h4", "h5", "h6", "blockquote", "p", "a", "ul", "ol",
"nl", "li", "ins", "del", "te", "bw", "em", "strong", "span", "hr", "br",
"div", "table", "thead", "caption", "tbody", "tr", "th", "td", "pre",
"code", "img", "details", "summary", "input"
],
ALLOWED_ATTR: [
"href", "name", "target", "src", "alt", "title", "class", "id", "type",
"checked", "disabled", "rel"
],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

Allowing the <input> tag along with attributes like type, name, checked, and disabled introduces a potential security risk (Stored XSS / Phishing / UI Redressing). An attacker could inject malicious input fields (e.g., text or password inputs) to harvest credentials or mislead users. Additionally, "nl", "te", and "bw" are not standard HTML tags and appear to be typos.

Recommendations:

  1. Remove "input" from ALLOWED_TAGS and "type", "checked", "disabled" from ALLOWED_ATTR if they are not strictly required.
  2. If task lists (checkboxes) are required, use a DOMPurify hook (e.g., uponSanitizeElement) to strictly enforce that any <input> element has type="checkbox" and disabled attributes, stripping any other input types.
  3. Remove the non-standard tags "nl", "te", and "bw".
    ALLOWED_TAGS: [
      "h1", "h2", "h3", "h4", "h5", "h6", "blockquote", "p", "a", "ul", "ol",
      "li", "ins", "del", "em", "strong", "span", "hr", "br",
      "div", "table", "thead", "caption", "tbody", "tr", "th", "td", "pre",
      "code", "img", "details", "summary"
    ],
    ALLOWED_ATTR: [
      "href", "name", "target", "src", "alt", "title", "class", "id", "rel"
    ],

Comment on lines +9 to +10
const LOCALE_REGEX = /^[a-z0-9-]+$/i;
const SLUG_REGEX = /^[a-z0-9-]+$/i;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There are two issues with this implementation:

  1. Testing Local Definitions: The test defines LOCALE_REGEX and SLUG_REGEX locally within the test file. This means the test is only validating its own local regexes rather than the actual regexes used in the production code (e.g., in the page component), which defeats the purpose of unit testing.
  2. Style Guide Violation: According to the Repository Style Guide (Rule 8), all inputs must be validated using Zod schemas from src/shared/validation/schemas.ts. Instead of using custom regexes, you should define and export Zod schemas for the locale and slug in src/shared/validation/schemas.ts, and import them here and in the production code for validation.
References
  1. Always validate inputs with Zod schemas from src/shared/validation/schemas.ts.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.40 June 28, 2026 17:45
diegosouzapw and others added 2 commits June 28, 2026 14:45
…XSS)

Review fixes on top of the Sentinel hardening so it builds and the tests guard
the real code paths:

- Declare `dompurify` as a direct dependency (was imported but missing from
  package.json → build/test would fail with module-not-found) + add it to the
  dependency-allowlist. Pinned to the existing `overrides` (^3.4.11).
- docsSanitizer: drop `USE_PROFILES` (when set, DOMPurify ignores ALLOWED_TAGS
  entirely) so the explicit curated allowlist actually governs; remove the bogus
  `nl`/`te`/`bw` non-tags; version-agnostic instance type.
- Extract the path-traversal guard into a pure, exported
  `resolveSafeI18nSectionDir` (src/lib/docsI18nPath.ts) used by the docs page,
  and fix the containment check (`startsWith(i18nRoot + path.sep)` so a sibling
  like `…/i18n-evil` cannot pass). The locale comes from a user-controllable
  cookie, so this is the real fix.
- Rewrite docs-path-traversal.test.ts to exercise the REAL exported helper
  (it previously only tested a private copy of the regex). 6/6 security tests
  pass; typecheck + lint + deps gate clean.
- Remove the `.jules/sentinel.md` agent scratch note (not repo content).

Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
@diegosouzapw
diegosouzapw merged commit 4c4dcbd into diegosouzapw:release/v3.8.40 Jun 28, 2026
7 checks passed
@diegosouzapw diegosouzapw mentioned this pull request Jun 29, 2026
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
Harden docs i18n rendering: path-traversal guard (cookie-controlled locale confined to docs/i18n via pure resolveSafeI18nSectionDir) + markdown XSS sanitization (DOMPurify allowlist). Review fixes: declared dompurify dep + allowlist, cleaned sanitizer config, extracted+tested the real path helper, removed agent scratch. Thanks @iamedwardngo!
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.

2 participants