Skip to content

feat(api): Track 3.1 — URL-mode ingestion for POST /api/analysis-runs - #36

Merged
slittycode merged 3 commits into
mainfrom
claude/track3-url-ingestion-5XA5r
May 13, 2026
Merged

feat(api): Track 3.1 — URL-mode ingestion for POST /api/analysis-runs#36
slittycode merged 3 commits into
mainfrom
claude/track3-url-ingestion-5XA5r

Conversation

@slittycode

Copy link
Copy Markdown
Owner

First of four pieces from Track 3 of the merged external-repo review (docs/external-repo-review-2026-05-13.md). Lets producers analyze a publicly-hosted reference track by URL instead of uploading bytes through the browser — they don't need a local copy of the file anymore.

What this adds

POST /api/analysis-runs now accepts exactly one of:

  • track multipart field (existing — unchanged)
  • url form field (new — public http/https URL)

Everything else about the request shape is the same: analysis_mode, pitch_note_mode, pitch_note_backend, interpretation_mode, interpretation_profile, interpretation_model all behave identically. The downstream pipeline is unchanged — the URL mode just fetches the bytes server-side and feeds them into the same runtime.create_run(...) call.

Defense in depth

Concern Mitigation
Wrong scheme (file://, ftp://, etc.) Allowlist: {http, https} only
Oversize URL Capped at 4 KiB
SSRF to private network Hostname resolved up-front; any is_private, is_loopback, is_link_local, is_multicast, is_reserved, is_unspecified address (IPv4 or IPv6) → URL_BLOCKED_PRIVATE_HOST. Cloud metadata endpoint (169.254.169.254) is covered by is_link_local.
Oversize body Up-front check on declared Content-Length; streaming abort if accumulated bytes pass MAX_UPLOAD_SIZE_BYTES (100 MiB). Matches the multipart limit.
Slow upstream 30 s connect timeout, 60 s read timeout.
Blocking I/O in async route Fetch dispatched to asyncio.to_thread so the event loop stays responsive.

Known TOCTOU window: there's a small gap between SSRF validation (DNS resolve) and the actual fetch (HTTP client does its own resolve). A pinning resolver is the v2 hardening — out of scope here, called out in the code comments.

Error envelope

Condition HTTP Code
Both track and url provided 400 AMBIGUOUS_AUDIO_SOURCE
Neither provided 400 MISSING_AUDIO_SOURCE
Malformed URL / disallowed scheme 400 URL_INVALID
Hostname resolves to private IP 400 URL_BLOCKED_PRIVATE_HOST
Body exceeds 100 MiB 413 URL_TOO_LARGE (matches existing UPLOAD_TOO_LARGE)
Network / DNS / timeout / non-2xx 502 URL_FETCH_FAILED

Files

File Role
apps/backend/url_ingest.py (new, ~280 LoC) Pure validation + fetch. requests-based. Typed exception hierarchy: UrlIngestionErrorUrlInvalidError / UrlBlockedPrivateHostError / UrlFetchFailedError / UrlTooLargeError.
apps/backend/server.py (+~150 LoC) Route now accepts either field. New _create_analysis_run_record_from_url helper parallels the existing upload helper. Legacy /api/analyze is untouched (still multipart-only).
apps/backend/tests/test_url_ingest.py (new, 36 tests) Pure-logic coverage: URL shape, SSRF (loopback, RFC1918, link-local, IPv6, public allow), filename extraction with URL-decoding, MIME picker, fetch happy path, each error class.
apps/backend/tests/test_server.py (+171 lines, 6 tests) Route-level: success path, missing source, ambiguous source, each URL-ingest error code mapped to the right HTTP status.
apps/backend/ARCHITECTURE.md Route list documents the new either-or contract.

Verification

  • 36 url_ingest unit tests pass locally (Python stdlib + requests):

    $ python -m unittest tests.test_url_ingest
    ........................................
    Ran 36 tests in 0.015s
    OK
    
  • 6 route tests in CreateAnalysisRunUrlIngestionTests run via the existing AnalysisRuntime + patch.object(server, "get_analysis_runtime", ...) pattern. CI will run them.

What this PR does NOT do

  • No frontend UI for URL entry. This is a backend-only addition. Adding a "URL" tab next to the upload tab in apps/ui/src/components/FileUpload.tsx is a separate follow-up.
  • No hosted-mode host allowlist. The review recommended this for multi-tenant deployments. For now, URL mode trusts the SSRF guard to handle the common cases. An allowlist is a v1.1 hardening.
  • No DNS pinning. Acknowledged TOCTOU window between SSRF resolution and fetch resolution. The current guard catches the common cases (clearly-private IPs); pinning is a v2 hardening.
  • Legacy /api/analyze is unchanged. Still multipart-only by design.

Track 3 sequencing

This is piece 1 of 4. Remaining pieces are independent, each landing as its own PR:

  • Piece 2: X-Admin-Key on DELETE /api/analysis-runs/{run_id} and admin ops
  • Piece 3: GET /api/analysis-runs/{run_id}/source-audio re-serve (avoids re-upload on Phase 2 reruns)
  • Piece 4: Public state-machine collapse 8 → 5 at the response boundary

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg


Generated by Claude Code

First of four pieces from Track 3 of the merged external-repo review.
Lets producers analyze publicly-hosted reference tracks by URL instead
of uploading bytes through the browser.

Behavior:

- POST /api/analysis-runs now accepts exactly one of:
  - 'track' multipart field (existing behavior, unchanged)
  - 'url' form field (new) — a public http/https URL

- Validation up-front, before any HTTP request is dispatched:
  - URL length capped at 4 KiB
  - http/https only (file://, ftp:// rejected)
  - SSRF guard: hostname is resolved and rejected if any resulting
    address is private, loopback, link-local, multicast, reserved,
    or unspecified. AWS/GCP metadata endpoints (169.254.169.254)
    fall into is_link_local and are blocked.

- Fetch contract:
  - 30 s connect timeout, 60 s read timeout
  - Streamed with abort if accumulated bytes exceed
    upload_limits.MAX_UPLOAD_SIZE_BYTES (100 MiB), matching the
    multipart upload limit
  - Content-Length checked up-front if declared
  - Empty body treated as fetch failure

- Filename comes from the URL's last path segment with safe fallbacks
  (urlencoded names are decoded; '..' and empty segments fall back to
  'url_audio.bin'). MIME type prefers the response Content-Type, then
  falls back to mimetypes.guess_type(filename), then to
  application/octet-stream.

Error envelope codes added (status code in parens):

- URL_INVALID (400) — malformed URL, too long, or disallowed scheme
- URL_BLOCKED_PRIVATE_HOST (400) — SSRF rejection
- URL_TOO_LARGE (413) — size cap exceeded (matches existing
  UPLOAD_TOO_LARGE convention)
- URL_FETCH_FAILED (502) — network/DNS/timeout/non-2xx upstream
- MISSING_AUDIO_SOURCE (400) — neither track nor url provided
- AMBIGUOUS_AUDIO_SOURCE (400) — both provided

Files:

- apps/backend/url_ingest.py (new, 280 lines) — pure validation +
  fetch module. requests-based, dispatched to asyncio.to_thread from
  the route so the event loop isn't pinned by blocking I/O.

- apps/backend/server.py — route handler accepts both modes; new
  _create_analysis_run_record_from_url helper parallels the existing
  _create_analysis_run_record. The legacy /api/analyze wrapper at
  line 2663 is unchanged (still multipart-only).

- apps/backend/tests/test_url_ingest.py (new, 36 tests, all pass
  locally) — covers URL shape validation, SSRF guard (loopback,
  RFC1918, link-local, IPv6, public allow), filename extraction,
  MIME picker, fetch happy path and each error class.

- apps/backend/tests/test_server.py (+171 lines) — 6 new tests in
  CreateAnalysisRunUrlIngestionTests covering: URL-mode success,
  missing source, ambiguous source, and each of the four URL-ingest
  error codes mapped to the right HTTP status.

- apps/backend/ARCHITECTURE.md — route list documents the new
  either-or contract.

Future improvements (deferred):

- Hosted-mode host allowlist (the review's recommendation for
  multi-tenant deployments)
- DNS pinning to close the TOCTOU window between SSRF check and
  fetch
- A frontend UI for URL-mode entry — this PR is backend-only

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

Adds server-side URL ingestion to POST /api/analysis-runs, letting producers point ASA at a hosted reference track instead of uploading bytes. The new module (url_ingest.py) is cleanly separated, the exception hierarchy is well-structured, and the 36 unit tests are meaningful and pass. The route integration is correct and the downstream pipeline is untouched. One security issue blocks merge.

Findings

Blocking

url_ingest.py:161allow_redirects=True without redirect re-validation is an SSRF bypass that contradicts the stated defense posture.

The pre-fetch _assert_host_is_public guard checks only the initial hostname. With allow_redirects=True, an attacker who controls any public hostname can serve a 302 Location: http://169.254.169.254/latest/meta-data/ (or any RFC1918 target). requests follows it transparently, the metadata endpoint returns HTTP 200, and the route returns that body as audio content — the guard never fires.

The TOCTOU window you acknowledged is a narrow DNS-pinning gap. This is a wider and more exploitable hole: anyone with a domain and a web server can do it, no race condition required.

Fix options:

  1. allow_redirects=False and surface a URL_FETCH_FAILED if a redirect is returned (simplest; real audio CDNs don't 302 for direct file links).
  2. Custom redirect hook that calls _assert_host_is_public on each Location target before following.

Option 1 is probably the right call here.


Should fix

server.pyURL_FETCH_FAILED (502) returns {"error": {"code": ..., "message": ...}} with no retryable field. The contract in ARCHITECTURE.md says error envelopes always carry retryable. The existing ValueError/RuntimeError handlers in this route also violate this (pre-existing), but this PR introduces the first 502 in the route — the one case where retryable: true has real semantic meaning for a client deciding whether to retry. Worth fixing here rather than propagating the gap.


Worth considering

_is_non_public_address does not block 100.64.0.0/10 (RFC 6598 Shared Address Space / CGNAT). In Python 3.11.4+, ipaddress.is_private returns False for this range and is_reserved doesn't cover it either. Low real-world risk for an audio tool, but easy to add: ip in ipaddress.ip_network('100.64.0.0/10') as an extra check in _is_non_public_address.

Test results

test_url_ingest: 36 passed, 0 failed.

test_server.CreateAnalysisRunUrlIngestionTests: cannot run in this environment without the full Essentia venv (import chain server → analyze → numpy). Not a CI concern — the test structure is correct and the mock boundaries are sound.

Phase boundary check

Clean. No Phase 1 measurements touched, no analyzer output fields added/removed/renamed, analyze.py stdout contract unchanged.


Generated by Claude Code

claude added 2 commits May 13, 2026 07:15
… signatures

Three findings from PR #36 review plus the CI failure on the prior commit.

Blocking — redirect SSRF bypass (url_ingest.py):

The pre-fetch _assert_host_is_public guard only validated the initial
hostname. With allow_redirects=True, an attacker controlling any
public hostname could serve a 302 Location pointing at a private/
loopback target (e.g. 169.254.169.254 metadata endpoint) and bypass
the guard entirely.

Fix: allow_redirects=False. Surface 3xx as URL_FETCH_FAILED with a
message telling the user to provide a direct URL. The trade-off is
that services like Dropbox/Drive that return 302 to the actual file
won't work — that's a known limit. If real users complain, a v2
fix could re-validate the redirect target through the same SSRF
guard before following.

Should fix — retryable field in error envelopes (server.py):

ARCHITECTURE.md's error contract says envelopes carry requestId,
error.code, error.message, error.retryable, and diagnostics. The
new envelopes I introduced (URL_INVALID, URL_BLOCKED_PRIVATE_HOST,
URL_TOO_LARGE, URL_FETCH_FAILED, MISSING_AUDIO_SOURCE,
AMBIGUOUS_AUDIO_SOURCE) now all carry retryable explicitly.
URL_FETCH_FAILED → true (transient upstream); everything else →
false.

Worth considering — CGNAT range 100.64.0.0/10 (url_ingest.py):

Python's ipaddress.is_private returns False for the RFC 6598 Shared
Address Space (CGNAT) range, so the SSRF guard missed it. Added
an explicit check against ipaddress.ip_network('100.64.0.0/10').

CI failure on the prior commit — Form() default sentinels:

When the route function is called directly in tests (not through
HTTP dispatch), FastAPI's Form(...) defaults are unresolved sentinel
objects rather than the strings they wrap. All 6 of the new route
tests now pass the full set of form fields explicitly via a shared
DEFAULT_FORM_KWARGS class attribute, matching the pattern used by
the existing test_analysis_runs_endpoint_returns_canonical_stage_snapshot
test.

Tests:

- url_ingest: 36 → 40 (added redirect rejection at 302/301, CGNAT
  block at three addresses inside /10, allow at two addresses just
  outside)
- test_server CreateAnalysisRunUrlIngestionTests: +1 retryable
  assertion in test_url_fetch_failed_returns_502, +1 new
  test_url_invalid_envelope_is_not_retryable, all 6 prior tests
  updated to pass DEFAULT_FORM_KWARGS

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
The existing canonical test_analysis_runs_endpoint_returns_canonical_stage_snapshot
calls server.create_analysis_run(track=UploadFile(...)) without
passing url. When called this way (not through HTTP dispatch),
FastAPI's Form(None) default leaks as an unresolved sentinel object
— neither None nor a string. The check 'url is not None' was True
on that sentinel, then 'url.strip()' AttributeError'd.

Fix: isinstance(url, str) before treating url as user input. The
sentinel falls through as 'not provided' and the multipart-upload
path runs unchanged.

Inline comment explains the test-time concern so the next reader
doesn't 'simplify' it back to the bug.

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
@slittycode
slittycode merged commit 7ed60e9 into main May 13, 2026
2 checks passed
@slittycode
slittycode deleted the claude/track3-url-ingestion-5XA5r branch May 13, 2026 20:52
slittycode added a commit that referenced this pull request May 14, 2026
Fourth and final piece of Track 3 from the merged external-repo review.
Additive over the existing 8-state stage status — does not change or
remove anything.

What's new:

- apps/backend/stage_status.py (new) — pure module with the canonical
  8 -> 5 mapping table, the to_public_status() helper, and
  PUBLIC_STATUS_VALUES exported for type generation / docs.

  Mapping (internal -> public):
    queued        -> queued
    running       -> running
    blocked       -> queued        (transient scheduling state)
    ready         -> queued        (scheduled but not yet running)
    completed     -> completed
    failed        -> failed
    interrupted   -> interrupted
    not_requested -> null          (stage not in pipeline)

- apps/backend/server_phase1.py — _normalize_run_snapshot now also
  annotates each stage with publicStatus via the new
  _annotate_public_status helper. The original status field is
  preserved untouched.

- apps/backend/tests/test_stage_status.py (new, 16 tests) — pins
  every cell of the mapping, the public-value set, defensive cases
  (None, unknown, empty, case-sensitive), and a completeness check
  that fails if the set of internal states drifts without an explicit
  mapping update.

- apps/backend/tests/test_server.py (+1 test) — integration test
  that GET /api/analysis-runs/{run_id} returns publicStatus on all
  three stages with the correct values for a fresh run.

- apps/ui/src/types/backend.ts — new PublicStageStatus type (the
  5-value union). publicStatus: PublicStageStatus | null added to
  MeasurementStageSnapshot, PitchNoteTranslationStageSnapshot,
  InterpretationStageSnapshot. attempt summaries unchanged
  (publicStatus is only on top-level stages).

- apps/ui/src/services/analysisRunsClient.ts — new
  parsePublicStageStatus helper + PUBLIC_STAGE_STATUSES set; threaded
  into the three stage parsers. The helper accepts null and missing
  values without throwing so legacy snapshots that pre-date the
  field continue to deserialize.

- docs/adr/0001-phase1-json-schema-v1.md — documents the additive
  field, the mapping table, and the rationale (internal scheduling
  states are scheduling concerns the client doesn't need to act on).

Out of scope (deferred):

- Removing publicStatus duplication from the internal status field
  later. The additive approach was the explicit choice; collapsing
  to one field would be a breaking change requiring a v2 schema bump.
- Frontend UI that reads publicStatus. Existing UI continues to use
  the 8-state status field; publicStatus is the additive surface for
  future external consumers and any new UI views that prefer the
  smaller vocabulary.

This closes Track 3 (all four pieces shipped):
  - 3.1 URL ingestion (#36, merged)
  - 3.2 X-Admin-Key on DELETE (#37, merged)
  - 3.3 GET /source-audio re-serve (#40, merged)
  - 3.4 publicStatus additive (this PR)

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg

Co-authored-by: Claude <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.

2 participants