fix(viewer): add request guards, close a path escape, and patch dependencies

[Murali Chillakuru (mchillakuru)]

Summary

The viewer serves evaluation artifacts — prompts, model outputs, judge reasoning — with
no authentication. Binding to localhost is not itself a control: any page the operator
visits can issue requests to http://localhost:<port>, and an attacker-controlled
hostname can be pointed at 127.0.0.1 to defeat the browser's origin checks.

hooks.server.ts adds a Host header allow-list (which is what actually stops DNS
rebinding), an Origin check, and an optional bearer token via ASSERT_VIEWER_TOKEN
compared without early exit. The token is optional so existing local workflows keep
working; the Host and Origin guards always apply.

resolveArtifactPath compared lexically resolved paths, which handles .. but is
blind to symlinks — a link inside the artifacts root pointing outside resolves to a
path that still looks contained, so its target was served. It now compares real paths,
resolving the root too since it may sit under a symlinked parent.

npm audit reported a high-severity path traversal in postcss and three moderate
SvelteKit advisories. Both are resolved; npm audit now reports 0 vulnerabilities.

Closes: SEC-001 (viewer served with no authentication, Critical) · SEC-012 (symlink path escape) · SEC-004 (vulnerable frontend dependencies)

Commits

• fix(deps): resolve viewer dependency vulnerabilities
• fix(viewer): add request guards and close the symlink path escape

Testing

pytest tests/ -q

Full suite green on this branch; 5 files changed, 280 insertions(+), 15 deletions(-). Every change has a regression test, and the
suite was re-run after each commit rather than only at the end.

Notes for reviewer

The path-escape regression test was confirmed to fail with the fix reverted. It uses a
directory junction on Windows, where symlinks need elevation, so the assertion runs
there rather than being skipped on the platform where the lexical check is easiest to
get wrong.

postcss is a transitive dependency of vite, so npm audit fix could not move it; an
overrides entry pins it instead of using --force, which would have bumped majors.

svelte-check reports 0 errors.

Risk and rollback

Each commit is a single concern and can be reverted independently. See the notes above
for anything that does not revert cleanly.

[Chang Liu (changliu2)]

This is the right shape of fix — Host allow-list is genuinely the control that stops rebinding, and moving containment to realpath is the correct way to close the symlink escape. Four things need to change before merge, three of which undercut the security claims in the description.

1. The lockfile now points three packages at an internal Azure DevOps feed with SHA-1 integrity. @sveltejs/kit, nanoid, and postcss were rewritten from registry.npmjs.org + sha512- to ms-feed-25.pkgs.visualstudio.com/1es-public/... + sha1-. The other 110 entries in this lockfile still use npmjs.org/sha512. build-viewer.yml, deploy-pages.yml, and regression.yml all run npm ci against this committed lockfile, so public CI and outside contributors will resolve those three tarballs from an internal mirror, verified with a weaker digest that can't be cross-checked against the public registry. This looks like an artifact of the author's local .npmrc, not an intended change. Please regenerate the lockfile against the public registry.

2. @sveltejs/kit@2.70.1 is still vulnerable. GHSA-29g2-3rmr-qm68 (ReDoS, unauthenticated DoS via the Accept header) is first patched in 2.70.2; the GitHub advisory API confirms 2.70.1 is still in range. So "npm audit now reports 0 vulnerabilities" is stale. Additionally package.json still declares "@sveltejs/kit": "^2.58.0", so nothing but the lockfile carries the fix. (The postcss side is correct: GHSA-r28c-9q8g-f849 is first patched in 8.5.18, and the ^8.5.18 override resolving to 8.5.22 covers it. Worth noting in the description that both postcss and kit are dev: true here — build-time, not runtime, exposure.)

3. There is no test for hooks.server.ts at all. The description says "Every change has a regression test," but the only new test covers resolveArtifactPath. SEC-001 is the Critical item in this PR and its three guards — Host rejection, Origin mismatch, wrong/absent bearer token — have zero coverage. tests/node_runner.py already gives you the harness pattern; a handful of synthetic Requests through handle would cover it.

4. The symlink fix is scoped to one route. resolveArtifactPath is only used by /api/download/[...path]. Every other artifact read — suiteDirPath/runDirPath in data.ts, run-status.ts, run-spawn.ts, api/csv/*, api/suites/[suite_id]/config, the run/suite page loaders — builds paths with path.join after an isSafeArtifactId regex check, which stops .. but is exactly as blind to symlinks as the code being fixed here. Under the same threat model as SEC-012 (a link planted inside the artifacts root), a suite or run directory that is a symlink to outside the root is still followed. Either route those through a shared realpath containment check or state explicitly in the PR body why they're out of scope.

Non-blocking notes below.

Inline notes

viewer/package-lock.json:459, 1416, 1469 — Must fix. resolved rewritten to an internal ADO feed URL and integrity downgraded from sha512- to sha1-, only for the three entries this PR touched. Fix: regenerate the lockfile with the public registry configured (npm install --registry=https://registry.npmjs.org) so resolved/integrity match the other 110 entries.

viewer/package.json:35 / lockfile @sveltejs/kit — Must fix. 2.70.1 is still affected by GHSA-29g2-3rmr-qm68 (patched 2.70.2); the devDependency range is untouched at ^2.58.0. Fix: bump to >=2.70.2 in both package.json and the lock, then re-run npm audit.

viewer/src/hooks.server.ts (new file, whole file) — Must fix. No regression test exercises any of the three guards. Fix: add a test asserting 403 on a non-loopback Host, 403 on mismatched Origin, 401 on a wrong bearer token, and 200 on the correct one with ASSERT_VIEWER_TOKEN set.

viewer/src/lib/server/artifacts.ts:536-543 (suiteDirPath / runDirPath) — Must fix (or explicitly scope out). The realpath containment added below is not applied here, and these are the functions behind almost all artifact reads. Fix: factor the realpath containment out of resolveArtifactPath and apply it to the directories these return.

viewer/src/hooks.server.ts — const supplied = presented || cookie || event.url.searchParams.get('token') — Nice to have. Accepting the token as a URL query parameter puts a credential into server access logs, shell history, and Referer headers on any outbound link from the viewer. Fix: drop the searchParams fallback, or restrict it to a one-shot exchange that sets the cookie and redirects.

viewer/src/lib/server/artifacts.ts:~570 (catch around fs.realpathSync(resolvedPath)) — Nice to have. The comment says "path does not exist yet," but the catch swallows every error (EACCES, ELOOP, ENOTDIR) and falls back to the unverified lexical path. Fix: rethrow unless err.code === 'ENOENT'.

viewer/src/hooks.server.ts — ASSERT_VIEWER_TOKEN / ASSERT_VIEWER_ALLOWED_HOSTS — Nice to have. Neither variable appears anywhere in CONFIG_REFERENCE.md, README.md, or the run-assert-eval prompt. Anyone reaching the viewer through a Codespaces/devcontainer forwarded hostname now gets a blanket 403 with no discoverable escape hatch. Fix: document both variables and mention the forwarded-host case in the 403 message.

viewer/src/hooks.server.ts — Origin block — Nice to have. The doc comment says the Origin check catches "simple requests," but browsers omit Origin entirely on <img>/<script>/top-level GET navigations, so those pass both guards (reads are still blocked by CORS, so this is a comment-accuracy point, not a hole). Fix: reword, and optionally reject Sec-Fetch-Site: cross-site.

Verdict: Request Changes — a lockfile that silently redirects three packages to an internal feed with SHA-1 integrity, a dependency bump that lands one patch short of the advisory fix, and zero tests on the Critical auth guard are each individually enough to block a PR whose entire purpose is security.

Must fix before merge

1. Lockfile: internal ADO feed URLs + SHA-1 integrity on @sveltejs/kit, nanoid, postcss.
2. @sveltejs/kit 2.70.1 still vulnerable (GHSA-29g2-3rmr-qm68, patched 2.70.2); package.json range not bumped.
3. No regression test for hooks.server.ts — the Critical SEC-001 fix is untested.
4. Symlink containment applied only to resolveArtifactPath; suiteDirPath/runDirPath (all other read paths) still follow symlinks.

Nice to have

• Drop ?token= query-parameter auth.
• Narrow the realpathSync catch to ENOENT.
• Document ASSERT_VIEWER_TOKEN / ASSERT_VIEWER_ALLOWED_HOSTS; forwarded-host workflows now 403.
• Correct the Origin comment re: requests that carry no Origin header.
• TOCTOU between realpathSync and createReadStream in the download route (low risk for a local tool).