Cached 410s for 1h to prevent amplification attacks

[rmgpinto]

ref no-issue

• Without this cache, an attacker can send many activities to our inbox referencing actor URLs that return 410/404, causing our server to make an outbound HTTP request for each one. This effectively turns our server into a traffic amplifier. This cache prevents that by remembering URLs that returned 410/404 for 1 hour and failing immediately on subsequent requests without making a network call.

[coderabbitai]

Warning

Rate limit exceeded

@rmgpinto has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 25 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46bd7ae1-c89d-4f25-976a-7c96b3413c21

📥 Commits

Reviewing files that changed from the base of the PR and between b2ef232 and 70151b3.

📒 Files selected for processing (1)

• src/configuration/registrations.ts

Walkthrough

The change extends the federation configuration in src/configuration/registrations.ts by adding a new documentLoaderFactory option to the createFederation call. It imports FetchError, getDocumentLoader, kvCache, and the KvStore type from @fedify/fedify and implements a KV-backed document loader that wraps getDocumentLoader. A per-URL "gone" cache with a 1-hour TTL is introduced to short-circuit repeated loads that previously returned HTTP 410/404 by throwing FetchError; loader errors mark 410/404 responses as gone and other errors are rethrown.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Cached 410s for 1h to prevent amplification attacks' directly reflects the main change: implementing a 1-hour cache for HTTP 410/404 responses to mitigate amplification attacks.
Description check	✅ Passed	The description clearly explains the security issue being addressed, details how the cache solves it, and directly relates to the code changes in the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cache-410s

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/configuration/registrations.ts (1)

289-295: String matching for HTTP error detection is fragile and dependent on message format.

The code checks error.message.includes('HTTP 410') and error.message.includes('HTTP 404'), which is brittle since the message format could change across @fedify/fedify releases. Unfortunately, FetchError does not expose a status code property—only a url property—so using a dedicated status check is not available. Consider whether there are other error detection patterns the library provides, or document this dependency on message format as a known limitation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/configuration/registrations.ts` around lines 289 - 295, Replace the
brittle string.includes checks on error.message with a robust status-code
extraction and comparison: in the FetchError handling block (where
goneUrlCache.set(url, Date.now()) is called), apply a regex to error.message to
extract a three-digit HTTP status (e.g. /\bHTTP\s*(\d{3})\b/ or /\b(\d{3})\b/)
and convert to a number, then mark the URL only when the parsed status is 404 or
410; if no status can be parsed, either skip marking the URL and log the full
error for diagnostics or add a short comment documenting the dependency on
message format so maintainers know this is a known limitation (refer to
FetchError, goneUrlCache, and the local url variable).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/configuration/registrations.ts`:
- Around line 246-248: The goneUrlCache Map is never pruned and can grow
unbounded; add a cleanup function (e.g., cleanupGoneCache) that iterates
goneUrlCache and deletes entries whose timestamp + GONE_CACHE_TTL_MS is expired,
and wire that cleanup to run periodically (either by calling cleanupGoneCache()
at the start of the returned loader function and/or scheduling a short interval
timer) so expired keys are removed; reference the existing goneUrlCache and
GONE_CACHE_TTL_MS constants and ensure the loader that uses goneUrlCache invokes
cleanupGoneCache before checking/setting entries (or replace goneUrlCache with a
bounded LRU/fedifyKv-based store if preferred).
- Line 280: The cached error currently always rethrows as "HTTP 410" by using
throw new FetchError(url, `HTTP 410: ${url}`); update the logic that
reconstructs cached errors (the code that calls new FetchError) to read and
preserve the original status (e.g., from the cached record) and include that
status in the message and/or error object instead of hard-coding 410; locate the
usage of FetchError and the variable url in the cache-retry/rethrow path and
replace the fixed status string with the stored original status (or a generic
message when original status is unavailable).

---

Nitpick comments:
In `@src/configuration/registrations.ts`:
- Around line 289-295: Replace the brittle string.includes checks on
error.message with a robust status-code extraction and comparison: in the
FetchError handling block (where goneUrlCache.set(url, Date.now()) is called),
apply a regex to error.message to extract a three-digit HTTP status (e.g.
/\bHTTP\s*(\d{3})\b/ or /\b(\d{3})\b/) and convert to a number, then mark the
URL only when the parsed status is 404 or 410; if no status can be parsed,
either skip marking the URL and log the full error for diagnostics or add a
short comment documenting the dependency on message format so maintainers know
this is a known limitation (refer to FetchError, goneUrlCache, and the local url
variable).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3d4b5d09-866f-4901-bdcd-f3cd2ee46601

📥 Commits

Reviewing files that changed from the base of the PR and between 150b53a and 8728d1f.

📒 Files selected for processing (1)

• src/configuration/registrations.ts