timothy/ersatztv
1
0
Fork 0
Code Issues 57 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

#197 S1 [BLOCKER]: API writes are fail-OPEN by default — no key ⇒ every mutation unauthenticated #280

New Issue
Closed
opened 2026-07-11 23:20:22 +02:00 by timothy · 2 comments
timothy commented 2026-07-11 23:20:22 +02:00
Owner
Copy Link
Copy Source

From the #197 cold API security review (see the roadmap comment on #197). Phase 1 hard gate — must land before any remote exposure.

Finding (BLOCKER): ApiKeyAuthorizationFilter returns early (open) when Api:WriteKey is null/empty — the shipped default — so ALL /api POST/PUT/PATCH/DELETE run unauthenticated, with no startup warning. Fail-open by construction. (ApiKeyAuthorizationFilter.cs:28-34.)

Fix (Phase-1 #1): delete the empty-key open branch; resolve the key from config/env, else auto-generate a 256-bit key and persist it to the config volume (mode 600, log the path not the value), then always enforce on mutating /api/* verbs. The SPA already stores + sends X-Api-Key on writes → near-zero SPA impact (verify the 401 UX links to the key-entry settings). No "open mode" flag (it would recreate this hole).

Constant-time key comparison for this filter already landed in PR #279. Refs #197.

From the #197 cold API security review (see the roadmap comment on #197). **Phase 1 hard gate — must land before any remote exposure.** **Finding (BLOCKER):** `ApiKeyAuthorizationFilter` returns early (open) when `Api:WriteKey` is null/empty — the shipped default — so ALL `/api` POST/PUT/PATCH/DELETE run unauthenticated, with no startup warning. Fail-open by construction. (`ApiKeyAuthorizationFilter.cs:28-34`.) **Fix (Phase-1 #1):** delete the empty-key open branch; resolve the key from config/env, else auto-generate a 256-bit key and persist it to the config volume (mode 600, log the *path* not the value), then always enforce on mutating `/api/*` verbs. The SPA already stores + sends `X-Api-Key` on writes → near-zero SPA impact (verify the 401 UX links to the key-entry settings). No "open mode" flag (it would recreate this hole). Constant-time key comparison for this filter already landed in **PR #279**. Refs #197.
timothy added the priority: highreviewsecurity labels 2026-07-11 23:20:22 +02:00
timothy commented 2026-07-11 23:28:38 +02:00
Author
Owner
Copy Link
Copy Source

Bundling — Bundle A "auth posture" (single session / one PR): #280 + #281 + #282 + #284 + #285.
These share the same surfaces — ApiKeyAuthorizationFilter, Startup.cs security config, the SPA web/src/api/client.ts key-attachment, and ApiControllerSecurityTests — so splitting them across sessions/PRs would guarantee merge conflicts. In particular #280 (fail-closed) and #282 ([RequiresApiKey] read-tier) both rewrite the filter and share the one SPA change ("send the key on all methods"). Parallel-safe against Bundle B (#283) and #289 (disjoint surfaces — can run concurrently). Phase-1 remote-exposure gate. Full plan: #197 roadmap comment.

**Bundling — Bundle A "auth posture" (single session / one PR): #280 + #281 + #282 + #284 + #285.** These share the same surfaces — `ApiKeyAuthorizationFilter`, `Startup.cs` security config, the SPA `web/src/api/client.ts` key-attachment, and `ApiControllerSecurityTests` — so splitting them across sessions/PRs would guarantee merge conflicts. In particular #280 (fail-closed) and #282 (`[RequiresApiKey]` read-tier) both rewrite the filter *and* share the one SPA change ("send the key on all methods"). **Parallel-safe against** Bundle B (#283) and #289 (disjoint surfaces — can run concurrently). Phase-1 remote-exposure gate. Full plan: #197 roadmap comment.
timothy added the in-progress label 2026-07-11 23:46:38 +02:00
timothy commented 2026-07-12 00:35:30 +02:00
Author
Owner
Copy Link
Copy Source

Closed — PR #292 (merged FF to main ee6be81c, CI green)

Done (S1 fail-closed writes). Deleted the empty-key "open" branch in ApiKeyAuthorizationFilter; there is no open mode. New IApiKeyProvider/ApiKeyProvider (singleton, resolved once at startup) yields a never-empty key: Api:WriteKey config → else persisted /config/api.key → else a generated 256-bit hex key. Every mutating /api request now requires X-Api-Key.

Key persistence hardening (from adversarial review): file created 0600 atomically (FileStreamOptions.UnixCreateMode, no world-readable window); an unreadable existing key file makes startup fail loud rather than silently regenerating + invalidating all client keys; a delete-race falls back to generate.

Files: ErsatzTV/Services/ApiKeyProvider.cs (new), ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs, ErsatzTV/Startup.cs, ErsatzTV.Core/FileSystemLayout.cs (ApiKeyPath); ErsatzTV.Tests/Services/ApiKeyProviderTests.cs + Filters/ApiKeyAuthorizationFilterTests.cs (rewritten). SPA: keyless /app/api-key entry screen + client sends the key on every method (web/src/screens/ApiKeyScreen.tsx, web/src/api/client.ts).

Verification: 1428/1428 .NET, 692/692 web, cold review + fix-commit re-review (both MERGEABLE), live E2E (no-key → 401, correct key → 200, /iptv streaming unaffected, api.key written 0600).

Docs: api-conventions §9 (auth posture), decisions.md (Bundle A entry).

Deferred to Phase-2/3: declarative OpenAPI security scheme + global 401 docs (#287), per-key rate limiting.

## Closed — PR #292 (merged FF to main `ee6be81c`, CI green) **Done (S1 fail-closed writes).** Deleted the empty-key "open" branch in `ApiKeyAuthorizationFilter`; there is no open mode. New `IApiKeyProvider`/`ApiKeyProvider` (singleton, resolved once at startup) yields a **never-empty** key: `Api:WriteKey` config → else persisted `/config/api.key` → else a generated 256-bit hex key. Every mutating `/api` request now requires `X-Api-Key`. **Key persistence hardening** (from adversarial review): file created `0600` atomically (`FileStreamOptions.UnixCreateMode`, no world-readable window); an unreadable existing key file makes startup **fail loud** rather than silently regenerating + invalidating all client keys; a delete-race falls back to generate. **Files**: `ErsatzTV/Services/ApiKeyProvider.cs` (new), `ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs`, `ErsatzTV/Startup.cs`, `ErsatzTV.Core/FileSystemLayout.cs` (`ApiKeyPath`); `ErsatzTV.Tests/Services/ApiKeyProviderTests.cs` + `Filters/ApiKeyAuthorizationFilterTests.cs` (rewritten). SPA: keyless `/app/api-key` entry screen + client sends the key on every method (`web/src/screens/ApiKeyScreen.tsx`, `web/src/api/client.ts`). **Verification**: 1428/1428 .NET, 692/692 web, cold review + fix-commit re-review (both MERGEABLE), live E2E (no-key → 401, correct key → 200, `/iptv` streaming unaffected, `api.key` written `0600`). **Docs**: api-conventions §9 (auth posture), decisions.md (Bundle A entry). **Deferred to Phase-2/3**: declarative OpenAPI security scheme + global 401 docs (#287), per-key rate limiting.
timothy removed the in-progress label 2026-07-12 00:36:03 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels priority: high review security
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#280