Code review · P3 Task 17, inspect a pipeline and launch a run¶
review-code on autopilot/P3-t17, base 643dd89, 2026-10-30. The step is P1 Task 17, built in the local-only order of inbox #80 (a), before Tasks 12 to 16. It adds:
packages/spectio-server/src/spectio_server/api/workflows.py:GET /api/v1/workflows:inspect, the location and Git rules, the fetch, and the no-follow reads of a pipeline's files;api/runs_write.py:POST /api/v1/runs, the body checks, the parameter check against the pipeline's schema, the run directory, then the rows and thelaunchcommand in one transaction;api/browser.py: the Host allow-list, cross-site and write checks before every route (dev-env §15.3 req. 4 and 7);api/problems.py:422 validation-failedwitherrors: [{field, message}];store/runs.py:insert_runandfirst, shared byPOST /runsand the index rebuild;app.py: the routers, mounted only in local mode on a machine that does not look shared (shared_host,no_api_reason);serve.py: the start text takes that one decision;sink.py: a launch stays active until its head is reported ended;rebuild.py: the Git import rule;- tests:
tests/api/test_launch.py,tests/api/test_req4_req7_browser.py,tests/security/test_shared_host.py,tests/test_serve.py, and four schema fixtures undertests/data/; - dependencies
jsonschema>=4.23,<5andpyyaml>=6.0.2,<7; dev dependencieshttpx>=0.27,types-jsonschema,types-pyyaml.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The endpoints are owned by api §5 (note 2026-10-30) and Imports are untrusted input; the browser checks by dev-env §15.3 req. 4 and 7; the rules the code assumed by inbox #86.
| Round | Head | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 4aaff5b (the step) |
changes (1 blocker, 1 major, 4 minors, 1 simplify) | block (1 blocker, 3 majors, 3 minors) |
| 2 | d9dcc6d (browser checks, linear tokenizer, no $ref retrieval, one insert_run) |
changes (0 blockers, 1 major, 3 minors) | approve (0 blockers, 0 majors, 3 minors) |
| 3 | a837e46 (the tokenizer's last two quadratic cases) |
approve (1 minor) | changes (0 blockers, 2 majors, 4 minors) |
| 4 | 2d64827 (no symlink read, no session-less API on a shared host) |
approve (4 minors) | approve (3 minors) |
| 5 | 5e4d12f (one shared-host decision, no symlink for profiles or entry files) |
approve (1 minor, 2 simplify) | approve (1 minor) |
| simplify | d2d4223, 3cbf735 |
code-simplifier: two changes, −13 +16 |
IDs repeat across rounds. Each round numbered its findings again, so each ID below carries its round: CR-P3T17-04 (r1) is not CR-P3T17-04 (r4). The commit messages use the bare IDs of the round they answer. The reports were relayed by the coordinator in summary.
Test-first. The worker reports that the round-5 fixes (5e4d12f) were written as tests first and failed before the change: test_serve_decides_the_api_once (new, a scan whose answer changes between calls), and test_inspect_symlinked_directory_not_followed (extended) with test_inspect_symlinked_profile_not_listed (new).
Gate at 3cbf735, from the worktree, re-run by doc-writer:
uv run --locked pytest -q # 429 passed, 15 deselected, 1 warning
uv run --locked ruff check . # All checks passed!
uv run --locked ruff format --check . # 68 files already formatted
uv run --locked mypy packages # Success: no issues found in 67 source files
uv run --locked lint-imports # Contracts: 4 kept, 0 broken.
uv lock --check # Resolved 54 packages
The one warning is the StarletteDeprecationWarning of CR-P3T17-03 (r1), still open.
CODE step=P3-t17 review=approve b/M/m=1/2/13 security=approve b/M/m=1/5/14 rounds=5 simplify=changed -13/+16 tests=pass
The counts add up the five rounds per reviewer. CR-P3T17-02 (r1), labelled "major simplify", is counted as a major; the three plain simplify findings (CR-P3T17-06 (r1), CR-P3T17-05 (r5), CR-P3T17-06 (r5)) are not counted.
Findings¶
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T17-01 (r1) | blocker | In local mode the API had no token, no Host allow-list and no Origin check, on the default port 8080. A web page could reach it through DNS rebinding and POST /runs an attacker's owner/repo, or start a clone with a cross-site GET of inspect. Inbox #37 (SR-091) decided this threat: Host allow-list, Origin check, JSON-only writes. |
fixed in d9dcc6d: api/browser.py enforces req. 4 and 7 before every route; the posture question is reserved in #86 |
| CR-P3T17-02 (r1) | major (simplify) | runs_write._insert copied about 60 lines of rebuild._add, and _first; the copies had drifted. |
fixed in d9dcc6d: one store.runs.insert_run and one first |
| CR-P3T17-03 (r1) | minor | Test output is not clean: StarletteDeprecationWarning: Using httpx with starlette.testclient is deprecated; install httpx2 instead. Raised again as CR-P3T17-05 (r2), CR-P3T17-04 (r3), CR-P3T17-07 (r4) and CR-P3T17-07 (r5). |
open, follow-up FU-P3-19 |
| CR-P3T17-04 (r1) | minor | head_left keeps a launch active when its run is terminal, it has a native ID and exited_at is null. If it can never be adopted again (its launch directory removed), the loop calls _try_adopt and _read for it on every tick, forever. |
open, follow-up FU-P3-18 |
| CR-P3T17-05 (r1) | minor | The rebuild skips the revision check for a Git location with no revision; the endpoint requires one. A revision-less Git manifest is foreign input that the rebuild accepts. | open, follow-up FU-P3-15 (rebuild hardening) |
| CR-P3T17-06 (r1) | simplify | wf.run_git = fetch assigns the module attribute directly and relies on the fixture's monkeypatch undo. |
open, follow-up FU-P3-19; code-simplifier does not edit tests of code that is kept |
| CR-P3T17-07 (r1) | minor | Item (10): P3-M4 has no Task 17 section; it should record CR-P3T17-01's outcome and the open points (cached branch clone, check_engine on every inspect, the flaky test_sigterm_stops_cleanly). |
fixed by the docs commit of this step: P3-M4 · Task 17 |
| SEC-P3T17-01 (r1) | blocker | No Host allow-list (req. 4) and no Origin or JSON check on writes (req. 7): DNS rebinding lets any page launch an attacker's Git pipeline as the user. | fixed in d9dcc6d |
| SEC-P3T17-02 (r1) | major | GET /workflows:inspect clones any https or ssh repository; a cross-site simple GET (an img tag) can trigger it. Each revision is a clone kept forever, with no size cap. |
partly fixed in d9dcc6d: a cross-site request is refused with 403. Open: the clone size cap, follow-up FU-P3-18 and inbox #86 |
| SEC-P3T17-03 (r1) | major | ReDoS in the nextflow.config tokenizer: an unterminated string with escapes rescans to the end at each quote (20k pairs: 6.5 s). |
fixed in d9dcc6d, completed in a837e46 (CR-P3T17-03 (r2)) |
| SEC-P3T17-04 (r1) | major | Parameter validation fetched remote $refs with jsonschema's default registry: file:// reads (demonstrated) and requests to internal hosts. |
fixed in d9dcc6d: an empty referencing.Registry; an unresolvable $ref is a 422 on params |
| SEC-P3T17-05 (r1) | minor | Inspect follows symlinks out of a cloned repository and returns the target's values as schema defaults. | fixed in 2d64827 (SEC-P3T17-05 (r3)) |
| SEC-P3T17-06 (r1) | minor | Git subprocess hardening: the whole os.environ reaches git (GIT_ALLOW_PROTOCOL, GIT_CONFIG_PARAMETERS); git-lfs smudge fetches from .lfsconfig URLs; the ssh user may start with -. |
open, follow-up FU-P3-18 (with SEC-P3T17-10 (r3), SEC-P3T17-09 (r4)) |
| SEC-P3T17-07 (r1) | minor | Input bounds: a location longer than the OS name limit gives a 500 (ENAMETOOLONG), demonstrated; no size cap on location, query, params or the body. |
open, follow-up FU-P3-17 |
| CR-P3T17-03 (r2) | major | Parsing nextflow.config was still quadratic: a lone final backslash defeated the \Z alternative, and the profiles regex had no word-start anchor. |
fixed in a837e46 |
| CR-P3T17-04 (r2) | minor | A malformed Referer such as http://[x/ makes urlsplit raise ValueError in the middleware: a 500 with a stack trace, not the 403. |
open, follow-up FU-P3-17 |
| CR-P3T17-05 (r2) | minor | The StarletteDeprecationWarning (see CR-P3T17-03 (r1)). |
open, FU-P3-19 |
| CR-P3T17-06 (r2) | minor | api/browser.py implements Task 14's and Task 16's checks (plan files auth/host.py, auth/writes.py), so two copies could exist later; NO_API still names "CSRF and Host checks" among the missing features. |
open: inbox #86 now says Tasks 14 and 16 take api/browser.py over; the NO_API wording goes to FU-P3-17 |
| SEC-P3T17-05 (r2) | minor | Inspect follows symlinks out of the pipeline directory (verified with a symlinked nextflow_schema.json returned as schema). |
fixed in 2d64827 |
| SEC-P3T17-06 (r2) | minor | A self-referential schema ({"$ref": "#"}) makes iter_errors recurse until RecursionError: a 500, not a 422 on params. Nothing is written. |
open, follow-up FU-P3-17 (with SEC-P3T17-08 (r3)) |
| SEC-P3T17-07 (r2) | minor | The cross-site refusal for GET fails open: a browser without Fetch Metadata [to be validated: which browser versions send Sec-Fetch-Site to http://127.0.0.1], a referrerpolicy="no-referrer" page, or an https page. Writes are not affected. |
open: recorded in inbox #86 rule 1 and in the dev-env §15.3 req. 7 note; closed by the SameSite=Strict session (P1 Tasks 12 and 13) and the clone size cap (FU-P3-18) |
| CR-P3T17-04 (r3) | minor | The StarletteDeprecationWarning (see CR-P3T17-03 (r1)). |
open, FU-P3-19 |
| SEC-P3T17-05 (r3) | major | Inspect follows symlinks out of an untrusted clone (for example config/config.yaml -> ../../../../../../.kube/config) and returns the file in the response; the UI could pre-fill those values into params. Reproduced with a gho_ token. |
fixed in 2d64827: every read walks from a descriptor of the pipeline directory with O_NOFOLLOW and takes only a regular file |
| SEC-P3T17-06 (r3) | major | The local-only gate is a configuration value: spectio init --local on a login node, or SPECTIO_EXECUTOR=local, mounts the session-less launch API for every Unix user of the node. |
fixed in 2d64827: shared_host() keeps the routers unmounted on a machine that looks like a cluster node or a shared one; inbox #86 corrected |
| SEC-P3T17-07 (r3) | minor | uvicorn's access log writes the raw query string, so a location with credentials reaches stderr even though it is then refused. | open, follow-up FU-P3-17 |
| SEC-P3T17-08 (r3) | minor | Validation against the pipeline's schema is unbounded and fails as 500: {"$ref": "#"} (RecursionError), a non-string $schema (TypeError), a catastrophic pattern; no cap on the body or on params depth. |
open, follow-up FU-P3-17 |
| SEC-P3T17-09 (r3) | minor | refuse_control rejects only C0 and C1: bidi overrides, zero-width characters, U+2028 and U+2029 pass in location and name. |
open, follow-up FU-P3-17 |
| SEC-P3T17-10 (r3) | minor | Git fetch hardening: the clone cache key drops the scheme, port and ssh user; the ssh user is not validated; git-lfs smudge fetches outside protocol.allow. |
open, follow-up FU-P3-18 |
| CR-P3T17-04 (r4) | minor | The shared-host decision was computed twice (routers and start text) and could disagree; test_serve_local_start_text scanned the test host's /proc. |
fixed in 5e4d12f: app.state.no_api → Started.no_api → start_text(…, no_api) |
| CR-P3T17-05 (r4) | minor | test_inspect_symlink_out_of_tree_not_read accepts 200 or 422, so a regression to 422 would still pass. |
open, follow-up FU-P3-19 |
| CR-P3T17-06 (r4) | minor | The api §5 note did not describe that a symlinked schema, config or nextflow.config is treated as absent, nor that the API is also absent on a host that looks shared. |
fixed by the docs commit of this step: two bullets of the api §5 note |
| CR-P3T17-07 (r4) | minor | The StarletteDeprecationWarning; starlette 1.10.0 imports httpx2 first and warns when it falls back to httpx. |
open, FU-P3-19 |
| SEC-P3T17-07 (r4) | minor | The shared-host check runs once, at boot: a user who logs in later gets the session-less API and can POST /runs with curl. Inbox #86 did not say so. The uid threshold is a fixed 1000, not UID_MIN. |
open: inbox #86 rule 1 now says so, with option (d); the reserved decision is Thomas's |
| SEC-P3T17-08 (r4) | minor | Inspect still followed symlinks in two places: snakemake_profiles (Path.glob) listed directories outside the clone, and is_file() let main.nf -> /etc/passwd count as an entry file. |
fixed in 5e4d12f |
| SEC-P3T17-09 (r4) | minor | git_url accepts an ssh user that starts with - (ssh://-oProxyCommand@localhost/a/b); only git's own check stops it (git 2.43.0: "strange hostname … blocked"). |
open, follow-up FU-P3-18 |
| CR-P3T17-05 (r5) | simplify | snakemake_profiles checks the same thing three times. |
fixed in d2d4223 by code-simplifier: os.listdir(fd), then _regular |
| CR-P3T17-06 (r5) | simplify | runs_write imports the private _regular; the path split is repeated. |
fixed in 3cbf735 by code-simplifier: _regular(root, rel) splits, and a public entry_files(directory) serves both callers |
| CR-P3T17-07 (r5) | minor | The StarletteDeprecationWarning. |
open, FU-P3-19 |
| SEC-P3T17-09 (r5) | minor | _read no longer catches an OSError from os.fstat or f.read (EIO on NFS, Lustre or FUSE): inspect answers a bare 500, not a 422 on location; if fstat raised, the descriptor would leak. |
open, follow-up FU-P3-17 |
Closed from earlier reviews:
- SEC-P3T11-01 (r3): the rebuild applies the Git import rule of
git_urlandcheck_revision(test_rebuild_git_location_import_rule); a revision-less Git manifest stays open as CR-P3T17-05 (r1). - SEC-P3T11-07 (r1):
test_rebuilt_queued_run_is_never_launchedpins that the rebuild writes no runner command. - CR-P3T09-R5-03:
test_launch_active_until_the_ended_report. - UX-G2-04: inspect returns
engine_filesand bothengine_versions. - SEC-P3T09-R5-05: control characters refused in location and revision (
test_location_control_characters_refused).
Fixes by round¶
| Commit | Findings |
|---|---|
d9dcc6d fix(api): Host, Origin and cross-site checks; linear config tokenizer; no $ref retrieval |
CR-P3T17-01 (r1), CR-P3T17-02 (r1), SEC-P3T17-01 to -04 (r1) |
a837e46 fix(api): linear nextflow.config scan with a trailing backslash or a long profiles word |
CR-P3T17-03 (r2) |
2d64827 fix(api): inspect reads no symlink; no session-less API on a shared host |
SEC-P3T17-05 (r1, r2, r3), SEC-P3T17-06 (r3) |
5e4d12f fix(api): one shared-host decision; inspect follows no symlink for profiles or entry files |
CR-P3T17-04 (r4), SEC-P3T17-08 (r4) |
d2d4223 refactor(P3-t17): simplify snakemake_profiles |
CR-P3T17-05 (r5), by code-simplifier |
3cbf735 refactor(P3-t17): simplify the entry-file check |
CR-P3T17-06 (r5), by code-simplifier |
code-simplifier left one thing alone: wf.run_git = fetch in test_launch.py:370 (CR-P3T17-06 (r1)), a test of code that is kept.
For the Progress page¶
The Task 17 section of P3-M4 is written: P3-M4 · Task 17, with its findings and open questions.