fix(mediorum): fail closed when the cidstream access lookup errors - #503
Open
rickyrombo wants to merge 1 commit into
Open
fix(mediorum): fail closed when the cidstream access lookup errors#503rickyrombo wants to merge 1 commit into
rickyrombo wants to merge 1 commit into
Conversation
The cidstream authorization path issued its GORM raw queries and discarded res.Error. A failed query left trackID empty and managementKeyCount zero, which requireRegisteredSignature reads as "this track has no access authorities" -- so it skipped the access-authority check entirely and fell through to the registered-validator-signature path. Any registered signer could stream a gated track. Reproduced by pointing one query at a nonexistent table: the request was authorized rather than rejected. Check res.Error at every site and deny instead. The uncached lookup moves into lookupTrackAccessInfo, which returns an error rather than a zero-valued struct the caller cannot distinguish from an ungated track, and its result is only cached on success so a transient failure is not memoized for five minutes. serveTrack and streamTrackGRPC get the same treatment; their swallowed errors surfaced as "track not found" and "signer not authorized" rather than as failures. The track-duration lookup stays non-fatal: it only sizes the presigned URL expiry, and one malformed transcode_results row fails the whole ::jsonb cast, which should not take streaming down. Note this is a real behavior change for a node whose database lacks core's sound_recordings table -- it now denies cidstream requests instead of silently treating every track as ungated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The bug
The cidstream authorization path issued its GORM raw queries and discarded
res.Error:If either query errors — table missing, connection blip, permissions —
trackIDstays""andmanagementKeyCountstays0.requireRegisteredSignaturereads that as "this track has no access authorities", skips the access-authority check entirely, and falls through to the registered-validator-signature path. A gated track becomes streamable by any registered signer.This was reproduced empirically while working on #502: deliberately pointing one literal at a nonexistent table produced exactly that bypass rather than an error.
The fix
Every site now checks
res.Errorand denies.lookupTrackAccessInfo, which returns an error rather than a zero-valued struct the caller cannot tell apart from an ungated track. Its result is cached only on success, so a transient failure is not memoized for five minutes.serveTrackandstreamTrackGRPCget the same treatment. Their swallowed errors previously surfaced astrack not foundandsigner not authorized— plausible-looking answers derived from a query that never ran."unable to verify track access"); the driver error goes to aWarnlog rather than to an unauthenticated caller.The track-duration lookup stays non-fatal on purpose: it only sizes the presigned-URL expiry, not the access decision, and one malformed
transcode_resultsrow fails the whole::jsonbcast — that should not take streaming down.Tests
Eight new tests, across all three functions. Two failure injectors:
failEveryQuery— a cancelled-context gorm handle, standing in for the database being unreachable.hideManagementKeys— renames the table away while leavingsound_recordingsintact, which is the shape hit on refactor(core): prefix chain-derived tables with core_ #502: the cid→track_id lookup still succeeds and only the authority queries error.The central test gates a track behind an unrelated authority and registers the fixture signer as a validator, so a fail-open lookup sails straight through the no-access-authorities branch. That is the bypass; it now returns 500.
All eight fail without the source change and pass with it.
serveTrackandstreamTrackGRPChad no coverage at all before this.Behavior change worth a look before merging
Failing closed turned two pre-existing tests red, for a real reason rather than a test artifact. They run against a server whose database has no
sound_recordingstable, so under the old code every request there silently took the ungated path.sound_recordingsandmanagement_keyscome from core's migrations (pkg/core/db/sql/migrations/00018_release_metadata.sql), not mediorum's. Same shared database in production, so they normally exist — but a node where mediorum serves before core's migrations have run now denies cidstream traffic instead of degrading to validator-signature auth. That is the correct security posture and I would keep it, but it is a genuine availability edge on fresh-node bootstrap, and it argues for watching thetrack access lookup failedwarn line after rollout.Conflicts
Touches the same lines #502 renames (
sound_recordings→core_sound_recordings,management_keys→core_management_keys). Whichever lands second needs a rebase.🤖 Generated with Claude Code