fix(nip59): authenticate daemon via seal identity, not rumor sender - #104
Conversation
The previous check compared the active Mostro pubkey against the rumor sender, which is self-asserted whenever the inner signature is absent — the common case for protocol responses. An attacker could seal a wrap with their own key and set the rumor pubkey to the configured Mostro key, bypassing the check. Authenticate against `identity` (the seal signer, verified inside `unwrap_message`) and reject unsigned rumors whose `sender` diverges from the verified `identity`.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes update NIP-59 message authentication in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as External Client
participant GiftWrap as Gift Wrap Handler
participant Dispatch as Dispatch Handler
participant Verifier as Identity Verifier
Client->>GiftWrap: Send wrapped message
activate GiftWrap
GiftWrap->>GiftWrap: Unwrap message<br/>(extract sender, identity, signature)
GiftWrap->>Verifier: Verify seal signature
activate Verifier
Verifier-->>GiftWrap: ✓ Seal verified against identity
deactivate Verifier
GiftWrap-->>Dispatch: Return UnwrappedMessage
deactivate GiftWrap
activate Dispatch
alt Inner signature missing AND sender ≠ identity
Dispatch->>Dispatch: Reject message
Dispatch-->>Client: ✗ Rumor origin integrity check failed
else Valid configuration
Dispatch->>Dispatch: Match identity against<br/>Mostro pubkey
alt Identity matches Mostro
Dispatch-->>Client: ✓ Route message
else Identity mismatch
Dispatch-->>Client: ✗ Reject message
end
end
deactivate Dispatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d3dab2a7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // it (checked inside `unwrap_message`). Without a signature, a `sender` | ||
| // that diverges from the verified `identity` is an unauthenticated claim | ||
| // — drop it rather than route a message whose origin we cannot prove. | ||
| if signature.is_none() && sender != identity { |
There was a problem hiding this comment.
After authenticating the wrap against identity == active_mostro_pubkey, this new guard drops any unsigned message where sender != identity. In Mostro's reputation-mode key split, sender (trade key) and identity (seal key) are expected to differ, and responses can legitimately be unsigned; in that case this returns early and prevents valid daemon updates (including order status/confirmation flows) from being dispatched even though the origin is already authenticated by the seal signature.
Useful? React with 👍 / 👎.
The previous guard rejected any unsigned wrap whose rumor sender differed from the seal identity. In Mostro's reputation-mode key split `sender` (per-trade key) and `identity` (long-lived seal key) are expected to differ, and protocol responses commonly omit the inner signature, so the check dropped legitimate daemon updates (order status, confirmations) even though the seal signature already authenticates the origin against the active Mostro pubkey. Authentication now relies solely on `identity == active_mostro_pubkey` — a forger who seals with their own key cannot make it match — and the unauthenticated `sender` is no longer used to gate dispatch.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/orders.rs (1)
1049-1053:⚠️ Potential issue | 🟡 MinorStale docstring — still says "Authenticates the sender".
The function-level doc above the changed body now contradicts the implementation: authentication is performed against the seal signer's
identity, notsender. Consider tightening the wording so the contract matches the new behavior (and the clarifiedunwrap_mostro_messagedoc inrust/src/nostr/gift_wrap.rs).📝 Suggested doc tweak
/// Dispatch a Mostro `Message` recovered from a gift-wrap. /// -/// Authenticates the sender against the active Mostro pubkey, runs the -/// centralized `validate_response` check (catches `CantDo` responses and -/// malformed `request_id` fields), then routes by action. +/// Authenticates the seal signer (`identity`) against the active Mostro +/// pubkey — never the unauthenticated rumor `sender` — and additionally +/// rejects unsigned rumors whose `sender` diverges from the verified +/// `identity`. Then runs the centralized `validate_response` check (catches +/// `CantDo` responses and malformed `request_id` fields) and routes by action.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 1049 - 1053, Update the function docstring for the dispatch implementation in rust/src/api/orders.rs so it accurately describes that authentication is performed against the seal signer's identity (not the message sender); mention that it uses unwrap_mostro_message (see unwrap_mostro_message in rust/src/nostr/gift_wrap.rs) to recover the Message and validate the seal signer, then runs validate_response and routes by action — keep the wording concise and aligned with the current implementation (referencing the seal signer/identity rather than sender).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/api/orders.rs`:
- Around line 1049-1053: Update the function docstring for the dispatch
implementation in rust/src/api/orders.rs so it accurately describes that
authentication is performed against the seal signer's identity (not the message
sender); mention that it uses unwrap_mostro_message (see unwrap_mostro_message
in rust/src/nostr/gift_wrap.rs) to recover the Message and validate the seal
signer, then runs validate_response and routes by action — keep the wording
concise and aligned with the current implementation (referencing the seal
signer/identity rather than sender).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a84dd32-adf6-421c-ad1d-167e79675e70
📒 Files selected for processing (2)
rust/src/api/orders.rsrust/src/nostr/gift_wrap.rs
The docstring claimed authentication was performed against the rumor sender, but the implementation authenticates against the seal signer's identity. Update the wording to match: reference unwrap_mostro_message as the source of the verified unwrap and clarify that identity (never sender) is checked against the active Mostro pubkey.
1 participant
The previous check compared the active Mostro pubkey against the rumor sender, which is self-asserted whenever the inner signature is absent — the common case for protocol responses. An attacker could seal a wrap with their own key and set the rumor pubkey to the configured Mostro key, bypassing the check.
Authenticate against
identity(the seal signer, verified insideunwrap_message) and reject unsigned rumors whosesenderdiverges from the verifiedidentity.Summary by CodeRabbit