Remove the unauthenticated read_media p2p request - #325
Draft
arsfeld wants to merge 2 commits into
Draft
Conversation
read_media took a peer-supplied file_path, checked File.exists?, and streamed the bytes back. ReadMediaRequest carries no auth_token, unlike GraphQLRequest and HlsRequest, so nothing in the path checked a credential. The accept loop validates ALPN only and iroh publishes the node ID over DNS discovery, so any peer that could dial the node could read any file the server process could read. The code comment said as much. Nothing calls it. Offline download, the one feature that would want raw file transfer, already runs over an authenticated HLS request that identifies content by job id and resolves the path server-side, which is strictly safer than confining a client-supplied one. serde_cbor tags enum variants by name rather than ordinal, so removing this variant leaves Ping, Pairing, GraphQL, HlsStream and Custom byte-identical on the wire. An older client sending one now gets a decode failure instead of a file.
The read_media handler removed in the previous commit was the sole caller of this
NIF. Removing it leaves std::fs::File and std::io::{Read, Seek, SeekFrom}
unused, which would fail the build under warnings-as-errors, so those imports go
too. std::thread stays; another spawn still uses it.
The Rust function and its Elixir stub have to go together, since the BEAM refuses
to load a native library registering a NIF with no matching Elixir function.
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
1 participant
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.
Closes an unauthenticated arbitrary file read on the p2p transport.
The problem
read_mediaaccepted a peer-suppliedfile_path, checkedFile.exists?, and streamed the bytes back.ReadMediaRequestcarried noauth_tokenfield at all, unlikeGraphQLRequestandHlsRequestwhich both do, so nothing in the path checked a credential. The accept loop validates ALPN and nothing else, and iroh publishes the node ID over DNS discovery, so reachability was not obscure. Any peer able to dial the node could read any file the server process could read.The existing code comment said as much:
# SECURITY: In production, verify path is within allowed directories!Blast radius is bounded to installs that opted into remote access.
remote_access_enableddefaults tofalse(config/config.exs), andMydia.P2p.Serveronly starts when that flag is on. Operators running with remote access enabled should upgrade.Why delete rather than secure it
Nothing calls it. Verified across
.rs,.dart,.exand.exs: the only references were the type definitions, the NIF dispatch arm, and the handler. Git history confirms it was never live in a shipped client,a0799ba8removed the last client-side caller and replaced it withnotImplemented.Offline download, the one feature that would plausibly want raw file transfer over p2p, already runs on a safer path: the player requests a job id over authenticated GraphQL, then fetches via an HLS request carrying an auth token with
session_id: "download:{jobId}", and the server resolves the path from the job rather than accepting one from the client. That is the capability-lookup pattern, strictly better than confining a client-supplied path. Securingread_mediawould have meant maintaining a second, weaker way to do what already works.Changes
Two commits, split so the tree stays both compilable and loadable in between:
ReadMediaRequest,ElixirReadMediaRequest, the NIF dispatch arm,Mydia.P2p.ReadMediaRequest, and the GenServer handler.respond_with_file_chunkNIF and its Elixir stub, plus the twostdimports it was the last user of. These two must land together: the BEAM refuses to load a native library registering a NIF with no matching Elixir function, which fails at load rather than at compile.Compatibility
No window to manage, which matters because this is self-hosted with no coordinated deploy order between server and player.
serde_cbor0.11 with a plain derive uses externally tagged enums keyed by variant name ({"ReadMedia": {...}}), not by ordinal. Confirmed against the vendored crate source:Serializer::newdefaults topacked: false, andserialize_newtype_variantwrites a one-entry map keyed by the variant name. Removing this variant therefore leavesPing,Pairing,GraphQL,HlsStreamandCustombyte-identical on the wire. The fix is server-side only; no client change is needed and none helps.Verification
./dev mix precommitpassed (compile, unused deps, format, credo --strict, tests).6213 tests, 0 failures, 34 skipped (142 excluded).function_exported?(Mydia.P2p, :respond_with_file_chunk, 5)isfalse.#[rustler::nif]functions audited 1:1 against their Elixir stubs; no orphan in either direction.Deliberately out of scope
Three p2p authorization gaps remain, all pre-existing and none a regression from this PR:
handle_download_stream/4streamsjob.output_pathwith no check that the job belongs to the authenticated user. Requires a valid token, and installs are typically single-household.MydiaRequest::Customis reachable without a credential. It reaches no handler and discloses nothing, but each request holds a pending-response entry, a task, and an open QUIC stream for the 30 second timeout.All three are tracked for a follow-up focused on connection-layer authorization.