Validate actor consistency for inbound Undo activities#1747
Conversation
WalkthroughTest teardown code across several integration tests was hardened by guarding database/client destroy calls with optional chaining to avoid errors when the DB/client is undefined. The undo dispatcher ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vitest.config.ts (1)
6-11: LGTM — sensible bounds for DB-backed tests.The 30s/60s timeouts and capped worker pool address the documented Docker flakiness. One follow-up to consider (non-blocking): since these limits primarily benefit integration tests, you could keep unit tests fast by scoping worker constraints via a separate
projects/workspace config or by overriding per-file. Fine to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 6 - 11, The current test config object sets conservative timeouts and worker caps (test.testTimeout, test.hookTimeout, test.minWorkers, test.maxWorkers) which are ideal for integration/DB-backed tests; to keep unit tests fast, create a projects array in vitest.config.ts and move these settings into a dedicated "integration" project (override its test.{testTimeout,hookTimeout,minWorkers,maxWorkers}), while leaving the default/unit project with standard (faster) timeouts and worker settings or no overrides; ensure each project uses appropriate include/exclude or file patterns to separate unit vs integration tests and reference the existing test config keys when applying overrides.src/dispatchers.ts (1)
532-549: Consider eliminating theobject.getActor(ctx)HTTP call now thatobject.actorIdis validated.After the new guard at lines 532-541,
object.actorId.href === undo.actorId.hrefis guaranteed (andundo.actorIditself was verified by Fedify’s HTTP-signature check). The subsequentawait object.getActor(ctx)at line 543 performs a network dereference solely to obtainsender.id, which we already have asobject.actorId. Skipping this avoids a potentially slow/failing remote fetch and is also consistent with the optimisation note in the PR description.♻️ Proposed refactor
- const sender = await object.getActor(ctx); - if (sender === null || sender.id === null) { - ctx.data.logger.debug( - 'Undo announce activity sender missing, exit early', - ); - return; - } - const senderAccount = await accountService.getByApId(sender.id); + const senderAccount = await accountService.getByApId( + object.actorId, + );If you keep this for a follow-up PR, that's fine — just flagging while the validation context is fresh. The corresponding test at lines 222-248 currently relies on
vi.spyOn(announce, 'getActor'); if you adopt the refactor, that mock can be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dispatchers.ts` around lines 532 - 549, After verifying undo.actorId and object.actorId.href match, remove the network dereference await object.getActor(ctx) and the sender null/id checks; instead treat object.actorId (and its href/id) as the authoritative sender identifier (use object.actorId or object.actorId.href wherever sender.id was used), and update any logging (ctx.data.logger.debug/warn) to reflect using the validated actorId; also remove or update tests that spy on getActor (e.g., announce.getActor) since the call is no longer needed.
🤖 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/db.integration.test.ts`:
- Around line 22-26: The teardown hook afterAll is currently defined inside
beforeAll so it won't be registered if createTestDb() or the setup throws; move
the afterAll block out of the beforeAll callback into the test suite scope so it
always registers, keep using the optional chaining on client
(client?.schema.dropTableIfExists(TEST_TABLE); client?.destroy()) and reference
the same TEST_TABLE and client variables; ensure the afterAll runs after tests
to perform cleanup even when beforeAll fails.
---
Nitpick comments:
In `@src/dispatchers.ts`:
- Around line 532-549: After verifying undo.actorId and object.actorId.href
match, remove the network dereference await object.getActor(ctx) and the sender
null/id checks; instead treat object.actorId (and its href/id) as the
authoritative sender identifier (use object.actorId or object.actorId.href
wherever sender.id was used), and update any logging
(ctx.data.logger.debug/warn) to reflect using the validated actorId; also remove
or update tests that spy on getActor (e.g., announce.getActor) since the call is
no longer needed.
In `@vitest.config.ts`:
- Around line 6-11: The current test config object sets conservative timeouts
and worker caps (test.testTimeout, test.hookTimeout, test.minWorkers,
test.maxWorkers) which are ideal for integration/DB-backed tests; to keep unit
tests fast, create a projects array in vitest.config.ts and move these settings
into a dedicated "integration" project (override its
test.{testTimeout,hookTimeout,minWorkers,maxWorkers}), while leaving the
default/unit project with standard (faster) timeouts and worker settings or no
overrides; ensure each project uses appropriate include/exclude or file patterns
to separate unit vs integration tests and reference the existing test config
keys when applying overrides.
🪄 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: 81696164-15ee-48c8-b4c6-346953d95ad0
📒 Files selected for processing (10)
src/db.integration.test.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/ghost/ghost-post.service.integration.test.tssrc/http/api/views/blocks.view.integration.test.tssrc/http/api/views/explore.view.integration.test.tssrc/http/api/views/recommendations.view.integration.test.tssrc/http/api/views/topic.view.integration.test.tssrc/post/post.service.integration.test.tsvitest.config.ts
ilyador
force-pushed
the
fix/undo-actor-validation
branch
from
50e3077 to
d629fa8
Compare
|
|
||
| afterAll(async () => { | ||
| await db.destroy(); | ||
| await db?.destroy(); |
There was a problem hiding this comment.
question: why do we need optional chaining here?
| plugins: [tsconfigPaths()], | ||
| test: { | ||
| testTimeout: 1000 * 10, | ||
| testTimeout: 1000 * 30, |
There was a problem hiding this comment.
question: why do we need to increase test timeouts?
sagzy
force-pushed
the
fix/undo-actor-validation
branch
from
d629fa8 to
e9494d8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dispatchers.unit.test.ts (1)
1-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix import ordering to satisfy lint gate
This file currently fails
assist/source/organizeImports; please run import organization so CI passes cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dispatchers.unit.test.ts` around lines 1 - 37, The import statements in the test file are not ordered correctly and fail the assist/source/organizeImports lint check; reorder or run your editor's "organize imports" (or run the project import-organizer) so imports are grouped and sorted (external modules first, then internal project paths, then types), and alphabetized within groups — specifically fix the ordering among imports like '@fedify/fedify' (Actor, Announce, Follow, Undo, exportJwk, RequestContext), 'vitest' (beforeEach, describe, expect, it, vi), and the internal imports referencing symbols such as ActorService types (Account, AccountEntity, AccountService, Account as AccountType), FollowersService, FedifyContext/FedifyRequestContext, constants (ACTIVITYPUB_COLLECTION_PAGE_SIZE, ACTOR_DEFAULT_HANDLE), result (error, ok), dispatcher factories (actorDispatcher, keypairDispatcher, createFollowersDispatcher, createFollowersCounter, createFollowingDispatcher, createFollowingCounter, createOutboxDispatcher, createOutboxCounter, createUndoHandler, likedDispatcher, nodeInfoDispatcher), HostDataContextLoader, and post entities/services (OutboxType, Post, PostType, KnexPostRepository, PostService) so CI lint passes.
🧹 Nitpick comments (1)
src/dispatchers.unit.test.ts (1)
203-220: ⚡ Quick winAssert no
globaldb.seton rejected Undo(Announce)The mismatch-path test should also assert
undoCtx.data.globaldb.setis not called, since PR intent is “no state mutation before actor-consistency passes.”Suggested assertion
await handler(undoCtx, undo); expect(accountService.getByApId).not.toHaveBeenCalled(); expect(postService.getByApId).not.toHaveBeenCalled(); expect(postRepository.save).not.toHaveBeenCalled(); + expect(undoCtx.data.globaldb.set).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dispatchers.unit.test.ts` around lines 203 - 220, The test for ignoring Undo(Announce) when outer actor doesn't match should also assert that no state mutation occurs by checking undoCtx.data.globaldb.set was not called; update the test (the it block that constructs Announce, Undo and calls handler(undoCtx, undo)) to add an expectation after the existing expects to verify undoCtx.data.globaldb.set is not invoked, referencing the undoCtx object and its data.globaldb.set method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/dispatchers.unit.test.ts`:
- Around line 1-37: The import statements in the test file are not ordered
correctly and fail the assist/source/organizeImports lint check; reorder or run
your editor's "organize imports" (or run the project import-organizer) so
imports are grouped and sorted (external modules first, then internal project
paths, then types), and alphabetized within groups — specifically fix the
ordering among imports like '@fedify/fedify' (Actor, Announce, Follow, Undo,
exportJwk, RequestContext), 'vitest' (beforeEach, describe, expect, it, vi), and
the internal imports referencing symbols such as ActorService types (Account,
AccountEntity, AccountService, Account as AccountType), FollowersService,
FedifyContext/FedifyRequestContext, constants (ACTIVITYPUB_COLLECTION_PAGE_SIZE,
ACTOR_DEFAULT_HANDLE), result (error, ok), dispatcher factories
(actorDispatcher, keypairDispatcher, createFollowersDispatcher,
createFollowersCounter, createFollowingDispatcher, createFollowingCounter,
createOutboxDispatcher, createOutboxCounter, createUndoHandler, likedDispatcher,
nodeInfoDispatcher), HostDataContextLoader, and post entities/services
(OutboxType, Post, PostType, KnexPostRepository, PostService) so CI lint passes.
---
Nitpick comments:
In `@src/dispatchers.unit.test.ts`:
- Around line 203-220: The test for ignoring Undo(Announce) when outer actor
doesn't match should also assert that no state mutation occurs by checking
undoCtx.data.globaldb.set was not called; update the test (the it block that
constructs Announce, Undo and calls handler(undoCtx, undo)) to add an
expectation after the existing expects to verify undoCtx.data.globaldb.set is
not invoked, referencing the undoCtx object and its data.globaldb.set method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fd81e7b-eefd-43f0-a019-49ca56ac27ef
📒 Files selected for processing (10)
src/db.integration.test.tssrc/dispatchers.tssrc/dispatchers.unit.test.tssrc/ghost/ghost-post.service.integration.test.tssrc/http/api/views/blocks.view.integration.test.tssrc/http/api/views/explore.view.integration.test.tssrc/http/api/views/recommendations.view.integration.test.tssrc/http/api/views/topic.view.integration.test.tssrc/post/post.service.integration.test.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- src/http/api/views/topic.view.integration.test.ts
- src/http/api/views/blocks.view.integration.test.ts
- src/http/api/views/recommendations.view.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/ghost/ghost-post.service.integration.test.ts
- src/post/post.service.integration.test.ts
- vitest.config.ts
- src/http/api/views/explore.view.integration.test.ts
- src/db.integration.test.ts
- src/dispatchers.ts
2 participants
What
Added actor-consistency guards to
createUndoHandlerindispatchers.tsfor bothUndo(Follow)andUndo(Announce)paths. The outerUndo.actorId(verified by Fedify's HTTP signature check) is now compared against the nested activity's actor before any state mutation or lookup occurs.Why
The handler trusted the actor on the nested activity without verifying it matched the signature-verified outer actor. A malicious federated server could send an
Undosigned by its own actor but containing a nestedFolloworAnnounceattributed to a different actor — effectively unfollowing or removing a repost on behalf of someone else.For example,
evil.examplecould send:{ "type": "Undo", "actor": "https://evil.example/users/attacker", "object": { "type": "Follow", "actor": "https://victim.example/users/alice", "object": "https://ghost.example/.ghost/activitypub/users/index" } }Fedify verifies the request against
evil.example, but the handler would process the unfollow as if Alice requested it.How
Early-return guard in each branch that compares
undo.actorId.hrefwith the nested activity'sactorId.href. Mismatches are logged atwarnlevel (security event, visible in production without debug mode) and silently rejected.No refactoring, no class conversion, no changes to unrelated Undo paths.
Test harness fix
The first commit fixes the test harness so tests can run locally via
yarn test. Two issues:Teardown crash on failure: When a test fails or times out, the
afterAll/afterEachhooks calldb.destroy()on a connection that was never created or already closed, throwing an unrelated error that masks the real failure. Fixed with optional chaining (db?.destroy()).Timeouts too tight for Docker: The default 10s test timeout and no hook timeout cause integration tests to fail when the MySQL container in Docker Compose takes longer to respond — especially on first run or under load. Increased test timeout to 30s, added 60s hook timeout, and limited workers to 4 to avoid overwhelming the containerized database.
Tests
Added the first unit test coverage for
createUndoHandlerindispatchers.unit.test.ts:ignores Undo(Follow) when outer actor does not match Follow actorprocesses Undo(Follow) when outer actor matches Follow actorignores Undo(Announce) when outer actor does not match Announce actorprocesses Undo(Announce) when outer actor matches Announce actorAll 47 tests pass.
Note
The
getActor(ctx)HTTP call in the Announce path could be replaced with the already-validatedobject.actorIdto skip a round-trip, but that changes existing behavior beyond the scope of this fix. Mentioned here as a potential follow-up optimization.Note
Medium Risk
Touches inbound ActivityPub
Undohandling to prevent unauthorized unfollow/repost removal; behavior change could reject some previously-accepted (but inconsistent) remote activities. Test runner timeout/worker tweaks may also affect CI duration and flakiness characteristics.Overview
Hardens inbound
Undoprocessing by requiring the signature-verifiedUndo.actorIdto match the nested activity’s actor before performing any lookups or state changes forUndo(Follow)andUndo(Announce).Adds unit coverage for these actor-mismatch/match cases, and stabilizes tests by making DB teardown null-safe (
db?.destroy()/client?.destroy()) plus relaxing Vitest timeouts and constraining worker counts for slower Dockerized DB runs.Reviewed by Cursor Bugbot for commit 50e3077. Bugbot is set up for automated code reviews on this repo. Configure here.