Refuse federation lookups for unauthenticated GraphQL callers#278
Merged
Conversation
Extends createFedCtx in test/postgres.ts with an optional lookupObject option, so upcoming guest-gate tests can install a spy and assert that federation lookup was not invoked. The default value is a function that throws a clearly-named Error, giving a meaningful failure if a code path unexpectedly calls fedCtx.lookupObject without an explicit override (instead of the previous TypeError from a missing method). This is a test-fixture-only change; no production behavior is affected and the existing 362 tests still pass. It is the first step toward restricting unauthenticated federation lookups described in hackers-pub#277. Assisted-by: Claude Code:claude-opus-4-7
Three GraphQL entry points (plus searchAsHandle, surfaced in
review) previously branched on ctx.account == null only to pick
which documentLoader to use, but still called fedCtx.lookupObject
followed by persistActor / persistPost. An unauthenticated
caller could therefore feed arbitrary handles or URLs to:
* actorByHandle in graphql/actor.ts
* lookupActorByUrl in graphql/lookup.ts (via actorByUrl)
* lookupPostByUrl in graphql/lookup.ts (via searchObject /
searchAsUrl)
* searchAsHandle in graphql/search.ts (via searchObject)
…and force the server to perform outbound WebFinger / ActivityPub
fetches and persist whatever object came back. The concrete
risks are spam-driven actor / post row growth, attacker-controlled
outbound requests, and noise in federation logs.
After this change, all four sites short-circuit to null when
ctx.account is null, immediately after the local-database checks
and before any federation lookup. Authenticated paths are
unchanged. Tests use the recording lookupObject hook on
createFedCtx to assert that no federation call happens for
guest contexts.
The mention hover-card flow from the previous PR will gracefully
fall back to "Could not load profile" for guests on remote actors
that haven't been pulled into the local DB by a signed-in user
yet — an acceptable trade-off per the issue.
The lookupRemoteFollower resolver in graphql/webfinger.ts is
intentionally left out of this commit: its purpose is the
cross-instance "follow from another instance" UX where guest
invocation is intrinsic, and it does not persist actors or posts.
hackers-pub#277
Assisted-by: Claude Code:claude-opus-4-7
The Question.poll resolver in graphql/poll.ts had the same shape as the four call sites already gated for guest callers: a local "do we already have it?" check, then a fall-through to ctx.fedCtx.lookupObject(question.iri) + persistPost when the poll subobject was missing. The IRI here is a known local Question's own iri rather than attacker-supplied input, so this is narrower than the originally listed entry points — but it still lets unauthenticated callers trigger outbound fetches and persist a poll subobject under their request. When ctx.account is null, the resolver now short-circuits to null right after the early returns for question.poll and question.sharedPostId, before any federation work happens. The three pre-existing backfill tests previously used makeGuestContext (success path, non-Question rejection, lookup failure). After the gate they would all return null without exercising the production code path they were written for, so they switch to makeUserContext and continue covering the authenticated branch. A new dedicated test "Question.poll refuses federation lookup for guests" installs a recording lookupObject and asserts the resolver returns null with no federation call. createFedCtx in test/postgres.ts gains a getDocumentLoader stub that resolves to a DocumentLoader which rejects when invoked. This is needed because the converted authenticated tests now reach `await ctx.fedCtx.getDocumentLoader(...)`, but they only override `lookupObject` (not the loader), and the loader value is never actually consumed in those tests since `lookupObject` returns synthetic vocab objects directly. The rejection on invocation makes any genuinely-needed real load fail loudly. Surfaced from a Codex branch-level review of the previous commit. hackers-pub#277 Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
📝 WalkthroughWalkthroughThis PR restricts unauthenticated federation lookups across five GraphQL resolvers by adding early-return guards when ChangesGuest-Blocking Federation Lookups
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
Author
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a security measure to prevent unauthenticated guest users from triggering outbound federation lookups. Changes were made to the GraphQL resolvers for actors, posts, polls, and search to return null for guests instead of proceeding with federation fetches. Corresponding test cases were added to verify this behavior and existing tests were updated to use authenticated contexts where federation is required. I have no feedback to provide.
|
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #277.
Why
Five GraphQL resolvers branched on
ctx.account == nullonly to pick adocumentLoader, then still calledfedCtx.lookupObjectandpersistActor/persistPostwhen the local DB lookup missed. An unauthenticated client could feed arbitrary handles or URLs and force the server into outbound WebFinger/ActivityPub fetches, persisting whatever came back asactororpostrows. That exposed the server to spam-driven row growth and attacker-controlled outbound requests, with federation-log noise as a secondary concern.What changed
When
ctx.account == null, the following sites now short-circuit tonullafter the local DB checks and before any federation work:actorByHandlein graphql/actor.tslookupActorByUrlin graphql/lookup.ts, reached viaactorByUrllookupPostByUrlin graphql/lookup.ts, reached viasearchObject→searchAsUrlsearchAsHandlein graphql/search.ts, reached viasearchObject(beyond the original issue scope; same guest-triggered federation path)Question.pollin graphql/poll.ts (surfaced from a Codex branch-level review; narrower than the others because the IRI is a known local Question'sirirather than caller input, but it still triggered a guest-driven outbound fetch andpersistPost)Authenticated paths are unchanged.
Out of scope
lookupRemoteFollowerin graphql/webfinger.ts is intentionally not gated. It backs the cross-instance “follow from another instance” flow, where the caller has no local account by definition: a remote user pastes their handle to get back a remote-follow URL. It also does not callpersistActororpersistPost, so the persistence concerns from the issue do not apply. Outbound-fetch concerns there can be handled separately with rate limiting or caching.The legacy Fresh route at web/routes/search.tsx mirrors the same pattern but lives on the pre-migration stack and falls outside this GraphQL-focused fix.
UX trade-off
The mention hover cards from #90/#276 will fall back to “Could not load profile” for guests on remote actors that no signed-in user has pulled into the local DB yet. Acceptable per the issue.
Tests
deno task testreports 369/0 (was 362 before this branch).New guest-path tests install a recording
lookupObjectvia a new option oncreateFedCtxin test/postgres.ts, then assert the resolver returnsnulland that nothing was recorded. Without the gate, every call site under test records one entry, so the assertions show the gate ran instead of passing because an unrelated lookup returnednull.Three pre-existing
Question.pollbackfill tests in graphql/poll.test.ts moved frommakeGuestContexttomakeUserContextso they keep covering the authenticated backfill path; a new dedicated test,Question.poll refuses federation lookup for guests, locks in the guest contract.The fixture stubs
getDocumentLoaderoncreateFedCtxtoo. The returnedDocumentLoaderrejects when invoked, so a test that needs genuine document loading fails with a named error instead of silently returning nothing.Test plan
deno task testpasses locallydeno task checkpasses (lint + format +tsc; also runs in the pre-commit hook)nullfromsearchObject(no redirect)