Make the relay double capable of lying - #36
Merged
Merged
Conversation
`FakeRelay` can only produce `["OK", <id>, <bool>, <str>]` -- the exact shape `NostrChannel` handles correctly, and therefore the shape that hides every way it does not. Every gap found reviewing #35 lives outside what that double can express, which is why a thorough suite was silent about all of them. `ScriptedRelay` hands the raw frame to the test instead. A relay is infrastructure we don't run: its *content* is already treated as hostile in `_classify_note`, and its *shape* had no such treatment. The nine cases that fail are marked `xfail(strict=True)` rather than left red, so this lands green and each marker deletes itself the moment its defect is fixed -- a non-strict xfail would outlive the bug it describes. Two of them needed care to be worth having. The malformed-OK-flag test first asserted `retriable` too, which a correct fix can defensibly set either way; it now asserts only the false-success. The no-deadline test measured frames rather than seconds, so an instant in-process double satisfied it by running out of script -- `ScriptedRelay.delay` makes the wall clock move, since a deadline that isn't measured in time isn't the thing being tested. Verified in both directions: all nine XFAIL against this branch, and all nine XPASS(strict) against a patched channel, so none is red for an unrelated reason and none would stay red after the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
magicka7
added a commit
that referenced
this pull request
Aug 13, 2026
#36 landed ScriptedRelay and nine xfail(strict=True) cases for the same findings this branch fixes -- by design, each marker was meant to delete itself the moment its defect was. Verified all nine flip from XFAIL to XPASS(strict) against the fix, then removed the now-satisfied markers. Also drops the FakeRelay.recv_returns shim and the duplicate malformed- frame tests added before the rebase picked up #36 -- ScriptedRelay already covers the same ground with a finer-grained double. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wired4ncer
added a commit
that referenced
this pull request
Aug 13, 2026
Two review findings on #35, both the same shape: an exception escaping `send`. That is worse than any verdict `send` could return, because the caller expected a `DeliveryResult` and gets an unhandled alarm instead -- and the loop never reaches the relays listed after the one that threw. A malformed reply is a retriable failure; it is not an exit. `except json.JSONDecodeError` covered a frame that isn't JSON, but not a frame that isn't *text*. websocket-client returns raw bytes for a BINARY opcode, and `json.loads` on bytes that aren't valid UTF-8 raises `UnicodeDecodeError` -- a ValueError, but not that subclass, and `send`'s `(OSError, WebSocketException)` handler doesn't catch it either. A TEXT frame carrying invalid UTF-8 raises the same thing one layer earlier, inside `recv` itself, which the widened `except ValueError` also covers because the call already sits inside that `try`. `RelayConnection.recv` is now typed `str | bytes` to say so, and `ScriptedRelay` can script a bytes frame -- a double that can only emit `str` can only test the case that already works, the same argument #36 made for its shape. The constructor validated the `wss://` prefix and nothing after it, so `wss://relay.example:99999`, `wss://[bad` or a truncated `wss://` constructed cleanly and then raised a plain `ValueError` out of `create_connection` on the alarm path. Checked with stdlib `urlsplit`, which rejects the same shapes websocket-client's own parser does without reaching into its private `_url` module. That is the posture the prefix check already states -- rejected at construction, not discovered mid-publish -- applied to the rest of the URL. Two mutations added, one per finding, and a test that a relay on a non-default port with a path still constructs, so the URL check can't pass by rejecting everything. Sweep is 76 now; ci.yml's count updated with it. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
magicka7
added a commit
that referenced
this pull request
Aug 13, 2026
* Give alerts a second rail that no relay can read Implements NostrChannel against the Channel protocol (#3): NIP-17 gift-wrapped DMs, sealed with the service's one persistent identity and wrapped under a fresh, single-use key per message so no relay can link two alerts to the same sender. Adds coincurve (secp256k1 ECDH + Schnorr) and websocket-client as the two dependencies this genuinely needed -- bech32, ChaCha20, HKDF and the NIP-44 padding scheme stay hand-rolled stdlib, each checked against the spec's own published test vectors: NIP-19's npub examples, RFC 8439's ChaCha20 vectors, NIP-44's full 126-vector suite, and NIP-59's real worked example -- decrypting a gift wrap nostr-tools actually produced, to prove interop rather than only self-consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Teach the relay double to misbehave, and mark what breaks when it does `FakeRelay` can only produce `["OK", <id>, <bool>, <str>]` -- the exact shape `NostrChannel` handles correctly, and therefore the shape that hides every way it does not. Every gap found reviewing #35 lives outside what that double can express, which is why a thorough suite was silent about all of them. `ScriptedRelay` hands the raw frame to the test instead. A relay is infrastructure we don't run: its *content* is already treated as hostile in `_classify_note`, and its *shape* had no such treatment. The nine cases that fail are marked `xfail(strict=True)` rather than left red, so this lands green and each marker deletes itself the moment its defect is fixed -- a non-strict xfail would outlive the bug it describes. Two of them needed care to be worth having. The malformed-OK-flag test first asserted `retriable` too, which a correct fix can defensibly set either way; it now asserts only the false-success. The no-deadline test measured frames rather than seconds, so an instant in-process double satisfied it by running out of script -- `ScriptedRelay.delay` makes the wall clock move, since a deadline that isn't measured in time isn't the thing being tested. Verified in both directions: all nine XFAIL against this branch, and all nine XPASS(strict) against a patched channel, so none is red for an unrelated reason and none would stay red after the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trust the relay's shape as little as its content PR review on #35 found seven bugs on the path where a relay's frame enters _publish_to unchecked: an uncaught JSONDecodeError on relay disconnect, a truthy-string OK flag reporting a rejected event as delivered, a CLOSED branch comparing against the wrong id so it could never fire, no overall deadline on the receive loop, a non-string OK message crashing classification, an off-curve npub accepted at enrolment, and a truncated private key silently booting a different identity. Fixes each with regression tests built on a FakeRelay that can now return malformed frames. The eighth finding -- delivery to one service-wide relay rather than the recipient's own NIP-17 kind:10050 list -- is a feature gap, not a bug, so it's recorded as an accepted residual in SECURITY.md and docs/architecture.md instead of patched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Delete the xfail markers the fix just satisfied #36 landed ScriptedRelay and nine xfail(strict=True) cases for the same findings this branch fixes -- by design, each marker was meant to delete itself the moment its defect was. Verified all nine flip from XFAIL to XPASS(strict) against the fix, then removed the now-satisfied markers. Also drops the FakeRelay.recv_returns shim and the duplicate malformed- frame tests added before the rebase picked up #36 -- ScriptedRelay already covers the same ground with a finer-grained double. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Refuse a relay reply that isn't text, and a URL that isn't a URL Two review findings on #35, both the same shape: an exception escaping `send`. That is worse than any verdict `send` could return, because the caller expected a `DeliveryResult` and gets an unhandled alarm instead -- and the loop never reaches the relays listed after the one that threw. A malformed reply is a retriable failure; it is not an exit. `except json.JSONDecodeError` covered a frame that isn't JSON, but not a frame that isn't *text*. websocket-client returns raw bytes for a BINARY opcode, and `json.loads` on bytes that aren't valid UTF-8 raises `UnicodeDecodeError` -- a ValueError, but not that subclass, and `send`'s `(OSError, WebSocketException)` handler doesn't catch it either. A TEXT frame carrying invalid UTF-8 raises the same thing one layer earlier, inside `recv` itself, which the widened `except ValueError` also covers because the call already sits inside that `try`. `RelayConnection.recv` is now typed `str | bytes` to say so, and `ScriptedRelay` can script a bytes frame -- a double that can only emit `str` can only test the case that already works, the same argument #36 made for its shape. The constructor validated the `wss://` prefix and nothing after it, so `wss://relay.example:99999`, `wss://[bad` or a truncated `wss://` constructed cleanly and then raised a plain `ValueError` out of `create_connection` on the alarm path. Checked with stdlib `urlsplit`, which rejects the same shapes websocket-client's own parser does without reaching into its private `_url` module. That is the posture the prefix check already states -- rejected at construction, not discovered mid-publish -- applied to the rest of the URL. Two mutations added, one per finding, and a test that a relay on a non-default port with a path still constructs, so the URL check can't pass by rejecting everything. Sweep is 76 now; ci.yml's count updated with it. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Wired4ncer <102553581+Wired4ncer@users.noreply.github.com> Co-authored-by: Wiredancer <wiredancer@reelnetwork.eu>
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.
Tests only — no change to
src/. Base isfeat/nostr-channel, so this lands into #35 rather than around it.What this is
The review on #35 found eight issues, seven of them at the relay boundary. This turns the frame-shaped ones into tests, and fixes the double that let them through in the first place.
FakeRelaycan only ever produce["OK", <id>, <bool>, <str>]. That is precisely the shapeNostrChannelhandles correctly, so a suite built on it can only test the case that already works — which is why a genuinely thorough set of tests was silent about all of this.ScriptedRelayhands the raw frame to the test instead, withok_frame()/closed_frame()builders left deliberately untyped so a test can send what a relay that disagrees with NIP-01 would send.The script is finite on purpose: when it runs out,
recvraises theWebSocketTimeoutExceptiona real socket raises once a relay goes quiet. That is what lets a test assertsendstopped reading early without any test being able to hang if it doesn't.The nine cases
All marked
xfail(strict=True), so this merges green:json.loadson the""websocket-client returns for a CLOSE opcode; the second checks relay B still gets triedJSONDecodeErroris aValueError, so it passes throughsend's except clause"false"bool("false")isTrue— a rejection read as a delivered alarm_classify_notecalls.spliton itCLOSEDframeCLOSEDcarries a subscription idtimeoutbounds onerecv, not the loop around itThe other four parameters of the not-JSON case are unmarked and pass today —
[], a non-list,null,42are already handled by the existingisinstance(frame, list)guard. Keeping them visible is the point: the guard exists, it is simply downstream of the parse that fails first.On
strict=TrueDeliberate. When a defect is fixed the test starts passing, pytest reports the XPASS as a failure, and the marker has to be deleted. A non-strict xfail would let these quietly outlive the bugs they describe, which is the failure mode that makes a suite lie about its own coverage.
Two tests that needed a second pass
Worth recording, because both were wrong in the direction that would have wasted your time:
retriable is True. But reading that note at face value givesinvalid, which is permanent, and distrusting the whole frame gives retriable — both defensible. It would have failed a correct fix for disagreeing about something the test isn't about. It now asserts only the false-success.ScriptedRelay.delaymakes time actually pass, and the test now runs a 0.05s timeout against a relay that takes 0.002s per frame.Both were caught by running the suite against a patched channel rather than assuming the markers were right.
Verified in both directions
ruffclean.channel.pyfixing all five frame-handling defects: all 9 flip to XPASS(strict). So none is red for an unrelated reason, and none would stay red once fixed.That patch is not included here — the fixes are yours to write, and these are meant to be the tests you write them against.
One thing this doesn't cover
tools/mutate.pyhas 7 mutations forchannels/email.pyand zero forchannels/nostr/. The greenmutationscheck on #35 re-ran a sweep that doesn't touch a line of the new module, so it attested to nothing about it. Same gap asfake_relay.py, one layer up — worth its own issue rather than smuggling it in here.The two non-frame findings from the review (
validate_destaccepting a non-curve npub, and the missing private-key length check) aren't in this PR either. Neither is frame-shaped, and both want a test next to the code that fixes them.🤖 Generated with Claude Code