feat(register): filter device registration by trusted Mostro instance#23
Conversation
Reorders imports and rewraps long lines per default rustfmt. No functional changes. Touched files were unrelated to in-flight feature work but had drifted from rustfmt; folding them into a single formatting commit keeps subsequent feature diffs reviewable.
… pubkey
Mobile clients now declare which Mostro instance they use via a new
optional `mostro_pubkey` field on POST /api/register. The server checks
that value against a compile-time whitelist embedded from
config/trusted_mostro_pubkeys.json, mirroring the trusted instances
already listed in the mobile app
(mobile/lib/core/config/communities.dart).
Behaviour:
- Empty whitelist => permissive mode; the field is ignored. Keeps the
existing JSON shape and lets the binary be built without an explicit
list configured.
- Populated whitelist => the field MUST be present and must match an
entry, otherwise registration is rejected with 403 Forbidden and the
fixed body {"success":false,"message":"Mostro instance not trusted"}.
Malformed values (length / hex) return 400 with a distinct body.
This is an honour-system filter only: there is no cryptographic proof
binding the device to the declared instance. It is intended to keep
well-behaved clients from arbitrary instances out of the push pipeline
and will be hardened once registration carries a daemon-issued
signature.
The previous MOSTRO_PUBKEY environment variable is removed; it was
only used as log context and never as an authors filter on the Nostr
listener (the no-author-filter invariant in CLAUDE.md still holds —
that path is unchanged). Deployment scripts, .env.example and docs
are updated accordingly.
Tests cover trusted, untrusted, missing-field, malformed and
permissive-mode paths. Existing byte-identical fixtures for /api/register
remain green: the new field is request-only and does not affect any
response shape.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (10)
WalkthroughThe PR transitions trusted Mostro instance authentication from a runtime environment variable ( ChangesTrusted Mostro Instance Whitelist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 4ae53735d6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nostr/listener.rs (1)
40-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReconnect delay log is inaccurate on error path
Line 41 says reconnecting in 10 seconds, but this path waits 10s (Line 44) plus the unconditional 5s at Line 47, so effective delay is 15s. This can mislead ops during incidents.
Suggested fix
- error!( - "Error in Nostr listener: {}, reconnecting in 10 seconds...", - e - ); + error!( + "Error in Nostr listener: {}, reconnecting in 15 seconds...", + e + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nostr/listener.rs` around lines 40 - 47, The error log message in the Nostr listener's error handling (the error! macro call) incorrectly states "reconnecting in 10 seconds..." while the code actually awaits sleep(Duration::from_secs(10)).await and then later does an unconditional sleep(Duration::from_secs(5)).await (total 15s); update the error! log to state "reconnecting in 15 seconds..." (or alternatively change the sleeps to match a 10s total) so the message matches the actual delay, referencing the error! call and the two sleep(Duration::from_secs(...)).await calls in the listener loop.docs/api.md (1)
200-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
403to the HTTP status summary table.The
403 Forbiddenresponse introduced by this PR for/api/registeris documented in the endpoint section but missing from the summary table, making the table incomplete.📝 Proposed fix
| 200 | `/api/health`, `/api/info`, `/api/status`, `/api/register`, `/api/unregister` | | 202 | `/api/notify` on parse-valid input | | 400 | Malformed body, invalid `trade_pubkey`, invalid `platform`, empty `token` | +| 403 | `/api/register` — `mostro_pubkey` missing or not on the trusted-instance whitelist | | 429 | `/api/notify` only — per-IP or per-pubkey rate limit | | 500 | `/api/notify` only — fail-closed when the per-IP key cannot be extracted |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api.md` around lines 200 - 208, Update the HTTP status summary table in docs/api.md to include the new 403 status used by the /api/register endpoint: add a row for "403" and list "/api/register" (or "/api/register" and any other endpoints returning 403) under "Used by" so the summary table matches the endpoint documentation; modify the existing table block (the pipe-delimited Status/Used by table) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/routes.rs`:
- Around line 161-191: The whitelist check is case-sensitive while hex::decode
accepts mixed-case; normalize the incoming req.mostro_pubkey to a canonical form
(e.g., lowercase) at the HTTP boundary before format validation and the
contains() check so hex::decode and state.trusted_mostro_pubkeys comparisons use
the same casing; update the logic around req.mostro_pubkey (used in the match)
to lowercase the string prior to length/hex validation and membership testing,
and also ensure trusted_mostro_pubkeys is populated in lowercase (e.g., in
src/main.rs) so the HashSet<String> invariant holds.
In `@src/trusted_pubkeys.rs`:
- Around line 15-25: Normalize trusted pubkeys to lowercase when loading them:
in the load() function that parses TRUSTED_PUBKEYS_JSON, after validating each
pk (hex::decode and len==64) convert it to lowercase before inserting into the
HashSet so all stored entries are canonical lowercase; additionally ensure the
incoming mostro_pubkey value checked with trusted_mostro_pubkeys.contains(...)
in routes.rs is lowercased (or otherwise canonicalized) before the contains call
so comparisons are case-insensitive and avoid false 403s.
- Around line 15-52: There is a case-sensitivity bug in the whitelist check in
src/api/routes.rs at the line where it calls HashSet::contains(). Although the
hex format validation allows both uppercase and lowercase, the contains() check
is case-sensitive and uses the original input string, causing mismatches if the
input uses uppercase letters. To fix this, convert the variable (mostro_pk) to
lowercase using to_ascii_lowercase() before passing it to contains(), ensuring
consistent normalization with the stored lowercase keys.
---
Outside diff comments:
In `@docs/api.md`:
- Around line 200-208: Update the HTTP status summary table in docs/api.md to
include the new 403 status used by the /api/register endpoint: add a row for
"403" and list "/api/register" (or "/api/register" and any other endpoints
returning 403) under "Used by" so the summary table matches the endpoint
documentation; modify the existing table block (the pipe-delimited Status/Used
by table) accordingly.
In `@src/nostr/listener.rs`:
- Around line 40-47: The error log message in the Nostr listener's error
handling (the error! macro call) incorrectly states "reconnecting in 10
seconds..." while the code actually awaits sleep(Duration::from_secs(10)).await
and then later does an unconditional sleep(Duration::from_secs(5)).await (total
15s); update the error! log to state "reconnecting in 15 seconds..." (or
alternatively change the sleeps to match a 10s total) so the message matches the
actual delay, referencing the error! call and the two
sleep(Duration::from_secs(...)).await calls in the listener loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f71673cd-2b62-4f30-99f4-ff556aa94bd8
📒 Files selected for processing (21)
.env.example.gitignoreCLAUDE.mdconfig/trusted_mostro_pubkeys.jsondeploy-fly.shdocs/api.mddocs/configuration.mddocs/deployment.mdsrc/api/mod.rssrc/api/notify.rssrc/api/rate_limit.rssrc/api/routes.rssrc/api/test_support.rssrc/config.rssrc/crypto/mod.rssrc/main.rssrc/nostr/listener.rssrc/push/fcm.rssrc/push/unifiedpush.rssrc/store/mod.rssrc/trusted_pubkeys.rs
💤 Files with no reviewable changes (2)
- docs/deployment.md
- deploy-fly.sh
…ist build Address PR #23 review feedback: - coderabbit (3 instances of the same finding): the 400-format gate uses hex::decode which is case-insensitive, but the 403 whitelist check used byte-exact HashSet::contains. A client sending an uppercase but otherwise valid pubkey would pass the 400 gate and falsely hit 403. Lowercase entries when populating the set in trusted_pubkeys::load and lowercase the incoming mostro_pubkey at the HTTP boundary in register_token before contains. - chatgpt-codex: the test embedded_json_parses_and_validates asserted that the embedded set was non-empty, which broke `cargo test` for an operator who chose the documented permissive configuration (`[]` in config/trusted_mostro_pubkeys.json). Drop the cardinality assertion, drop the redundant default_main_instance_is_present test (it pinned a specific pubkey that should not gate the build), and keep only a format/canonicalization check that succeeds on any valid input including the empty array. Adds a regression test (register_with_uppercase_trusted_mostro_pubkey_succeeds) that submits the uppercase form of a trusted entry and asserts 200.
|
@codex review" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b4e85929c
ℹ️ 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".
PR #23 review (chatgpt-codex, P1): the builder stage copies only Cargo.toml, Cargo.lock and src/ before `cargo build --release`. With the trusted-instances whitelist now embedded via `include_str!("../config/trusted_mostro_pubkeys.json")`, that path is absent inside the image and the build fails. Add a COPY for `config/` ahead of the build step so Fly/Docker deployments compile against the same JSON the developer commits.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Adds a compile-time whitelist of trusted Mostro instance pubkeys (loaded from config/trusted_mostro_pubkeys.json, mirroring the list in mobile/lib/core/config/communities.dart) and a new optional mostro_pubkey field on POST /api/register. When the whitelist is non-empty, registrations whose declared instance is missing or not on the list are rejected with 403 Forbidden; an empty whitelist keeps the endpoint permissive. This implements Option A from the discussion (filter at register, not at the Nostr listener) because the no-author-filter invariant on Gift Wrap events still applies. The previous unused MOSTRO_PUBKEY env var is removed. The filter is honour-system and will be hardened once registration carries a daemon-issued signature. Related: #6.
Summary by CodeRabbit
Release Notes
New Features
mostro_pubkeywhen a whitelist is configured, returning403 Forbiddenfor untrusted instances.Changes
MOSTRO_PUBKEYenvironment variable with compile-time configuration from a trusted instances list.Documentation