Skip to content

fix(api): bound inbound control request bodies - #799

Open
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-request-body-budgets
Open

fix(api): bound inbound control request bodies#799
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-request-body-budgets

Conversation

@Quick104

Copy link
Copy Markdown
Contributor

Problem

Related issue: N/A — audit-validated security fix

Plugin HTTP routes, webhook-sync deliveries, and Jellyfin control requests could buffer or parse request bodies without a byte ceiling. A public plugin route made the strongest case unauthenticated; authenticated plugin routes, signed webhooks, PlaybackInfo, and Capabilities shared the same missing boundary. Chunked bodies bypassed Content-Length-based screening, and Plex multipart parsing could spill an unbounded aggregate body to temporary disk.

Fresh verification against origin/main at 8164fd594b9fdd8c1944bb0b6251f2d00e4a24ca reproduced a 3 MiB + 1 byte chunked public-plugin request reaching the plugin and returning 200 before this patch.

Approach

Add internal/requestbody as the shared byte-counted read boundary. It uses http.MaxBytesReader, reads through the enforced limit, closes the transport body, and distinguishes overflow from other read failures. Bodies are rejected rather than truncated.

The limits reflect each control surface:

  • Plugin routes: 3 MiB. The body is opaque, and this leaves room inside the plugin runtime's default 4 MiB gRPC receive envelope for protobuf fields and forwarded metadata.
  • Webhook sync: 4 MiB aggregate. Provider JSON/form payloads are normally a few KiB, while Plex may include a small thumbnail. The cap is below Plex's 10 MiB multipart memory threshold, so an oversized delivery cannot spill to temporary disk.
  • Jellyfin PlaybackInfo and Capabilities: 256 KiB. These requests contain control JSON and device profiles, which are normally tens of KiB and never media.

Plugin descriptor and access checks still run before the body read. A valid webhook secret is resolved before reading, preserving unknown-secret 404 behavior, but overflow is rejected before provider parsing, event logging, or watch-state mutation. All four PlaybackInfo routes and both Capabilities aliases share the Jellyfin limit. Jellyfin debug logging now captures a prefix while forwarding the complete body instead of truncating what the handler receives.

No native client contract changed, and no Apple or Android follow-up is needed. Jellyfin compatibility is handled directly. Upload and media-stream endpoints were reviewed and left unchanged because their large-body semantics are intentional or already have dedicated budgets. There is no published API document for these oversized control-body errors to update.

Validation

Focused and adversarial checks:

go test ./internal/requestbody ./internal/plugins ./internal/api/handlers ./internal/webhooksync ./internal/jellycompat -run '<body-budget tests>' -count=1
PASS

go test -race ./internal/requestbody ./internal/plugins ./internal/api/handlers ./internal/webhooksync ./internal/jellycompat -run '<body-budget tests>' -count=1
PASS

go vet ./internal/requestbody ./internal/plugins ./internal/api/handlers ./internal/webhooksync ./internal/jellycompat
PASS

golangci-lint run --new-from-merge-base="origin/main" ./...
0 issues

The complete Linux Go suite passed against the exact candidate mirror on dev-builder:

make test-go
PASS (including internal/jellycompat)

The local macOS make test-go run reached two pre-existing process-lock failures in internal/jellycompat: TestBeginWebOperationRecoversDeadProcessLock and TestBeginWebOperationRejectsLiveProcessLock. The same failures reproduce on untouched origin/main; the exact Linux candidate run passed them.

The remaining repository gate passed:

make embed-stub
go build ./...
gofmt -l .
go vet ./...
pnpm install --frozen-lockfile
pnpm run lint                 # 0 errors; inherited warnings only
pnpm run format:check
pnpm run build
make test-web                 # 294 files, 2,184 tests passed
make verify-settings-bindings-all
make verify-playback-fixtures
make verify-local-paths

Live validation used the isolated audit-body-budget dev-builder sandbox with the exact candidate. doctor passed before testing. A registered synthetic plugin and API-created webhook connections produced:

plugin public normal/exact-limit: 200/200
plugin public/authenticated over-limit, chunked: 413/413
Emby, Jellyfin, Plex normal webhook controls: 204/204/204
Emby, Jellyfin, Plex, and legacy webhook alias over-limit, chunked: 413
Jellyfin Capabilities normal/over-limit: 204/413
Jellyfin PlaybackInfo over-limit, chunked: 413
24 concurrent oversized requests: health 200, process running/healthy
memory before/after: 215.7 MiB / 106 MiB

The sandbox was torn down after validation. Tokens, webhook secrets, private hostnames, and media details were not printed.

Risks

Clients or plugins sending unusually large control documents will now receive 413. The limits have substantial headroom over observed legitimate payloads, but a legitimate integration above a cap would need a narrowly justified limit adjustment. Rollback is the single commit in this PR; there is no migration or persisted-format change.

AI Disclosure

  • Tool(s): Codex; Modern Go Guidelines CLI; Codex Security fix-finding workflow; Silo dev-builder workflow
  • Model(s): gpt-5.6-sol
  • Involvement: Fully AI-generated; human verification pending
  • Adversarial review: A fresh read-only investigator traced every body source through plugin gRPC buffering, Plex/Emby/Jellyfin webhook parsing, Jellyfin profile persistence, alternate encodings, multipart behavior, compression behavior, and auth modes before editing. A separate fresh read-only reviewer then challenged the final diff for bypasses and compatibility regressions. It found no source-backed bypasses or regressions. One additional contract hardening was made after review: valid webhook secrets are resolved before reading so unknown-secret 404 precedence remains unchanged. Focused, race, full Linux, and live concurrent validation passed afterward.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 37 minutes.

View limit details

Limit details: You’ve used the included review currently available. Your 94 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c726986f-b6da-4a9a-a9e8-753c0324333a

📥 Commits

Reviewing files that changed from the base of the PR and between 8164fd5 and 6884625.

📒 Files selected for processing (12)
  • internal/api/handlers/webhook_sync.go
  • internal/api/handlers/webhook_sync_test.go
  • internal/jellycompat/handlers_playback.go
  • internal/jellycompat/logging.go
  • internal/jellycompat/playback_negotiation_dedup_test.go
  • internal/jellycompat/readfrom_test.go
  • internal/plugins/http_proxy.go
  • internal/plugins/http_proxy_profile_test.go
  • internal/requestbody/requestbody.go
  • internal/requestbody/requestbody_test.go
  • internal/webhooksync/providers_test.go
  • internal/webhooksync/service.go

Comment @coderabbitai help to get the list of available commands.

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