Conversation
Move Alice's inbox fixtures to separate IPv6 documentation ranges and keep the document loader and stored actor fixtures consistent. Add module-level fetch interceptors, which this checkout lacks, so fixture deliveries receive a local response without DNS or network I/O. This implements the fixture portion of: fedify-dev#1046 Validated with mise run check-each relay and mise run test-each relay (Deno, Node.js, and Bun). Changelog: none Assisted-by: Codex:gpt-6-astra
Check for 202 Accepted at all 14 previously discarded inbox responses in the LitePub and Mastodon relay tests. This prevents follower-store assertions from hiding errors returned by the inbox handler. fedify-dev#1046 Validated with mise run test-each relay, including code checks and Deno, Node.js, and Bun tests. Changelog: none Assisted-by: Codex:gpt-6-astra
Record deliveries to Alice's inbox and assert that approved follows produce exactly one Accept POST from the relay for the expected Follow. Cover subscription approval, public follows, and malformed follower rows. Use unique Follow IDs to isolate concurrent tests and loop iterations. fedify-dev#1046 Validated with mise run test-each relay, including code checks and Deno, Node.js, and Bun tests. Changelog: none Assisted-by: Codex:gpt-6-astra
✅ Deploy Preview for fedify-json-schema canceled.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughChangesDelivery failure handling and relay test coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The change records DNS delivery failures correctly and strengthens relay tests to require successful responses and expected Accept deliveries; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR also changes Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (2 skipped: 2 unsupported.)
✨ 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 |
sij411
changed the title
500 Internal Server Errors
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
DNS resolution failures are temporary transport failures, not evidence that a destination violates the private-address policy. Preserve their FetchError classification so the existing outbox middleware counts them against the circuit breaker. Keep policy rejections as UrlError so they do not count as transport failures or weaken SSRF protection. The URL validator currently attaches a cause only when DNS resolution throws; private-address and protocol rejections have no cause. Use that existing distinction at the delivery boundary to retain the resolver's original error without changing the shared validator's API or behavior for its other callers. This relies on that distinction remaining true; if policy errors gain causes, introduce an explicit discriminator. Move initial URL validation inside the delivery try/catch so direct DNS failures record failure metrics just like redirect and fetch failures. Validation still happens before any request is sent. Add signed and unsigned regression cases for direct and redirected DNS failures, checking FetchError, failure metrics, and no fetch to the failed destination. Existing destination tests retain private-address coverage. Verified that the regression tests fail before the fix and pass after it. Focused Deno tests and the full fedify package suite passed on Deno, Node.js, and Bun. Added a Sacho fragment and regenerated the changelog. Assisted-by: Codex:gpt-6-astra
|
@codex review |
|
@coderabbitai review |
|
af66039 : not 100% sure this is the right solution so if y'all have better solution leave some comments on please |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
The test changes do what #1046 asked for, and the new assertions have teeth: putting Alice's inbox back on remote.example.com fails three tests in litepub.test.ts.
My one structural request is the last commit, “Preserve transport errors for DNS failures,” which changes delivery behavior in @fedify/fedify and is unrelated to this issue. Details are inline. Please drop it from this branch and open a separate issue for it; the relay tests pass without it, so nothing here depends on it.
The rest is minor.
| id: new URL("https://remote.example.com/users/alice"), | ||
| preferredUsername: "alice", | ||
| inbox: new URL("https://remote.example.com/users/alice/inbox"), | ||
| inbox: new URL(aliceInbox), |
There was a problem hiding this comment.
follower2's inbox on line 1050 still points at remote.example.com, so this test mixes two fixture conventions. Nothing fails today because it never delivers, but that is easy to misread later. Either move bob onto the intercepted prefix as well, or note that this fixture is store-only.
| strictEqual(accepts[0].body.actor, "https://relay.example.com/users/relay"); | ||
| strictEqual(accepts[0].body.object.type, "Follow"); | ||
| strictEqual( | ||
| accepts[0].body.object.actor?.id ?? accepts[0].body.object.actor, |
There was a problem hiding this comment.
This accepts either serialization shape, so it passes whichever one the relay happens to emit. Pinning the shape the relay actually produces would catch a regression that changes it. The same helper in mastodon.test.ts has the same issue.
| } catch (error) { | ||
| // DNS resolution errors carry the resolver's failure as their cause; | ||
| // private-address and protocol policy rejections do not. | ||
| if (error instanceof UrlError && error.cause != null) { |
There was a problem hiding this comment.
This is unrelated to #1046 and is not mentioned in the pull request description. Could you split it into its own issue and pull request?
Two things to settle there:
- The bug is already released. 68ec7b4 shipped in 2.0.28 through 2.3.8, so under the branch policy the fix belongs on 2.0-maintenance rather than main.
error.cause != nullrelies onvalidatePublicUrl()attaching a cause only for DNS failures, which is an internal detail of the validator. Please add an explicit property toUrlErrorand discriminate on that instead. Adding API in a patch release is not ideal, but this is small enough to be acceptable. It would also coverDNS lookup did not return any IP addressin packages/vocab-runtime/src/url.ts, which carries no cause and so stays aUrlErrorunder the current heuristic.
| } | ||
| let response: Response; | ||
| try { | ||
| await validateUrl(inbox.href); |
There was a problem hiding this comment.
Moving this inside the try also makes private address rejections record a delivery failure metric and an error log, which is a separate behavior change from the DNS one and is not in the changelog fragment. Worth deciding explicitly when this moves to its own pull request.
| ignore: "Deno" in globalThis && | ||
| (await Deno.permissions.query({ name: "net" })).state !== "granted", | ||
| async fn(t) { | ||
| const destination = "https://fedify-delivery.invalid/inbox"; |
There was a problem hiding this comment.
fedify-delivery.invalid performs a real DNS query on Node.js and Bun. On a network that answers NXDOMAIN with a public address, validation passes, the catch-all mock returns 202 Accepted, and this fails for reasons unrelated to the code. Stubbing the resolver would be better; failing that, a comment would save the next person the debugging.
This branch has not been deployed
2 participants
Fixes #1046
202 Acceptedfor previously unchecked inbox responses.Accept POSTfrom the relay, referencing the correctFollowAI assistance