Skip to content

chore(security): clear the code-scanning queue (easy wins + investigations)#182

Merged
arediss merged 5 commits intomainfrom
chore/security-easy-wins
May 4, 2026
Merged

chore(security): clear the code-scanning queue (easy wins + investigations)#182
arediss merged 5 commits intomainfrom
chore/security-easy-wins

Conversation

@arediss
Copy link
Copy Markdown
Owner

@arediss arediss commented May 4, 2026

Summary

Closes the easy-win cluster of the code-scanning queue (Phase 0 of the platform-evolution master plan). Goal: reach a clean baseline before larger refactors land, so future PRs' `+N -M alerts` deltas are readable signals instead of noise.

State before: 30 open code-scanning alerts + 1 Dependabot.
Addressed in this PR: 11 CodeQL alerts + 1 Dependabot (uuid).
Will remain after merge: 17 Trivy CVEs in the Docker base image / bundled npm CLI (separate PR — rebuild on `node:22-alpine` + `apk upgrade zlib`).

Per-commit breakdown

`0086156` — bump uuid to >=14.0.0 via override

Closes Dependabot alert #24 (uuid <14.0.0 missing buffer bounds check in v3/v5/v6). Transitive via `resend → svix`, pinned via root `overrides`. Lock fully regenerated to dedupe.

`27c6130` — close low-hanging CodeQL alerts

  • `js/log-injection` (`engine.ts:44`) — strip CR/LF in the PluginEngine `log()` helper so a plugin id with embedded newlines from `request.params` can't splice fake entries. Sanitized at the helper, not at every call site.
  • `js/missing-rate-limiting` (`POST /install`) — cap at 5/min. Most expensive route in the router (outbound fetch + tarball extract).
  • `js/unused-local-variable` ×5 — dead code removed:
    • `AccountSection.tsx` — `saving` (loop-local, unused)
    • `LogsTab.tsx` — outer `levelColors` shadowed by an identical inner declaration
    • `media.ts` — `normalizeEpisodeInfo` helper, never wired up
    • `RequestsPage.tsx` — `total` state, written but never read
    • `MediaDetailPage.tsx` — default `api` import, only named re-exports used

`07a0f51` — close 4 remaining CodeQL alerts in code

  • `js/request-forgery` ×2 (`plugins/routes.ts:153,156`) — re-validate the `repository` identifier against the same regex as the route schema at the top of `resolveInstallUrlFromRepo`. Doubles as a CodeQL taint cleanse and a real safety net against future callers that bypass the schema.
  • `js/clear-text-storage-of-sensitive-data` (`InstallPage.tsx:332`) — stop writing the install setup secret to `sessionStorage`. Hold it only in `api.ts`'s module-local var via `setSetupSecret()`. Page refresh during install loses the secret (acceptable — re-enter on the gate). Cleared on wizard exit (manual button + countdown auto-redirect).
  • `js/http-to-file-access` (`backupService.ts:172`) — inline `// codeql[js/http-to-file-access]` suppression with rationale pointing to the 7-step trust chain already documented above the function (RBAC, CSRF, rate-limit, password re-auth, version check, SQLite magic-header, HMAC). `dbPath` is a config constant, not user input.
  • `jwt-token` ×2 (`dist/server.js{,.map}`) — `TMDB_DEFAULT_TOKEN` is a public read-only TMDB v4 token (`api_read` scope), not a secret. Dismissed in the GitHub UI with rationale (alerts Hover effects don't trigger on macOS Edge/Chrome trackpad cursor movement #172, qBittorrent connector plugin #173). Source comment expanded; startup warning added when the fallback is in use to nudge admins toward setting their own `TMDB_API_TOKEN` (mitigates rate-limit risk if the shared token hits TMDB's per-IP throttle).

