Skip to content

fix: confine the http_request proxy to the panel scope - #174

Open
dragonstyle wants to merge 5 commits into
mainfrom
security/proxy-scope-2026-08-28
Open

fix: confine the http_request proxy to the panel scope#174
dragonstyle wants to merge 5 commits into
mainfrom
security/proxy-scope-2026-08-28

Conversation

@dragonstyle

Copy link
Copy Markdown
Member

Summary

The log and scan webviews expose a generic http_request proxy
(kMethodHttpRequestproxyRpcRequest) that forwarded an arbitrary
webview-supplied request straight to the token-authorized view server. Every
named RPC method confines its location parameter to the panel's scope
(requireScope / requireScanScope), but the proxy did not — so injected
webview script could reach the same endpoints (e.g.
/api/log-bytes/<encoded /etc/…>, /api/log-edit/…, /api/v2/scans/… DELETE)
outside the panel scope, making the named-method guards ineffective for the
modern viewer (which prefers the proxy).

Change

  • Add src/core/package/proxy-scope.ts. It parses the requested view-server
    route and extracts any location it carries — path segment
    (/api/log-bytes/<enc>), query parameter (/api/logs?log_dir=…,
    ?log=…, repeated ?file=…), or encoded directory
    (/api/scout/transcripts/<base64url>, /api/v2/scans/<base64>) — then runs
    each through the panel's existing scope predicate. No-location endpoints
    (/api/log-dir, /api/app-config, /api/dist, the scan listing, …) are
    allowed, and any unrecognized route is rejected by default.
  • Wire assertLogProxyInScope / assertScanProxyInScope into LogviewPanel
    and ScanviewPanel immediately before proxyRpcRequest.

The route table mirrors the endpoints the extension's own named methods build
(which mirror the view-server API the viewer uses).

Testing

  • pnpm typecheck, pnpm lint, pnpm format:check, pnpm compile-tests pass.
  • Added proxy-scope.test.ts covering in-scope allow, out-of-scope reject
    (segment/query/base64), and unknown-route reject for both surfaces.
  • Please exercise the log & scan viewers in CI / manually before merge.
    Because unrecognized routes are rejected by design, if the viewer relies on a
    view-server endpoint not represented in the route table it will need to be
    added — this is the one behavior worth verifying against a live viewer.

Merge

Squash merge.

🤖 Generated with Claude Code

Both the log and scan webviews expose a generic `http_request` proxy that
forwarded an arbitrary webview-supplied request to the token-authorized view
server, bypassing the requireScope / requireScanScope guards applied to every
named RPC method — so injected webview script could read/write/delete any path
or URL the server can reach (e.g. /api/log-bytes/<encoded /etc/...>).

Add core/package/proxy-scope.ts, which parses the requested view-server route,
extracts any file/dir/URL location it carries (path segment, query parameter, or
base64/base64url directory), and requires each to pass the same panel scope
check as the named methods; unrecognized routes are rejected by default. Wire it
into both LogviewPanel and ScanviewPanel before proxyRpcRequest. Adds unit tests
covering in-scope, out-of-scope, and unknown-route cases for both surfaces.

CWE-863.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/core/package/proxy-scope.ts Outdated

