fix: close the two findings the security review left open

[jonaswre]

The two findings from the max-effort review of #24 that were reported as
skipped, both because the fix looked larger than that PR's scope. Looking
at them properly, both are contained.

execute_push_sink reported success for a refused push

execute_push_sink opened a bidirectional stream and immediately stopped the
receive half — "we are not going to need this!" — then returned
Ok(Stats::default()) however the push went.

That half is the only channel a refusal travels on. When the event mask, an
interceptor or a rate limit turns a push down, the provider resets it with the
reason code (handle_read_request_result → writer.reset(e.code())). The
provider side was already correct; the client threw the answer away before the
request had even been written.

The consequence lands on exactly the change #24 made: a node that upgrades to an
enforced EventMask::DEFAULT (push: Disabled) keeps reporting that its pushes
worked, while the receiver drops every byte.

Fix. Keep the stream and read it once the request is written: cleanly closed
means accepted, reset means refused, and the code says which. Compatible in both
directions — a provider from before this drops its half without writing, which
arrives as a clean end of stream and reads as acceptance, the same answer it gave
before.

Behaviour change worth a release note. Pushes are now acknowledged, so
execute_push returns once the blob has landed rather than once the last byte
reached the socket.

One undecodable frame ended the sending client's relay session

handle_frame turned any decode error into a fatal one, ending run_inner and
tearing the connection down. The client is relaying for every peer it is talking
to, so one unparseable frame cost all of that instead of one datagram.

It is also the inverse of what the send side already does since 519a645: an
unencodable packet is dropped, precisely because disconnecting over it would be
out of proportion. The receiving client got that leniency; the sending one did
not.

Fix. RecvError already separates the cases. Proto means this frame is
unusable while the connection underneath is fine — drop it, count it, carry on.
StreamError means there is nothing left to keep, so it stays fatal.

Nothing bounds how many a client may send, deliberately: the read side is already
rate limited, so a client sending malformed frames gets no more of this server
than one sending valid ones. The new recv_frames_invalid counter is what makes
it visible; sustained growth there is a version mismatch or a client-side bug.

Tests

Both new assertions were verified against the defect rather than just against
green:

• Neutralising only the push acknowledgement puts the deny-side result back to
  Ok(Stats { .. }) and fails the test on it.
• Neutralising only the Proto/StreamError split leaves the Ping queued behind
  the bad frame unanswered — "Unexpected EOF, expected frame Pong" — because the
  session is already gone.

The push test also lost the one-second polling window it needed while a refusal
was unobservable; the acknowledgement is the synchronisation now.

cargo fmt, cargo clippy --all-features --tests --examples, and the full
krikos-relay and krikos-blobs suites pass. Determinism baselines re-based for the
one new test-only spawn, classified under the existing #[cfg(test)] row.

Change checklist

• Tests if relevant.
• All breaking changes documented.
• Self-review.

🤖 Generated with Claude Code

[github-actions]

Documentation for this PR has been generated and is available at: https://holon-technologies.github.io/iroh/pr/25/docs/krikos/

Last updated: 2026-08-05T07:27:33Z