diff --git a/CHANGELOG.md b/CHANGELOG.md index 982a5bc..a50e6d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- [`PR-REVIEW.md`](PR-REVIEW.md) — the 13-criteria PR review checklist + the maintainer runs against every incoming PR, published at the + repo root so contributors can run the same pass on their own change + (directly or by handing the document to a coding agent) before + opening. Linked from `CONTRIBUTING.md` under "Submitting pull + requests" and "Further reading". Running the checklist before + opening surfaces problems that would otherwise come back as review + comments, saving a round trip. + ### Changed - Nostr discovery startup is now non-blocking. `Node::start` no @@ -92,6 +103,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Nostr discovery: filter unroutable direct UDP/TCP advert endpoints. + Publisher and validator now retain only endpoints that parse as + concrete socket addresses with routable IPs and nonzero ports. + `udp:nat` rendezvous endpoints and Tor endpoints pass through + unchanged. Adverts that collapse to zero usable endpoints after + filtering are rejected with a clear "missing publicly routable + endpoints" error. Before this change, misconfigured nodes could + publish RFC1918, loopback, link-local, CGNAT 100.64/10, IPv6 ULA, + or IPv6 link-local endpoints into Nostr discovery, and consumers + would cache and dial them; in mixed LAN/VPN/NAT environments, that + could prefer a misleading one-way private path over the intended + `udp:nat` bootstrap. - Coord cache invalidation made surgical at parent-position-change and root-change sites. Replaces the previous unconditional `CoordCache::clear()` calls with two targeted methods: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a616897..2583bfc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -148,6 +148,20 @@ that touches your code path. This is the same matrix that runs on GitHub Actions. Catching a regression locally is much cheaper than catching it in CI. +### Self-review against the project review checklist + +The 13-criteria checklist the maintainer runs on every incoming PR is +published at [PR-REVIEW.md](PR-REVIEW.md). Run your own change through +it before opening — or hand the document to your coding agent with +"review my branch against this checklist" and let it do the pass. The +checklist covers PR hygiene (body, commit shape, base freshness), diff +content (does the change do what the description says, does it fit the +codebase as a natural extension), and cross-cutting concerns (tests, +docs, dependencies, security, contributor-conventional Rust patterns). + +This is the first thing the maintainer does on any submission, so +running it yourself saves a review round trip. + ### Additional requirements for feature PRs - **New CI coverage.** Features added without a test that exercises @@ -233,6 +247,9 @@ home yet, file a GitHub issue with the `design` label. ## Further reading +- [PR-REVIEW.md](PR-REVIEW.md) — the 13-criteria PR review checklist + the maintainer runs on every incoming PR; run it yourself before + opening to save a round trip. - [docs/design/README.md](docs/design/README.md) — protocol design tree. - [docs/branching.md](docs/branching.md) — full release workflow and merge-direction rationale. diff --git a/PR-REVIEW.md b/PR-REVIEW.md new file mode 100644 index 0000000..a6f6d1c --- /dev/null +++ b/PR-REVIEW.md @@ -0,0 +1,199 @@ +# PR Review Checklist + + + +This is the 13-criteria checklist the maintainer runs against every +incoming PR. The first pass on any submission is exactly this list, +so executing it yourself before opening — or after pushing a fresh +revision — saves a review round trip and surfaces problems faster. + +The document is also written so you can hand it to a coding agent +(Claude Code, Copilot, Cursor, Aider, etc.) with "review my branch +against this checklist" and get a structured pass. The agent gets +better results than a free-form "review my PR" because every concern +the maintainer cares about is enumerated below. + +## Step 1 — Should this even be reviewed? + +Skip the review (and say so) if the PR is: + +- closed, merged, or marked draft +- automated (bot author, dependabot, etc.) and trivially OK +- so small and obviously correct (typo fix, single-line doc tweak) + that a thirteen-point pass is overkill — a one-paragraph informal + review is better in that case + +## Step 2 — Gather context + +Read these *before* analyzing the diff so the review is grounded: + +1. PR metadata. Title, body, author, head ref, base ref, head SHA, + base SHA, mergeable status, CI rollup, commit list. + + ```bash + gh pr view --json title,body,author,headRefName,baseRefName,headRefOid,baseRefOid,mergeable,statusCheckRollup,commits + ``` + +2. The diff. + + ```bash + gh pr diff + ``` + +3. Base-branch freshness. How many commits have landed on the PR's + base since the PR forked from it. +4. Project guidance. Read [CLAUDE.md](CLAUDE.md) at the repo root and + any nested `CLAUDE.md` in directories the diff touches. These + describe project-specific conventions and constraints not visible + from the diff alone. +5. Related work on GitHub. Skim the [open issues](https://github.com/jmcorgan/fips/issues) + and other [open PRs](https://github.com/jmcorgan/fips/pulls) for + work that overlaps, duplicates, partially addresses, or is unblocked + by this PR. +6. For "this looks wrong" observations later: `git blame` the modified + lines and read recent commit history on the same files for context + before flagging something as a problem. What looks like a bug at + first glance is often a deliberate workaround documented in a prior + commit message. + +## Step 3 — The 13 criteria + +The review must address all 13 criteria below at some point. They +group naturally into PR hygiene, diff content, and cross-cutting +concerns — but the report itself is *not* organized this way; see +Step 4. + +### Group A — PR hygiene (structural review) + +1. **PR body and issue cross-reference**. Does the body accurately + describe the change (feature added or bug fixed) and match what + the diff actually does? Is there an associated issue that + should be referenced via `Closes #N` / `Fixes #N`? +2. **Commit hygiene and base freshness**. Is the PR a clean set of + commits (or a single commit) representing appropriately chunked + development items, or are there intermediate "WIP" / "fix typo" / + "address review" commits that should have been squashed? Is the + branch based off a recent `maint` / `master` / `next`, or has the + base diverged far enough that rebase work is needed? +3. **Commit message quality**. Are the commit messages well-structured + (subject + body where the change warrants), accurately referencing + everything actually in each commit, and free of extraneous footers + — particularly coding-assistant attribution (`Generated with + Claude Code`, `Co-Authored-By: Claude`, similar from other AI + tools)? + +### Group B — Diff content + +4. **Does it do what it says it does**. Walk each claimed behavior + from the PR body against the actual diff lines. +5. **Coherent whole**. Are all parts of the diff in service of the + stated goal, or are there drive-by formatting changes, unrelated + touch-ups, or scope creep? +6. **Fits the codebase as a natural extension**. Does the new code + use existing idioms, helpers, error types, and patterns, or does + it introduce new ones where existing ones would have served? + +### Group C — Cross-cutting concerns + +7. **New dependency surface**. Any new crates, system deps, + build-time requirements, or external-service dependencies? +8. **New test coverage**. Are the new code paths covered, are the + tests scoped correctly (unit / integration / end-to-end), and + are there obvious test gaps? Don't reflag anything CI already + enforces (formatting, lint, type errors, unit-test pass/fail). +9. **Documentation impact**. Does this need a CHANGELOG entry, + rustdoc updates, design-doc changes + ([docs/design/](docs/design/)), README adjustments, or operator + doc updates in [docs/](docs/)? +10. **Security vulnerabilities**. Any new attack surface, + untrusted-input parsing, `unsafe` blocks, panic-on-untrusted + paths, secret-handling concerns, or side-channel exposure? +11. **Rust and OSS best practices**. Idiomatic error handling, no + silently-swallowed errors, no `unwrap` / `expect` on untrusted + input, no `#[allow]` without justification, appropriate + visibility (`pub` vs `pub(crate)` vs private), naming, and + module shape. +12. **Overlap with existing work**. Cross-check open issues and + other open PRs (and recently closed/merged ones) for related + work that overlaps, duplicates, partially addresses, or is + unblocked by this PR. +13. **Other concerns**. Anything not captured above — wire-format + implications, branch-flow questions (`maint` vs `master` vs + `next`; see [docs/branching.md](docs/branching.md)), + deployment / packaging impact, contributor coordination needs, + fragility notes for future maintainers. + +## Step 4 — Compose the review + +The review report is **not** a Q&A walk through the 13 criteria. +Write it as natural prose in a coherent, integrated narrative that +reads start-to-finish. All 13 criteria must be addressed at some +point in the body, but ordering, grouping, and emphasis follow the +actual shape of THIS PR — lead with what matters most for this PR, +not a fixed template. + +A typical shape that often falls out naturally: + +- **Opening paragraph**: what the PR does and the headline + observations (subsumes criteria 1 and 4). +- **Substantive body**: diff analysis, design fit, cross-cutting + concerns, surprises, fragilities, missing coverage, + cross-PR/issue overlap, anything unusual. Don't reference + criterion numbers in the prose. +- **Closing**: short summary and a proposed disposition — *land*, + *land-with-followups* (list them), *request-changes* (with the + blocking items called out), or *hold-for-thematic-batch*. + +Short subheadings are fine where they aid scanning. Bullets are fine +for enumerable items (test names, file paths, follow-up actions). +Avoid bullets that just enumerate criterion responses. + +## Step 5 — Filter aggressively + +Quality over quantity. Do not flag: + +- Pre-existing issues on lines the PR did not modify +- Issues that linter, type-checker, formatter, or CI would catch +- Pedantic style nitpicks a senior engineer would not call out +- Likely intentional changes related to the broader goal +- Things explicitly silenced by an `#[allow]` with justification +- Stylistic preferences not anchored in `CLAUDE.md` or the + surrounding codebase's idioms + +When in doubt about whether something is worth surfacing: would a +senior maintainer skim past it, or would they want it raised? +Skim-past items don't belong in the report. + +For every issue you *do* surface, include a concrete fix suggestion +inline ("rename X to Y", "extract this into the existing helper at +`foo.rs:42`", "add a test exercising the `Err` branch") so the +author can act without a round-trip. + +## Step 6 — Citation discipline + +When the review references a specific code location, use full-SHA +GitHub permalinks so the link survives future history rewrites: + +```text +https://github.com/jmcorgan/fips/blob//#L-L +``` + +For multi-line ranges include at least one line of context before +and after the line(s) being discussed. After `gh pr checkout `, +use `git rev-parse HEAD` to grab the full SHA — never partial SHAs +in permalinks. + +## Notes + +- The review is one human's read of the PR. Confidence calibration + matters: distinguish "this is a blocker" from "this is worth asking + about" from "this is a fragility note for future maintainers." The + closing disposition makes the action explicit. +- If a re-review is triggered after the author pushes new commits, + lead with the delta from the prior review rather than re-walking + the whole PR. +- This checklist exists to surface problems, not to assign blame. + If you're running it as the author or via an agent, treat each + finding as "would the maintainer ask about this?" — and either fix + it before opening, or pre-empt it in the PR body so the maintainer + doesn't have to ask. diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index b6b15a2..aceb7e2 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -57,6 +57,67 @@ fn endpoint_summary(endpoints: &[OverlayEndpointAdvert]) -> String { .join(",") } +fn is_unroutable_direct_advert_ip(ip: std::net::IpAddr) -> bool { + match ip { + std::net::IpAddr::V4(v4) => { + v4.is_private() + || v4.is_loopback() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_multicast() + || v4.is_broadcast() + || v4.is_documentation() + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xc0) == 64) + } + std::net::IpAddr::V6(v6) => { + v6.is_loopback() + || v6.is_unspecified() + || v6.is_unique_local() + || v6.is_multicast() + || (v6.segments()[0] & 0xffc0) == 0xfe80 + } + } +} + +fn endpoint_advert_is_publicly_usable(endpoint: &OverlayEndpointAdvert) -> bool { + let addr = endpoint.addr.trim(); + if addr.is_empty() { + return false; + } + + if endpoint.transport == super::types::OverlayTransportKind::Udp + && addr.eq_ignore_ascii_case("nat") + { + return true; + } + if addr.eq_ignore_ascii_case("nat") { + return false; + } + + match endpoint.transport { + super::types::OverlayTransportKind::Udp | super::types::OverlayTransportKind::Tcp => { + let Ok(socket_addr) = addr.parse::() else { + let Some((host, port)) = addr.rsplit_once(':') else { + return false; + }; + let host = host.trim().trim_start_matches('[').trim_end_matches(']'); + if host.is_empty() || port.trim().parse::().ok().is_none_or(|p| p == 0) { + return false; + } + if host.eq_ignore_ascii_case("localhost") { + return false; + } + return host + .parse::() + .ok() + .is_none_or(|ip| !is_unroutable_direct_advert_ip(ip)); + }; + socket_addr.port() != 0 && !is_unroutable_direct_advert_ip(socket_addr.ip()) + } + super::types::OverlayTransportKind::Tor => true, + } +} + /// Cached STUN-derived public address for an advert-eligible UDP transport /// bound to a wildcard. Lives on `NostrDiscovery` so the freshness window /// survives advert refresh cycles. @@ -851,6 +912,7 @@ impl NostrDiscovery { advert.identifier = ADVERT_IDENTIFIER.to_string(); advert.version = ADVERT_VERSION; + advert.endpoints.retain(endpoint_advert_is_publicly_usable); // Defensive: build_overlay_advert returns None on empty endpoints, // so this is only reachable from non-lifecycle callers. if advert.endpoints.is_empty() { @@ -1393,12 +1455,11 @@ impl NostrDiscovery { "missing required endpoints".to_string(), )); } - for endpoint in &advert.endpoints { - if endpoint.addr.trim().is_empty() { - return Err(BootstrapError::InvalidAdvert( - "endpoint addr cannot be empty".to_string(), - )); - } + advert.endpoints.retain(endpoint_advert_is_publicly_usable); + if advert.endpoints.is_empty() { + return Err(BootstrapError::InvalidAdvert( + "missing publicly routable endpoints".to_string(), + )); } let has_nat = advert.has_udp_nat_endpoint(); diff --git a/src/discovery/nostr/tests.rs b/src/discovery/nostr/tests.rs index b412058..d28201c 100644 --- a/src/discovery/nostr/tests.rs +++ b/src/discovery/nostr/tests.rs @@ -39,7 +39,7 @@ fn can_reach(local_nat: NatType, remote_nat: NatType) -> bool { fn signed_overlay_advert_event(created_at_secs: u64, expiration_secs: Option) -> nostr::Event { let keys = nostr::Keys::generate(); - let content = r#"{"identifier":"fips-overlay-v1","version":1,"endpoints":[{"transport":"tcp","addr":"203.0.113.10:443"}]}"#; + let content = r#"{"identifier":"fips-overlay-v1","version":1,"endpoints":[{"transport":"tcp","addr":"8.8.8.8:443"}]}"#; let mut builder = EventBuilder::new(Kind::Custom(ADVERT_KIND), content) .custom_created_at(Timestamp::from(created_at_secs)); if let Some(expiration_secs) = expiration_secs { @@ -118,6 +118,57 @@ fn rejects_invalid_overlay_adverts() { assert!(NostrDiscovery::validate_overlay_advert(wrong_identifier).is_err()); } +#[test] +fn validate_overlay_advert_filters_unroutable_direct_endpoints() { + let advert = OverlayAdvert { + identifier: ADVERT_IDENTIFIER.to_string(), + version: ADVERT_VERSION, + endpoints: vec![ + OverlayEndpointAdvert { + transport: OverlayTransportKind::Tcp, + addr: "192.168.1.10:443".to_string(), + }, + OverlayEndpointAdvert { + transport: OverlayTransportKind::Udp, + addr: "100.64.1.2:2121".to_string(), + }, + OverlayEndpointAdvert { + transport: OverlayTransportKind::Tcp, + addr: "8.8.8.8:443".to_string(), + }, + ], + signal_relays: None, + stun_servers: None, + }; + + let validated = NostrDiscovery::validate_overlay_advert(advert).unwrap(); + assert_eq!(validated.endpoints.len(), 1); + assert_eq!(validated.endpoints[0].addr, "8.8.8.8:443"); +} + +#[test] +fn validate_overlay_advert_rejects_only_unroutable_direct_endpoints() { + let advert = OverlayAdvert { + identifier: ADVERT_IDENTIFIER.to_string(), + version: ADVERT_VERSION, + endpoints: vec![ + OverlayEndpointAdvert { + transport: OverlayTransportKind::Tcp, + addr: "127.0.0.1:443".to_string(), + }, + OverlayEndpointAdvert { + transport: OverlayTransportKind::Udp, + addr: "10.0.0.2:2121".to_string(), + }, + ], + signal_relays: None, + stun_servers: None, + }; + + let err = NostrDiscovery::validate_overlay_advert(advert).unwrap_err(); + assert!(err.to_string().contains("missing publicly routable")); +} + #[test] fn advert_freshness_rejects_expired_events() { let now_secs = Timestamp::now().as_secs();