// Endpoints whose location is a query parameter.
if (pathname === "/api/logs" || pathname === "/api/log-files") {
return check(params.get("log_dir"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Duplicate query parameters can bypass the scope check. params.get("log_dir") validates only the first occurrence, but a Starlette/FastAPI server resolves a scalar query param to the last occurrence (QueryParams is an ImmutableMultiDict whose lookup dict is built last-wins) — and this repo's own comments note the backing server is FastAPI (editLog unwraps FastAPI detail, scout v2 routes). So injected webview script can send

/api/logs?log_dir=<in-scope>&log_dir=<out-of-scope>

which passes this check while the server acts on the out-of-scope value. Same pattern for log (line 89), and results_dir in assertScanProxyInScope (line 153). Even on a server that takes the first value, validating one copy of a repeatable parameter is fragile across server versions.

Fix is the same shape as the log-headers branch: validate every occurrence, e.g.

const dirs = params.getAll("log_dir");
if (dirs.length === 0) {
  throw proxyError(request.path);
}
dirs.forEach(check);

Comment thread src/core/package/proxy-scope.ts Outdated
if (!dirSegment) {
throw proxyError(request.path);
}
return check(decodeBase64(dirSegment));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The optional <base64 file> segment is forwarded unvalidated. Only the dir segment is checked, but deleteScan in scout-view-server.ts shows this route also takes a second base64 segment (DELETE /api/v2/scans/<b64 dir>/<b64 file>). The named method builds that file from basename(), so it can never contain a separator — but the proxy lets the webview supply an arbitrary decoded value such as ../../other/scan or an absolute path/URL. If the server joins dir + file (the natural implementation), the DELETE resolves outside the validated dir even though the dir segment passed the scope check.

Suggest validating the decoded file segment too when present — e.g. reject it if it contains / (or check dir + "/" + file against inScope).

"/api/log-edit/",
"/api/log-message/",
];
if (kSegmentRoutes.some((route) => pathname.startsWith(route))) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hardening (non-blocking): pin the exact route shape instead of prefix-matching. startsWith(route) + checking only segments[3] admits deeper paths the named methods never generate, e.g. /api/log-bytes/<in-scope>/<anything> passes with only the first segment validated, and the raw tail is forwarded to the server. Today that extra tail most likely just 404s, but if any current or future server route binds the remainder with a path-style converter, the effective location the server acts on differs from the one validated here. Since every legitimate caller produces exactly one segment after the prefix, requiring segments.length === 4 (here and in the scan variant at line 167) closes that gap for free.

Related: decodeBase64/decodeBase64Url use Node's lenient decoders (invalid characters are dropped / the alternate alphabet is accepted), while the Python server decodes with different leniency rules — rejecting non-canonical base64 input before decoding would remove that parser-differential as well.

};

// Endpoints that carry no file/dir location.
const kNoLocation = new Set([

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regression risk (flagged in the PR body — two concrete candidates to verify). Since unrecognized routes fail closed and the viewer dist lives in inspect_ai (not this repo), the route table can only be validated against a live viewer. Two specific cases to check:

  1. inspect_ai's browser API client polls /api/events for log-update detection. If the modern viewer routes its browser client through the http_request proxy, every poll now gets rejected (likely as repeated errors rather than a visible failure). The endpoint carries no file location (loaded_time/last_eval_time params only), so adding it to kNoLocation is safe if it turns out to be used.
  2. evalLogsSolo reports log_dir: "" for single-file panels. If the viewer ever echoes that back as /api/logs?log_dir= (empty value) or omits the param, the request is rejected here — worth confirming file-mode panels still refresh.

Neither is confirmable from this repo, so this is informational: it just makes the "exercise the viewers before merge" step in the PR description concrete.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review summary

This closes a real authorization gap (the http_request proxy carrying the server auth token with none of the named-method scope guards), and the overall design is sound: the check and the forwarded request go through the same WHATWG URL parsing (parsePath here, fetch in serverFetch), so dot-segment/backslash/percent-encoding tricks normalize identically on both sides and cannot smuggle a different path past the check; malformed input fails closed; and the route table matches every endpoint the extension itself builds (I cross-checked all of inspect-view-server.ts and scout-view-server.ts — nothing the named methods generate is missing).

Findings (inline comments have details):

  1. Duplicate query parameters bypass the scope check (proxy-scope.ts:83, also lines 89 and 153). params.get() validates the first occurrence of log_dir/log/results_dir, but Starlette/FastAPI resolves a repeated scalar query param to the last occurrence, so ?log_dir=<in-scope>&log_dir=<out-of-scope> passes the guard while the server acts on the out-of-scope value. Validate all occurrences via getAll, as the log-headers branch already does. This is the one change I would consider blocking, since it defeats the confinement this PR exists to add.
  2. /api/v2/scans/<b64 dir>/<b64 file>: the file segment is unvalidated (proxy-scope.ts:177). The named deleteScan builds it from basename(), but the proxy accepts a decoded file of ../../… or an absolute path; if the server joins dir + file, the DELETE escapes the validated dir. Validate the file segment when present.
  3. Non-blocking hardening (proxy-scope.ts:109, :167): segment routes prefix-match and check only segments[3], admitting deeper paths no legitimate caller generates — require the exact segment count. Also consider rejecting non-canonical base64 rather than relying on Node's lenient decoders, to avoid decode-differentials with the Python server.
  4. Regression risk (informational; matches the verification ask already in the PR body): fail-closed unknown routes could break viewer features that proxy endpoints outside the extension's named set — the concrete candidates are /api/events (inspect_ai browser-client polling; location-less, safe to allowlist if used) and /api/logs with the empty log_dir that evalLogsSolo reports for file-mode panels. Needs the live-viewer check the PR description calls for; classification: accepted given that verification happens before merge.
  5. Nit (non-blocking): assertLogProxyInScope and assertScanProxyInScope duplicate their scaffolding (parse-with-fallback, the check closure, the error). A small shared helper taking a route table would remove the duplication and make future endpoint additions one-line changes.

Tests are well-aimed (in-scope allow, out-of-scope reject per route class, unknown-route reject); fixes for items 1–2 should come with matching cases (duplicate params, hostile v2 file segment).

Note: I could not run pnpm check in this sandbox (command-approval restrictions), so this review is static; nothing in the diff looks type- or lint-problematic.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

🔎 Review complete.

dragonstyle and others added 4 commits August 28, 2026 16:01
The proxy authorizer rejected /api/logs (and the other query-location listing
endpoints) when called with no location parameter, which broke the viewer's
config load ("Failed to load application configuration: Refusing proxied request
to /api/logs …"). A missing location means the server uses its own configured
default — not an attacker-chosen path — so allow it; scope is still enforced
whenever a location IS supplied (and every file-content path-segment endpoint
still requires an in-scope location).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Temporary: surfaces the exact endpoint the viewer requests through the proxy so
we can complete the route table. Reduce/remove before merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enumerated the actual FastAPI routes (fastapi_server.py). The viewer uses
several endpoints the extension's named methods don't, so they were rejected as
unknown routes and the log viewer hung. Add them:
- path-segment file routes: /api/log-info/, /api/log-download/
- query routes: /api/pending-sample-data-urls (log), /api/log-message (log_file),
  /api/eval-set + /api/flow (log_dir [+ joined dir])
- no-location: /api/events

Scope is still enforced on every location that's supplied. Extends the tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enumerated inspect_scout's /api/v2 routers. The scan viewer talks to the scout
server via these, none of which were mapped, so the scan/transcript views hung.
Add them:
- no-location: /api/v2/{app-config,project/config,topics,topics/stream,scanners,
  code,searches,scans/active,startscan,validations}
- directory-scoped (segments[4] is base64url, matching decode_base64url):
  /api/v2/scans/, /api/v2/transcripts/, /api/v2/validations/
Also fixes the pre-existing /api/v2/scans/ decode (base64url, not standard
base64). Legacy /api/scan* routes unchanged. Extends the tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant