fix(stremio_mcp): harden credential handling, library reads, and network I/O - #20
Merged
Conversation
Implements the three P0 fixes from the read-only security audit. F1 - TMDB credentials can no longer reach logs or returned errors. Network failures are now described only by a category, host, and status code that this module constructs; no path logs a prepared URL, a request payload, or a raw upstream exception. A redaction filter installed on the server logger, the root handlers, and the httpx/httpcore loggers strips registered credentials, secret-bearing query parameters, and Authorization values from every log record, traceback, and tool result. A TMDB v4 read access token is now sent as an Authorization header so it never enters a URL; a legacy v3 key has no header form and is still a query parameter, which is why no failure path reports one. F2 - Library reads return typed outcomes (LibraryRead, LibraryListRead, MetaRead) that separate found, an authoritative not-found, and an error. Mutations fail closed: add and remove abort without writing on any read error, _id mismatch, duplicate row, unrequested extra row, or content-type mismatch, so a transient failure can no longer be read as absence and reset existing watch state. Writes abort when the write request itself fails and when verification cannot confirm identity, type, removal state, and watch state. The dispatcher now reports an unavailable library separately from an empty one for list, continue, search, check, and library-sourced play. F3 - All HTTP work moves to one lifecycle-managed async httpx client with explicit connect/read/write/pool timeouts, a bounded response body, a bounded connection pool, and cancellation. Previously TMDB requests were synchronous with no timeout and blocked the MCP event loop, freezing unrelated device controls. Automatic searches resolve external IDs concurrently under a bounded semaphore instead of up to ten serial requests, and an auto search reports a half-outage rather than passing it off as "no results". Adds 56 tests covering sentinel-secret non-disclosure across HTTP, timeout, JSON and connection faults; bearer vs query authorization; library fault injection for every failure category, empty success, mismatched identity, duplicate rows and a read/write race; and transport timeouts, cancellation, bounded size, bounded fan-out, and device-control responsiveness during a stalled network call. Compatibility: TMDBClient and StremioAPIClient methods are now coroutines taking a shared AsyncHTTPClient, and requests is replaced by httpx (already a transitive MCP dependency). Documented in CHANGELOG.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…de, scope state verify, drop wrappers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Implement the first three P0 fixes (F1, F2, F3) from a read-only security audit of the stremio-mcp MCP server, then validate them. This is security-hardening work on a released 0.1.0 package.
F1: Prevent TMDB credentials from entering logs on HTTP, timeout, JSON, or connection failures. Previously requests' raise_for_status() produced exception text containing the prepared URL (with api_key in the query string) and broad handlers logged it verbatim. Deliberate approach: describe every network failure only by facts the module constructs itself (category/host/status code), never a prepared URL, request payload, or raw upstream exception; plus a SecretRedactingFilter installed on the server logger, root handlers, and the httpx/httpcore loggers as a backstop for tracebacks and third-party request-line logs. The audit asked to 'prefer an authorization form that does not place secrets in URLs when compatible' - TMDB only accepts a v4 read access token (a JWT) as an Authorization: Bearer header, while a legacy v3 key has no header form and must remain a query parameter, so the credential form is detected from the credential itself rather than assumed. This is intentional, not an oversight: v3 keys still travel in the query string, which is exactly why no failure path may report a URL. Sentinel-secret regression tests prove credentials and secret-bearing query strings never appear in logs or returned errors.
F2: Replace ambiguous Stremio library reads with typed outcomes. Previously _make_request collapsed every transport/HTTP/JSON/API error to {} and get_library_item collapsed both an error and a successful not-found to None, so add_to_library treated a transient read failure as proof the item did not exist and overwrote existing watch state with default state; get_library_item also trusted the first returned row without checking its _id. Deliberate approach: LibraryRead/LibraryListRead/MetaRead dataclasses with a ReadStatus enum that separates FOUND, authoritative NOT_FOUND, and ERROR. Reads fail closed - anything other than exactly one row whose _id equals the requested ID is an ERROR, including duplicate rows, unrequested extra rows, non-dict rows, and unexpected response shapes. Every mutation aborts without writing on ERROR, verifies exact identity and expected content type, and preserves watch state. Writes abort when the write request itself fails and when the verification read cannot confirm identity, type, removal state, and watch state (a state difference is treated as a read/write race and reported as failure rather than claimed as success). Fault-injection tests cover timeouts, HTTP/API/JSON errors, empty success, mismatched identity, duplicate rows, and a read/write race.
F3: Replace synchronous timeout-free network work in async MCP handlers. Previously all three TMDB GET paths used a synchronous requests.Session with no timeout, called directly from the async MCP dispatcher, so one slow call blocked the event loop and froze unrelated device/playback controls; an auto search was also an N+1 of up to 12 serial requests. Deliberate approach: one lifecycle-managed AsyncHTTPClient wrapping httpx.AsyncClient with explicit bounded connect/read/write/pool timeouts, a bounded response body read via streaming, a bounded connection pool, and cancellation support; external-ID fan-out runs concurrently under a semaphore. Bounds are configurable via STREMIO_MCP_* env vars whose bad values are reported by variable name without echoing the value.
Deliberate decisions a reviewer reading only the diff would not know:
Verification already run locally and passing: uv sync --locked, 86 unit tests (up from 30), compileall, uv build, uv lock --check, uv pip check, built-wheel console startup with all sensitive/device configuration blank, and a focused MCP protocol-level session confirming tools/list, schema rejection, and that device controls answer in ~2ms while a TMDB request is stalled mid-flight.
What Changed
SecretRedactingFilteris installed on the server logger, root handlers, and thehttpx/httpcoreloggers as a backstop. TMDB auth form is detected from the credential itself (v4 read token →Authorization: Bearer; legacy v3 key stays a query parameter).LibraryRead/LibraryListRead/MetaReadoutcomes with aReadStatusenum separatingFOUND, authoritativeNOT_FOUND, andERROR, so a transient read failure can no longer be mistaken for "item absent". Anything other than exactly one row whose_idmatches the request is anERROR; mutations abort without writing onERROR, verify identity/type/removal/watch state after the write, and preserve existing watch state on re-add.requestssession is replaced by a single lifecycle-managedAsyncHTTPClientoverhttpxwith explicit connect/read/write/pool timeouts, a bounded streamed response body, a bounded pool, and cancellation support; external-ID lookups fan out concurrently under a semaphore instead of running up to 12 serial requests. Bounds are configurable viaSTREMIO_MCP_*env vars whose bad values are reported by variable name only. TMDB searches now raiseHTTPClientErrorso the dispatcher can distinguish an outage from "no results", andtype=autoreturns the working half with an explicit(partial results — …)note.Compatibility:
TMDBClient/StremioAPIClientmethods are now coroutines taking a sharedAsyncHTTPClient, andrequestsis replaced byhttpx(already a transitive MCP dependency); recorded inCHANGELOG.mdunder Unreleased/Changed. Review feedback was folded in: redirect andhttpx.StreamErrorresponses get typed categories, the Stremio API errorcodeis surfaced, write verification compares only the state keys this module writes, and the error-erasing legacy wrappers were removed.Audit findings F4–F10 are P1 and lower and are intentionally out of scope.
Verification:
uv sync --locked, 91 unit tests,compileall,uv build, anduv lock --checkall pass, plus an end-to-end MCP-protocol harness (real server subprocess over stdio, fake TMDB/Stremio/Cinemeta upstream, stub adb) covering a sentinel-secret leak check on a TMDB 401, a faulted-read check asserting zerodatastorePutrequests, a soft-deleted re-add watch-state comparison, and a concurrency check that device controls answer while a TMDB request is stalled.Risk Assessment
✅ Low: The follow-up commit resolves every round-1 finding with narrowly scoped, well-tested changes that preserve the typed fail-closed and credential-redaction invariants, and no new issues surfaced in a full re-review of the delta and its surrounding call sites.
Testing
Ran the full CI-equivalent set (locked sync, 91 unit tests, compileall, build, lock check) and then drove the actual MCP server as a subprocess over stdio against a local fake TMDB/Stremio/Cinemeta upstream with synthetic sentinel credentials and a stub adb, capturing a JSON transcript plus the server's stderr: a TMDB 401 surfaced as a category/host/status-only error with the sentinel key provably on the wire but absent from every log line and response; a faulted library read made both check and add fail closed with zero datastorePut requests sent, while a successful re-add wrote back the exact prior watch state; and a device control call answered in ~0.3s while a 6s TMDB request was stalled mid-flight, showing the event loop is no longer blocked. No UI surface is involved (stdio MCP server), so the end-user artifacts are the protocol transcript and server log rather than screenshots. All checks passed and build artifacts were cleaned from the worktree.
Evidence: End-to-end MCP session transcript (F1/F2/F3 evidence)
Evidence: Server stderr during the MCP session (no credential or URL present)
INFO:stremio-mcp:Stremio library access enabled ERROR:stremio-mcp:TMDB movie search failed: category=http_status host=127.0.0.1 status=401 ERROR:stremio-mcp:Network failure in tool 'search': category=http_status host=127.0.0.1 status=401 ERROR:stremio-mcp:Stremio API request failed (datastoreGet): category=http_status host=127.0.0.1 status=503 INFO:stremio-mcp:Connected to Android TV at 127.0.0.1:5555Evidence: Harness used to produce the evidence
Evidence: Key end-user-visible outcomes from the MCP session
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed ✅
src/stremio_mcp.py:719-request_jsononly treatsstatus_code >= 400as an error, but the client is built withfollow_redirects=False(line 667). A 3xx response therefore falls through tojson.loads(body)on an empty/HTML redirect body and is reported ascategory=invalid_json, which is misleading and also a silent behavior change from the previousrequestsclient (which followed redirects by default). Add an explicit300 <= status_code < 400branch raising aredirectcategory (or enable bounded redirect following).src/stremio_mcp.py:974- A Stremio API-level error is collapsed tocategory=api_error kind=dict, discarding every diagnosable fact. The common real case — an expired or revokedSTREMIO_AUTH_KEY— surfaces to the user asError: library unavailable (category=api_error kind=dict)with no indication that re-authentication is needed. Stremio's error object carries a numericcodethat is server-generated and credential-free; including it (e.g.code=1alongsidekind=) keeps the no-echo guarantee while making the failure actionable.src/stremio_mcp.py:1211-put_library_itemrequirespersisted.get("state") == item.get("state")by exact dict equality. If the Stremio datastore normalizes the echoed state at all (drops a null-valued key, adds a server-side field, coerces a type), a write that actually succeeded is reported aswrite verification state conflictandlibrary addreturnsFailed to add ...— prompting the user to retry a mutation that already landed. The intent records this fail-closed choice deliberately, so flagging rather than changing: consider comparing only the state keys this module writes, or reporting a distinct 'written but state differs' outcome instead of a flat failure.src/stremio_mcp.py:1043-get_library_item(1043),get_library(1066),get_continue_watching(1098) andsearch_library(1118) survive as wrappers that collapse ERROR back intoNone/[]— exactly the ambiguity F2 removes. They have no call sites left insrc/; only tests exercise them (one test even names the behavior..._hides_errors_as_none). Since the CHANGELOG already declares the client classes breaking-changed, keeping error-erasing wrappers is a live footgun for the next caller. Consider removing them or documenting them as deprecated.src/stremio_mcp.py:714-request_jsoncatcheshttpx.TimeoutExceptionandhttpx.HTTPError, buthttpx.StreamError(and its subclasses, which derive fromRuntimeError, notHTTPError) can escape fromresponse.aiter_bytes()in_read_bounded. Such a failure bypasses the typedHTTPClientErrorcontract and lands in the dispatcher's genericexcept Exceptionhandler, so callers likeadd_to_librarysee an unexpected exception rather than a categorized transport error. Addinghttpx.StreamErrorto the connection branch closes the gap.🔧 Fix: Type redirect/stream errors, expose API error code, scope state verify, drop wrappers
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
uv sync --lockeduv run --locked python -m unittest discover -s tests -v(91 tests, all pass)uv run --locked python -m compileall -q src testsuv buildanduv lock --checkEnd-to-end MCP-protocol harness: real server subprocess over stdio + fake TMDB/Stremio/Cinemeta HTTP upstream + stub adb (e2e_mcp_harness.py)F1 live check:tools/call search {query: leak-test}against a TMDB 401, then grep of server stderr and all MCP responses for the sentinel API key and forapi_key=/ upstream URLF2 live check:tools/call library {action: check|add, imdb_id: tt0068646}with datastoreGet faulted to 503, asserting zero/api/datastorePutrequests reached the upstreamF2 live check:tools/call library {action: add, type: movie, imdb_id: tt0111161}re-adding a soft-deleted item, comparing the captured datastorePut payload's state block against the pre-existing watch stateF3 live check: concurrenttools/call search {query: stall-test}(upstream sleeps 6s) andtools/call tv_control {category: volume, action: up}, measuring per-call latency and response ordering✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.