Test plan

  • `tsc --noEmit` passes on backend + frontend.
  • `npm run dev` boots cleanly (verify no startup regression, including the new TMDB warning when `TMDB_API_TOKEN` is unset).
  • Manual smoke: install a plugin via UI → confirms `POST /install` still works under the 5/min rate limit and that the regex re-validation doesn't reject legit `owner/repo` identifiers.
  • Manual smoke: re-run the install wizard from a fresh DB → confirms the in-memory setup secret flow works (gate → wizard → completion). A mid-wizard tab refresh should send back to the gate.
  • Manual smoke: backup restore still succeeds (the `codeql` suppression doesn't change runtime behavior, but worth verifying after the `writeFileSync` line is touched).
  • After merge: confirm the code-scanning queue drops to 17 Trivy CVEs only once CodeQL re-runs (the 9 in-code fixes auto-close on next scan, the 2 JWT are already dismissed).

Out of scope

Trivy CVEs in the Docker image (`zlib` Alpine + `cross-spawn`/`tar`/`minimatch`/`glob`/`brace-expansion`/`diff` bundled inside the npm CLI of `node:20-alpine`). Will be addressed by rebuilding on `node:22-alpine` + `apk upgrade zlib`.

arediss added 5 commits May 4, 2026 18:36
PRAGMA journal_mode returns the resulting mode as a row, which
$executeRawUnsafe rejects, surfacing as a noisy startup warning.
$queryRawUnsafe accepts the row and the WAL setting actually applies.
…akeover

Plugin runtime:
- Expose the full react-dom namespace via __OSCARR_REACT_DOM__ (was
  react-dom/client only) so plugins can import { createPortal, flushSync }.
  Vidstack-based players (jellyfin-watch) and any plugin needing portals
  for fullscreen overlays were broken without this.
- Extend the React shim's named exports to cover isValidElement,
  cloneElement, StrictMode, Profiler, version. Vidstack, Radix and
  Headless UI all reach for these at module-eval time.
- Document the CSS scope rewrite and the createPortal escape (portaled
  subtrees must re-apply data-oscarr-plugin="<id>" or Tailwind utilities
  silently stop matching).

Service worker:
- workbox skipWaiting + clientsClaim so a new SW takes control on next
  navigation instead of waiting for every tab to close.
- main.tsx listens for controllerchange and reloads the page once when
  a new SW takes over, so a deploy lands in every open tab without a
  hard refresh. The hadController guard suppresses the spurious reload
  on first install.
Closes Dependabot alert #24 — uuid <14.0.0 is missing a buffer bounds
check in v3/v5/v6 when buf is provided (CVE / GHSA pending).

We don't depend on uuid directly; it lands transitively through
resend → svix. Pin via npm overrides so the fix is durable until
upstream svix bumps. Lock regenerated to dedupe the new version
across the tree.
Address 7 CodeQL alerts in one sweep:

- js/log-injection (engine.ts:44) — strip CR/LF in the PluginEngine log
  helper so a plugin id like 'foo\n[INFO] forged' coming from
  request.params can't splice fake entries into the log. Sanitizing at
  the helper covers every call site in one place.
- js/missing-rate-limiting (routes.ts:/install) — cap plugin install at
  5/min. Each call triggers an outbound fetch + tarball extraction, so
  this is the most expensive path in the router and the right place
  to cap if an admin token leaks.
- js/unused-local-variable ×5 — delete dead code:
  - account/sections/AccountSection.tsx — 'saving' (local in a tile loop)
  - admin/LogsTab.tsx — outer 'levelColors' shadowed by an identical
    inner declaration
  - routes/media.ts — 'normalizeEpisodeInfo' helper, never wired up
    (parseEpisodeInfo handles the same shape inline)
  - pages/RequestsPage.tsx — 'total' state, written but never read
  - pages/MediaDetailPage.tsx — default 'api' import, only the named
    re-exports posterUrl/backdropUrl are used
- js/request-forgery (plugins/routes.ts) — re-validate the repository
  identifier against the same regex as the route schema at the top of
  resolveInstallUrlFromRepo, so any future caller (or a refactor that
  bypasses the schema) can't splice ':// .. ? #' segments into the
  github.com URL we build. Also doubles as a CodeQL taint cleanse.
- js/clear-text-storage-of-sensitive-data (InstallPage) — stop writing
  the install setup secret to sessionStorage. Hold it only in api.ts's
  module-local var via setSetupSecret(), exposed from the api module.
  The interceptor reads the in-memory value when calling /setup/*. A
  refresh during install loses the secret (acceptable trade — re-enter
  on the gate). Cleared on wizard exit (manual + countdown auto-redirect).
- js/http-to-file-access (backupService) — codeql[…] suppression with
  rationale pointing to the 7-step trust chain already documented above
  the function (RBAC, CSRF, rate-limit, password re-auth, version,
  SQLite magic-header, HMAC). dbPath is a config constant, not user
  input — no path traversal possible.
- jwt-token (×2 alerts on dist/server.js{,.map}) — TMDB_DEFAULT_TOKEN
  is a public read-only TMDB v4 token, dismissed in the GitHub UI with
  rationale. Source comment expanded to clarify; startup warning added
  when the fallback is in use to nudge admins toward setting their own
  TMDB_API_TOKEN (mitigates rate-limit/ban risk if the shared token
  hits TMDB's per-IP throttle).
} else {
const fn = level === 'error' ? console.error : level === 'warn' ? console.warn : console.log;
fn('[PluginEngine]', String(msg));
fn('[PluginEngine]', sanitized);
@arediss arediss merged commit 068b5d2 into main May 4, 2026
6 checks passed
arediss added a commit that referenced this pull request May 4, 2026
PR #182 regenerated package-lock.json on macOS arm64, which pruned the
14 non-darwin rolldown native bindings (linux-x64-musl, linux-x64-gnu,
win32-x64-msvc, etc.). The Docker build on Alpine amd64 then crashes
with 'Cannot find module ./rolldown-binding.linux-x64-musl.node'.

Restore the pre-PR lockfile (which had all 15 rolldown bindings) and
re-apply ONLY the uuid >=14.0.0 override surgically by patching the
node_modules/uuid entry directly. This keeps the Dependabot fix without
losing any other platform's native deps.

Verified by running 'npm ci' in a node:20-alpine amd64 container —
binding-linux-x64-musl is now resolved.
@arediss arediss deleted the chore/security-easy-wins branch May 8, 2026 17:19
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