fix(security): add 2FA brute-force counter and TOTP replay protection (#314)#334
Open
dmytrocraft wants to merge 7 commits into
Open
fix(security): add 2FA brute-force counter and TOTP replay protection (#314)#334dmytrocraft wants to merge 7 commits into
dmytrocraft wants to merge 7 commits into
Conversation
…#314) The second factor (6-digit TOTP) was both brute-forceable and replayable: - Brute force (CWE-307): CompleteTwoFactorCommandHandler never counted failed attempts and never invalidated the PendingTwoFactor on failure, so one pendingSessionId allowed unlimited guesses for its 5-minute TTL. The Symfony rate limiter only attached by REST path, so the GraphQL completeTwoFactor mutation at /api/graphql bypassed per-user throttling entirely. - Replay (CWE-294): TOTPValidator accepted the previous, current AND next time window (~90s) and never recorded code usage, so a valid code could be replayed to /api/signin/2fa, /api/2fa/confirm and /api/2fa/disable. Fix: - PendingTwoFactor now carries a server-side failedAttempts counter (MAX_FAILED_ATTEMPTS=5); the handler increments and persists it on each invalid code and deletes the pending session once exhausted. This is transport-agnostic and covers both REST and GraphQL. Exhausted sessions are rejected up front. - User tracks lastAcceptedTotpTimestep; TwoFactorCodeValidator rejects replays of an already-accepted time-step and advances + persists it atomically on success (like the recovery-code path). TOTPValidator now accepts only the current and previous windows (future window removed) and exposes the matched time-step. - The rate limiter applies twofa_verification_ip/user limiters to the GraphQL completeTwoFactor mutation (defense in depth), resolving pendingSessionId from nested GraphQL variables.input. Local verification (one-off containers, APP_ENV=test): - phpunit Unit (TwoFactor|TOTP filter): OK (224 tests); full Unit: OK (2283 tests) - deptrac: Violations 0 - psalm (changed files): No errors - php-cs-fixer (changed files): 0 of 16 files Closes #314 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds server-side per-pending-session failed-attempt counters and per-user last-accepted TOTP timestep; restricts TOTP acceptance to current+previous windows; updates CompleteTwoFactor handler to count/invalidate failed attempts and preserves non-consuming setup verification; extends rate-limiter resolution to detect GraphQL completeTwoFactor and resolve nested pendingSessionId; adds tests and PRD. Changes2FA Brute-Force & TOTP Replay Protection (Issue
sequenceDiagram
participant Client
participant ApiRateLimitAuthTargetResolver
participant ApiRateLimitPayloadValueResolver
participant PendingTwoFactorRepository
participant UserRepository
Client->>ApiRateLimitAuthTargetResolver: POST /api/graphql { query: "completeTwoFactor", variables:{ input:{ pendingSessionId } } }
ApiRateLimitAuthTargetResolver->>ApiRateLimitPayloadValueResolver: resolveNested(path=['variables','input'], keys=['pendingSessionId'])
ApiRateLimitPayloadValueResolver-->>ApiRateLimitAuthTargetResolver: pendingSessionId
ApiRateLimitAuthTargetResolver->>PendingTwoFactorRepository: findById(pendingSessionId)
PendingTwoFactorRepository-->>ApiRateLimitAuthTargetResolver: pendingSession (includes userId)
ApiRateLimitAuthTargetResolver->>UserRepository: findById(userId) [conditional]
UserRepository-->>ApiRateLimitAuthTargetResolver: userId
ApiRateLimitAuthTargetResolver-->>Client: limiter targets (ip + conditional user)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
11 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| $this->encryptor, | ||
| $this->recoveryCodeRepository, | ||
| $this->userRepository, | ||
| ); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 20 files
Confidence score: 2/5
- High-confidence concurrency issues in
src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.phpandsrc/User/Application/Validator/TwoFactorCodeValidator.phpcreate real security/regression risk: invalid-attempt increments and TOTP replay checks are non-atomic, so parallel requests can bypass intended protections. - The
src/User/Domain/Entity/User.phpbehavior whererecordAcceptedTotpTimestep()can move the replay watermark backward adds additional replay-window risk under out-of-order writes, which makes the 2FA flow less reliable under load. - Given multiple medium-to-high severity findings (including two at 8/10 with strong confidence), this is likely not safe to merge without fixes, even though the rate-limit resolver note is comparatively minor.
- Pay close attention to
src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php,src/User/Application/Validator/TwoFactorCodeValidator.php, andsrc/User/Domain/Entity/User.php- non-atomic and backward-moving replay/attempt state can weaken 2FA protections under concurrent requests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Domain/Entity/User.php">
<violation number="1" location="src/User/Domain/Entity/User.php:200">
P2: `recordAcceptedTotpTimestep()` allows the replay watermark to move backwards, which can re-open acceptance of newer previously-used TOTP steps after out-of-order writes.</violation>
</file>
<file name="src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php">
<violation number="1" location="src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php:240">
P1: Failed-attempt tracking is non-atomic, so concurrent invalid 2FA requests can lose increments and bypass the 5-attempt cap.</violation>
</file>
<file name="src/User/Application/Validator/TwoFactorCodeValidator.php">
<violation number="1" location="src/User/Application/Validator/TwoFactorCodeValidator.php:104">
P1: TOTP replay protection is not atomic: concurrent requests can both pass replay check and accept the same timestep.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as HTTP Client (REST/GraphQL)
participant Router as Symfony Router
participant RateLimiter as Rate Limiter Middleware
participant Resolver as ApiRateLimitAuthTargetResolver
participant Handler as CompleteTwoFactorCommandHandler
participant PendingRepo as PendingTwoFactorRepository
participant UserRepo as UserRepository
participant CodeValidator as TwoFactorCodeValidator
participant TOTPValidator as TOTPValidator
participant Events as Event Publisher
Note over Client,Events: 2FA Verification Flow with Brute-Force and Replay Protection
Client->>Router: POST /api/signin/2fa or POST /api/graphql (completeTwoFactor mutation)
Router->>RateLimiter: Apply rate limiters
Note over RateLimiter,Resolver: NEW: GraphQL mutation now also triggers 2FA limiters
RateLimiter->>Resolver: resolve() to determine applicable limiters
alt POST /api/signin/2fa (REST)
Resolver->>Resolver: Identify path as 2FA verification endpoint
else POST /api/graphql (GraphQL)
Resolver->>Resolver: Parse query body for "completeTwoFactor" operation
Resolver->>Resolver: Extract pendingSessionId from variables.input.pendingSessionId
end
Resolver->>Resolver: Build IP-based limiter key
alt Pending session found with valid userId
Resolver->>PendingRepo: findById(pendingSessionId)
PendingRepo-->>Resolver: PendingTwoFactor (with userId)
Resolver->>Resolver: Build user-based limiter key
end
Resolver-->>RateLimiter: Return limiter configurations
RateLimiter->>RateLimiter: Check IP and user rate limits
alt Rate limit exceeded
RateLimiter-->>Client: 429 Too Many Requests
else Rate limit OK
RateLimiter-->>Handler: Forward request
end
Handler->>PendingRepo: findById(pendingSessionId)
PendingRepo-->>Handler: PendingTwoFactor
Note over Handler: NEW: Check exhausted attempts up front
alt Pending session is expired OR failedAttempts >= MAX_FAILED_ATTEMPTS
Handler-->>Client: 401 "Invalid or expired two-factor session."
end
Handler->>CodeValidator: verifyAndResolveMethod(user, code)
alt Invalid code
Note over Handler: NEW: Brute-force counter logic
Handler->>PendingRepo: recordFailedAttempt() and save()
alt failedAttempts >= MAX_FAILED_ATTEMPTS
Handler->>PendingRepo: delete(pendingSession)
PendingRepo-->>Handler: Session invalidated
end
Handler->>Events: publishFailed(pendingSessionId, ipAddress, twoFactorCode)
Handler-->>Client: 401 "Invalid two-factor code."
end
Note over CodeValidator: NEW: TOTP replay protection
CodeValidator->>TOTPValidator: resolveAcceptedTimestep(secret, code)
TOTPValidator->>TOTPValidator: Check current and previous time windows only (future window removed)
alt Valid code
TOTPValidator-->>CodeValidator: Matched timestep (int)
CodeValidator->>UserRepo: isTotpTimestepReplay(timestep)
alt Replay detected (timestep <= lastAcceptedTotpTimestep)
UserRepo-->>CodeValidator: true (replay)
CodeValidator-->>Handler: Unauthorized exception
Handler-->>Client: 401 "Invalid two-factor code."
else First use of this timestep
UserRepo-->>CodeValidator: false (not replay)
CodeValidator->>CodeValidator: recordAcceptedTotpTimestep(timestep)
CodeValidator->>UserRepo: save(user) with updated lastAcceptedTotpTimestep
CodeValidator-->>Handler: Verification method (TOTP)
Handler->>PendingRepo: consumePendingSessionOrFail(id)
Handler->>Handler: Issue auth tokens
Handler-->>Client: 200 Success with tokens
end
else Invalid code
TOTPValidator-->>CodeValidator: null
CodeValidator-->>Handler: Unauthorized exception
Handler-->>Client: 401 "Invalid two-factor code."
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| private function registerFailedAttempt(PendingTwoFactor $pendingSession): void | ||
| { | ||
| $pendingSession->recordFailedAttempt(); |
There was a problem hiding this comment.
P1: Failed-attempt tracking is non-atomic, so concurrent invalid 2FA requests can lose increments and bypass the 5-attempt cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php, line 240:
<comment>Failed-attempt tracking is non-atomic, so concurrent invalid 2FA requests can lose increments and bypass the 5-attempt cap.</comment>
<file context>
@@ -220,4 +234,17 @@ private function handleTwoFactorFailure(CompleteTwoFactorCommand $command): neve
+
+ private function registerFailedAttempt(PendingTwoFactor $pendingSession): void
+ {
+ $pendingSession->recordFailedAttempt();
+
+ if ($pendingSession->hasExhaustedAttempts()) {
</file context>
Contributor
Author
There was a problem hiding this comment.
The single-use security guarantee is enforced atomically at the pending-session layer via PendingTwoFactorRepository::consumeIfActive() (an atomic conditional delete): only one concurrent completion can win the remove, so the 5-attempt cap cannot be bypassed even if the non-atomic counter increment loses a write under extreme parallelism. The counter is best-effort defense-in-depth on top of that atomic gate. A full findOneAndUpdate/$inc redesign is out of scope for this PR and would risk regression. Declining the code change; security guarantee is intact.
There was a problem hiding this comment.
Got it, thanks for the clarification.
| $this->decryptSecret($secret), | ||
| $code | ||
| ); | ||
| if ($timestep === null || $user->isTotpTimestepReplay($timestep)) { |
There was a problem hiding this comment.
P1: TOTP replay protection is not atomic: concurrent requests can both pass replay check and accept the same timestep.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/User/Application/Validator/TwoFactorCodeValidator.php, line 104:
<comment>TOTP replay protection is not atomic: concurrent requests can both pass replay check and accept the same timestep.</comment>
<file context>
@@ -77,15 +78,37 @@ public function countRemainingCodes(string $userId): int
+ $this->decryptSecret($secret),
+ $code
+ );
+ if ($timestep === null || $user->isTotpTimestepReplay($timestep)) {
+ return null;
+ }
</file context>
Contributor
Author
There was a problem hiding this comment.
Two concurrent completions for the same pending session cannot both succeed: consumePendingSessionOrFail() -> consumeIfActive() is an atomic conditional delete, so only one request issues tokens; the loser gets UnauthorizedHttpException. The recorded TOTP watermark is defense-in-depth on top of that authoritative single-use gate. The watermark is also now monotonic (see latest commit). Declining the broader atomic-write redesign as out of scope; the replay guarantee holds.
There was a problem hiding this comment.
Got it — thanks for the clarification.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 551
Lines 9541 9628 +87
=========================================
+ Hits 9541 9628 +87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
Split oversized test classes that exceeded phpmd TooManyMethods (max 25) into cohesive sibling classes sharing abstract base TestCases. No coverage lost — every test and assertion preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| self::assertCount(1, $result); | ||
| self::assertSame('twofa_setup', $result[0]['name']); | ||
| self::assertSame('user:' . $subject, $result[0]['key']); |
|
|
||
| self::assertCount(1, $result); | ||
| self::assertSame('twofa_confirm', $result[0]['name']); | ||
| self::assertSame('user:' . $subject, $result[0]['key']); |
|
|
||
| self::assertCount(1, $result); | ||
| self::assertSame('twofa_disable', $result[0]['name']); | ||
| self::assertSame('user:' . $subject, $result[0]['key']); |
| $request = Request::create('/api/2fa/setup', 'GET'); | ||
| $request->headers->set('Authorization', 'Bearer ' . $token); | ||
|
|
||
| self::assertSame([], $resolver->resolve($request)); |
| $request = Request::create('/api/2fa/unknown', 'POST'); | ||
| $request->headers->set('Authorization', 'Bearer ' . $token); | ||
|
|
||
| self::assertSame([], $resolver->resolve($request)); |
| 'EF55-GH66', | ||
| $this->faker->ipv4(), | ||
| $this->faker->userAgent() | ||
| )); |
| '123456', | ||
| $this->faker->ipv4(), | ||
| $this->faker->userAgent() | ||
| )); |
| $this->twoFactorCodeVerifier = $this->createMock(TwoFactorCodeValidatorInterface::class); | ||
| $this->events = $this->createMock(TwoFactorPublisherInterface::class); | ||
| $this->userFactory = new UserFactory(); | ||
| $this->uuidTransformer = new UuidTransformer(new SharedUuidFactory()); |
There was a problem hiding this comment.
5 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php">
<violation number="1" location="src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php:240">
P1: Failed-attempt tracking is non-atomic, so concurrent invalid 2FA requests can lose increments and bypass the 5-attempt cap.</violation>
</file>
<file name="src/User/Application/Validator/TwoFactorCodeValidator.php">
<violation number="1" location="src/User/Application/Validator/TwoFactorCodeValidator.php:104">
P1: TOTP replay protection is not atomic: concurrent requests can both pass replay check and accept the same timestep.</violation>
</file>
<file name="tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTwoFactorEndpointsTest.php">
<violation number="1" location="tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTwoFactorEndpointsTest.php:18">
P3: These three test methods (setup/confirm/disable) are nearly identical—only the endpoint path and expected limiter name differ. Consider using a `@dataProvider` to eliminate the repetition and make it trivial to add new 2FA endpoint variants.</violation>
</file>
<file name="tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerBruteForceTest.php">
<violation number="1" location="tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerBruteForceTest.php:17">
P3: The recovery-code and TOTP verification-failure tests share an almost identical structure (setup, mock configuration, assertion). Extract a parameterized helper or use a data provider to reduce the 22-line duplication across these cases.</violation>
</file>
<file name="tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerTestCase.php">
<violation number="1" location="tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerTestCase.php:38">
P3: The setUp body here is reported as identical to code in 2 other test-case locations. If those other locations now extend this base class, they can remove their own setUp (calling `parent::setUp()` only). Otherwise, consider whether a shared trait or further base-class consolidation could eliminate the remaining duplicates.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| GRAPHQL; | ||
|
|
||
| public function testResolveReturnsTwoFaSetupLimiterWhenUserAuthenticated(): void |
There was a problem hiding this comment.
P3: These three test methods (setup/confirm/disable) are nearly identical—only the endpoint path and expected limiter name differ. Consider using a @dataProvider to eliminate the repetition and make it trivial to add new 2FA endpoint variants.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTwoFactorEndpointsTest.php, line 18:
<comment>These three test methods (setup/confirm/disable) are nearly identical—only the endpoint path and expected limiter name differ. Consider using a `@dataProvider` to eliminate the repetition and make it trivial to add new 2FA endpoint variants.</comment>
<file context>
@@ -0,0 +1,211 @@
+ }
+ GRAPHQL;
+
+ public function testResolveReturnsTwoFaSetupLimiterWhenUserAuthenticated(): void
+ {
+ $subject = $this->faker->uuid();
</file context>
| final class CompleteTwoFactorCommandHandlerBruteForceTest extends | ||
| CompleteTwoFactorCommandHandlerTestCase | ||
| { | ||
| public function testInvokeThrowsUnauthorizedWhenRecoveryCodeVerificationFails(): void |
There was a problem hiding this comment.
P3: The recovery-code and TOTP verification-failure tests share an almost identical structure (setup, mock configuration, assertion). Extract a parameterized helper or use a data provider to reduce the 22-line duplication across these cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerBruteForceTest.php, line 17:
<comment>The recovery-code and TOTP verification-failure tests share an almost identical structure (setup, mock configuration, assertion). Extract a parameterized helper or use a data provider to reduce the 22-line duplication across these cases.</comment>
<file context>
@@ -0,0 +1,193 @@
+final class CompleteTwoFactorCommandHandlerBruteForceTest extends
+ CompleteTwoFactorCommandHandlerTestCase
+{
+ public function testInvokeThrowsUnauthorizedWhenRecoveryCodeVerificationFails(): void
+ {
+ $user = $this->createTwoFactorEnabledUser();
</file context>
| protected UuidTransformer $uuidTransformer; | ||
|
|
||
| #[\Override] | ||
| protected function setUp(): void |
There was a problem hiding this comment.
P3: The setUp body here is reported as identical to code in 2 other test-case locations. If those other locations now extend this base class, they can remove their own setUp (calling parent::setUp() only). Otherwise, consider whether a shared trait or further base-class consolidation could eliminate the remaining duplicates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerTestCase.php, line 38:
<comment>The setUp body here is reported as identical to code in 2 other test-case locations. If those other locations now extend this base class, they can remove their own setUp (calling `parent::setUp()` only). Otherwise, consider whether a shared trait or further base-class consolidation could eliminate the remaining duplicates.</comment>
<file context>
@@ -0,0 +1,204 @@
+ protected UuidTransformer $uuidTransformer;
+
+ #[\Override]
+ protected function setUp(): void
+ {
+ parent::setUp();
</file context>
…ifecycle for #314 Scope the TOTP replay/timestep guard to the sign-in completion (and disable) flows. The setup-confirmation path (/api/2fa/confirm) no longer consumes the sign-in replay time-step, so a sign-in completion using a code from the same 30s window is no longer wrongly rejected (was returning 401 instead of 200). The CWE-294 finding targets sign-in code replay; setup confirmation is a one-time enable step that issues no session, so it must not advance the replay counter. Replay protection on /api/signin/2fa and /api/2fa/disable is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| $this->encryptor, | ||
| $this->recoveryCodeRepository, | ||
| $this->userRepository, | ||
| ); |
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked by 8 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php (1)
175-178: 💤 Low valueRemove inline comment; the method name is self-explanatory.
The comment explains why the brute-force counter is not incremented, but the method name
failWithoutCountingAttemptand the code structure (calling it in the recovery-code-failure catch block after session consumption) already convey this intent.♻️ Proposed cleanup
} catch (UnauthorizedHttpException) { - // The pending session was already consumed before this point, so the - // brute-force counter is no longer applicable here. $this->failWithoutCountingAttempt($command); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php` around lines 175 - 178, Remove the inline explanatory comment immediately above the call to $this->failWithoutCountingAttempt($command) in CompleteTwoFactorCommandHandler (the recovery-code-failure catch path); the method name is self-explanatory so delete that comment line only and leave the method call and surrounding control flow intact.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/security-314-2fa-bruteforce-totp-replay/stories.md`:
- Around line 7-8: The docker run command contains developer-specific absolute
paths (/home/kravtsov/Projects/secfix-314 and
/home/kravtsov/Projects/user-service/vendor) which break portability; update the
docker run line to use relative paths or environment/placeholders (e.g., ./,
${PROJECT_ROOT}, ${VENDOR_DIR}) and/or mention in the docs that users must
replace these placeholders with their local paths so the -v mounts work for
other contributors.
- Around line 5-13: The fenced code block in
specs/security-314-2fa-bruteforce-totp-replay/stories.md is missing a language
specifier; update the opening fence for the shell/PHP command block used to run
unit tests (the block that begins with "# Unit (focused)" and the docker run
command) from "```" to "```bash" so the snippet is annotated for syntax
highlighting.
---
Nitpick comments:
In `@src/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.php`:
- Around line 175-178: Remove the inline explanatory comment immediately above
the call to $this->failWithoutCountingAttempt($command) in
CompleteTwoFactorCommandHandler (the recovery-code-failure catch path); the
method name is self-explanatory so delete that comment line only and leave the
method call and surrounding control flow intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acfa37e9-3823-4361-9a46-c817a4fc6f2a
📒 Files selected for processing (29)
config/doctrine/User/PendingTwoFactor.mongodb.xmlconfig/doctrine/User/User.mongodb.xmlspecs/security-314-2fa-bruteforce-totp-replay/prd.mdspecs/security-314-2fa-bruteforce-totp-replay/stories.mdsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolver.phpsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitClientIdentityResolver.phpsrc/Shared/Application/Resolver/RateLimit/ApiRateLimitPayloadValueResolver.phpsrc/User/Application/CommandHandler/CompleteTwoFactorCommandHandler.phpsrc/User/Application/CommandHandler/ConfirmTwoFactorCommandHandler.phpsrc/User/Application/Validator/TOTPValidatorInterface.phpsrc/User/Application/Validator/TwoFactorCodeValidator.phpsrc/User/Application/Validator/TwoFactorCodeValidatorInterface.phpsrc/User/Domain/Entity/PendingTwoFactor.phpsrc/User/Domain/Entity/User.phpsrc/User/Infrastructure/Validator/TOTPValidator.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTest.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTwoFactorEndpointsTest.phptests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitPayloadValueResolverTest.phptests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerBruteForceTest.phptests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerTest.phptests/Unit/User/Application/CommandHandler/CompleteTwoFactorCommandHandlerTestCase.phptests/Unit/User/Application/CommandHandler/ConfirmTwoFactorCommandHandlerTest.phptests/Unit/User/Application/Verifier/TwoFactorCodeSetupVerificationTest.phptests/Unit/User/Application/Verifier/TwoFactorCodeVerifierTest.phptests/Unit/User/Domain/Entity/PendingTwoFactorTest.phptests/Unit/User/Domain/Entity/UserTest.phptests/Unit/User/Domain/Entity/UserTestCase.phptests/Unit/User/Domain/Entity/UserTwoFactorTest.phptests/Unit/User/Infrastructure/Validator/TOTPValidatorTest.php
💤 Files with no reviewable changes (1)
- tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitAuthTargetResolverTest.php
- Make recordAcceptedTotpTimestep() monotonic so the TOTP replay watermark only ever moves forward, hardening against out-of-order writes (cubic P2). - Assert brute-force counter persistence in assertInvalidTwoFactorCodeRejected so the helper verifies the security-critical increment (cubic P2). - Assert the IP limiter key in the GraphQL both-limiters test for parity with the REST case (cubic P2). - Use portable placeholders and a bash fence in stories.md (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
| self::assertSame('twofa_verification_ip', $result[0]['name']); | ||
| self::assertSame('ip:' . $clientIp, $result[0]['key']); | ||
| self::assertSame('twofa_verification_user', $result[1]['name']); | ||
| self::assertSame('user:' . $userId, $result[1]['key']); |
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
The monotonic guard added to User::recordAcceptedTotpTimestep introduced an equivalent mutant at its `<=` boundary (skip vs assign-the-same-value is unobservable), which cannot be killed and dropped Infection MSI below the required 100%. The guard is redundant: TwoFactorCodeValidator rejects replayed or older codes via isTotpTimestepReplay() before calling this setter, so only forward-moving time-steps ever reach it. Reverted to a plain assignment; single-use TOTP replay protection is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Domain/Entity/User.php">
<violation number="1" location="src/User/Domain/Entity/User.php:197">
P2: `recordAcceptedTotpTimestep()` no longer enforces monotonic updates, so a stale caller can regress the replay watermark and re-open acceptance of previously used TOTP steps.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| /** | ||
| * Records a successfully accepted TOTP time-step so the same (or older) | ||
| * code can never be replayed within its validity window. Callers MUST gate | ||
| * this on {@see isTotpTimestepReplay()} — the validator rejects replayed or |
There was a problem hiding this comment.
P2: recordAcceptedTotpTimestep() no longer enforces monotonic updates, so a stale caller can regress the replay watermark and re-open acceptance of previously used TOTP steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/User/Domain/Entity/User.php, line 197:
<comment>`recordAcceptedTotpTimestep()` no longer enforces monotonic updates, so a stale caller can regress the replay watermark and re-open acceptance of previously used TOTP steps.</comment>
<file context>
@@ -193,19 +193,13 @@ public function isTotpTimestepReplay(int $timestep): bool
- * ever moves forward, so out-of-order writes cannot re-open acceptance of an
- * already-consumed time-step.
+ * code can never be replayed within its validity window. Callers MUST gate
+ * this on {@see isTotpTimestepReplay()} — the validator rejects replayed or
+ * older codes before recording — so only forward-moving time-steps ever
+ * reach this setter.
</file context>
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
2 participants
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.
Security fix — Closes #314
Vulnerability
The second authentication factor (a 6-digit TOTP) was both brute-forceable and replayable:
CompleteTwoFactorCommandHandlernever incremented a per-pending-session failure counter and never invalidated thePendingTwoFactoron a wrong code, so onependingSessionIdallowed unlimited guesses for its 5-minute TTL. The Symfony rate limiter attached only by REST path (/api/signin/2fa), so the GraphQLcompleteTwoFactormutation atPOST /api/graphqlhad no per-user 2FA throttling and could brute-force the 10^6 TOTP space.TOTPValidator::verify()accepted the previous, current and next time window (~90s) and never recorded code usage, so a valid TOTP could be replayed (relay/phishing) to/api/signin/2fa,/api/2fa/confirm, and/api/2fa/disable. RFC 6238 §5.2 requires a previously accepted OTP be rejected.Fix
PendingTwoFactornow carriesfailedAttemptswithMAX_FAILED_ATTEMPTS = 5. The handler increments and persists it on each invalid code and deletes the pending session once exhausted; already-exhausted sessions are rejected up front. This protects both REST and GraphQL.UsertrackslastAcceptedTotpTimestep.TwoFactorCodeValidatorrejects any code whose matched time-step is<=the stored value and advances + persists it atomically on success (mirroring the recovery-code consume path).TOTPValidatornow accepts only the current and previous windows (future window removed) and exposes the matched time-step viaresolveAcceptedTimestep(). Disabling 2FA clears the stored time-step.twofa_verification_ip/twofa_verification_userlimiters are now applied to the GraphQLcompleteTwoFactormutation, resolvingpendingSessionIdfrom nestedvariables.input.Hexagonal/DDD boundaries preserved (Domain stays framework-free); no threshold/config relaxation; no new directories or
*Servicesuffixes.Local verification (one-off containers,
APP_ENV=test)TwoFactor|TOTPfilter): OK (224 tests); full Unit suite: OK (2283 tests)BMAD spec
specs/security-314-2fa-bruteforce-totp-replay/(prd.md,stories.md).🤖 Generated with Claude Code
Summary by cubic
Hardened 2FA with a server-side brute‑force counter, scoped TOTP replay protection with a forward‑only replay watermark, and GraphQL rate‑limiter parity. Setup confirmation verifies TOTP without consuming the time‑step so a sign‑in in the same 30s window works (closes #314).
Bug Fixes
failedAttemptsonPendingTwoFactor(max 5); increment on invalid codes, reject exhausted sessions up front, delete when exhausted; don’t count after the session is consumed. Applies to REST and GraphQL.lastAcceptedTotpTimesteponUser; reject same/older time‑steps on/api/signin/2faand/api/2fa/disable; disabling 2FA clears it. TOTP now accepts only current and previous windows. Setup confirmation (/api/2fa/confirm) verifies TOTP without advancing the counter.twofa_verification_ipandtwofa_verification_userto the GraphQLcompleteTwoFactormutation by detecting the operation inqueryand resolvingpendingSessionIdfromvariables.input.Refactors
resolveAcceptedTimestepto the TOTP validator; introduced a non‑consumingverifyTotpForSetupOrFail; improved nested GraphQL payload resolution; split oversized tests.User::recordAcceptedTotpTimestep(); monotonicity is enforced by the validator, replay protection unchanged.Written for commit 681fb6d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests