fix(security): hash password-reset tokens at rest (#313)#331
fix(security): hash password-reset tokens at rest (#313)#331dmytrocraft wants to merge 8 commits into
Conversation
Password-reset tokens were persisted verbatim as the password_reset_tokens
MongoDB document id and looked up by exact equality on the raw value. The
stored value equalled the bearer credential emailed to the user, so any
read access to the collection (backup, replica snapshot, query log,
read-only breach, NoSQL-injection sink) yielded directly usable, unexpired
reset tokens for every account with a pending reset, enabling account
takeover without further interaction (CWE-256).
Fix (mirrors the existing AuthRefreshToken hash-at-rest precedent):
- PasswordResetToken now stores only hash('sha256', $plainToken) as its
identifier and keeps the plaintext transiently (never persisted) for the
reset link; adds hashToken()/matchesToken() (constant-time hash_equals).
- MongoDBPasswordResetTokenRepository::findByToken() hashes the candidate
before querying, so lookups are by stored hash, never by raw plaintext.
- The request->email flow carries the plaintext via the domain event and
re-attaches it to the reconstituted entity so the reset link stays usable.
- Seeders/fixtures/E2E helpers replay the plaintext captured at creation.
Email-confirmation tokens (Redis-backed) are out of scope (low severity,
different storage/serialization contract); documented in the BMAD spec.
Local verification (one-off containers):
- phpunit Unit: OK (2265 tests, 6234 assertions)
- deptrac: Violations 0
- psalm (changed files): No errors found
- php-cs-fixer (changed files): 0 of 19 files need fixing
BMAD spec: specs/security-313-reset-confirm-tokens-plaintext/
Closes #313
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? |
|
Warning Review limit reached
More reviews will be available in 2 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 21 files
Confidence score: 3/5
- There is meaningful regression risk in the password reset flow:
src/User/Domain/Entity/PasswordResetToken.phpleaves nullable transientplainTokenuninitialized, which can trigger runtime fatals on hydrated entities. src/User/Application/Factory/Event/PasswordResetEmailSendEventFactory.phpmay fall back togetTokenValue()and email a hashed token, which can produce invalid reset links for users when plaintext is missing.- This lands at a moderate risk score because the top issues are concrete and user-facing (reset failures/errors), but they appear localized and fixable before or shortly after merge.
- Pay close attention to
src/User/Domain/Entity/PasswordResetToken.php,src/User/Application/Factory/Event/PasswordResetEmailSendEventFactory.php, andsrc/User/Application/CommandHandler/RequestPasswordResetCommandHandler.php- token nullability/fallback handling can cause fatals, invalid links, or empty published tokens.
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/RequestPasswordResetCommandHandler.php">
<violation number="1" location="src/User/Application/CommandHandler/RequestPasswordResetCommandHandler.php:48">
P2: Casting nullable `getPlainToken()` to string can silently publish an empty reset token (`null` -> `''`) instead of failing fast.</violation>
</file>
<file name="src/User/Application/Factory/Event/PasswordResetEmailSendEventFactory.php">
<violation number="1" location="src/User/Application/Factory/Event/PasswordResetEmailSendEventFactory.php:22">
P2: Fallback to `getTokenValue()` can send the hashed token in reset emails, producing invalid reset links when plaintext was not attached.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Web/Mobile Client
participant API as Password Reset API
participant Handler as RequestPasswordResetHandler
participant Factory as PasswordResetTokenFactory
participant Token as PasswordResetToken
participant Repo as MongoDBPasswordResetTokenRepository
participant DB as MongoDB
participant EventBus as Event Bus
participant Subscriber as PasswordResetRequestedEventSubscriber
participant EmailEvent as PasswordResetEmailSendEventFactory
participant EmailService as Email Delivery Service
Note over Client,EmailService: Password Reset Request Flow
Client->>API: POST /reset-password (email)
API->>Handler: __invoke()
Handler->>Factory: create(userId)
Factory->>Factory: generate random_bytes(32)
Factory->>Token: new PasswordResetToken(plainToken, userId, ...)
Token->>Token: hashToken(plainToken) = SHA-256
Token->>Token: store hash as tokenValue, keep plainToken transient
Factory-->>Handler: PasswordResetToken
Handler->>Repo: save(token)
Repo->>DB: persist hash (sha256 hex) only
DB-->>Repo: OK
Note over Handler,EventBus: Emit event with plainToken
Handler->>EventBus: publish PasswordResetRequestedEvent(plainToken)
EventBus->>Subscriber: __invoke(event)
Subscriber->>Repo: findByToken(event.plainToken)
Repo->>DB: query by hash(plainToken)
DB-->>Repo: matched entity (hash only)
Repo-->>Subscriber: PasswordResetToken (no plainToken)
Note over Subscriber: Re-attach plaintext for email
Subscriber->>Token: attachPlainToken(event.plainToken)
Subscriber->>EmailEvent: create(token, user)
EmailEvent->>Token: getPlainToken() → plaintext
EmailEvent->>Token: getTokenValue() → hash (NOT used)
EmailEvent-->>Subscriber: PasswordResetEmailSentEvent(plainToken)
Subscriber->>EmailService: dispatch email with plainToken in link
Note over Client,EmailService: Password Reset Confirm Flow
Client->>API: POST /confirm-reset (token=plainToken)
API->>Repo: findByToken(plainToken)
Repo->>Repo: hashToken(plainToken)
Repo->>DB: query by ['tokenValue' => hash]
DB-->>Repo: matched entity
Repo-->>API: PasswordResetToken
API->>Token: matchesToken(plainToken)
Token->>Token: hash_equals(storedHash, hash(plainToken))
alt Matches
API->>DB: update password, invalidate token
DB-->>API: OK
API-->>Client: 200 Success
else No match (hash-of-hash submitted)
API-->>Client: 401 Invalid token
end
Note over Token,DB: Attacker with DB read access
Note over DB: Attacker sees: hash(plainToken) = "a3f8c2..."
Note over API,Token: Attacker submits hash as token
API->>Token: matchesToken("a3f8c2...")
Token->>Token: hash("a3f8c2...") != stored hash
Token-->>API: false → reject
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 3 unresolved issues from previous reviews.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
=============================================
Coverage 100.00% 100.00%
- Complexity 0 2772 +2772
=============================================
Files 551 551
Lines 9541 9558 +17
=============================================
+ Hits 9541 9558 +17
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:
|
Resolve two CI failures introduced by the token-hashing fix:
- Replace static PasswordResetToken::hashToken() call in the repository
with the inline hash('sha256', ...) used by the sibling refresh-token
repository, clearing the PHPMD StaticAccess violation that broke the
Bats "make phpinsights" check.
- Default the transient $plainToken property to null so reads stay safe
after Doctrine ODM hydration (which bypasses the constructor), fixing
the Behat fatal error in the password-reset confirm flow.
- Add unit regression tests covering constructorless hydration; Infection
keeps 100% MSI on both changed files.
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.
1 issue found across 3 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/Infrastructure/Repository/MongoDBPasswordResetTokenRepository.php">
<violation number="1" location="src/User/Infrastructure/Repository/MongoDBPasswordResetTokenRepository.php:43">
P3: Token hashing logic is duplicated in the repository instead of using the domain’s canonical `hashToken()` helper, creating a drift risk.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| return $this->findOneBy(['tokenValue' => $token]); | ||
| return $this->findOneBy([ | ||
| 'tokenValue' => hash('sha256', $token), |
There was a problem hiding this comment.
P3: Token hashing logic is duplicated in the repository instead of using the domain’s canonical hashToken() helper, creating a drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/User/Infrastructure/Repository/MongoDBPasswordResetTokenRepository.php, line 43:
<comment>Token hashing logic is duplicated in the repository instead of using the domain’s canonical `hashToken()` helper, creating a drift risk.</comment>
<file context>
@@ -40,7 +40,7 @@ public function save(
{
return $this->findOneBy([
- 'tokenValue' => PasswordResetToken::hashToken($token),
+ 'tokenValue' => hash('sha256', $token),
]);
}
</file context>
| 'tokenValue' => hash('sha256', $token), | |
| 'tokenValue' => PasswordResetToken::hashToken($token), |
There was a problem hiding this comment.
Keeping the inline hash('sha256', $token) here intentionally. Routing through PasswordResetToken::hashToken() triggers the project's PHPMD cleancode StaticAccess rule (commit ba4c710 had already switched away from the static call for exactly that CI reason). Drift is not silent: MongoDBPasswordResetTokenRepositoryTest::testFindByTokenLooksUpByHashNotPlaintext pins the repository lookup to PasswordResetToken::hashToken(), so any divergence in the hashing algorithm fails the test suite. Added a comment documenting this.
There was a problem hiding this comment.
Got it — thanks for the clarification.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
The HTTP password-reset flow persists only the hashed token, so the session-integration scenario confirmed with the stored hash and got 404. Record the plaintext emitted on PasswordResetRequestedEvent during the request and replay it on confirmation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
…ns for #313 Behat drives the API over real HTTP (browserkit_http), so the PasswordResetRequestedEvent is dispatched in the web-server process while the confirmation step resolves the plaintext in the Behat CLI process. The in-process PasswordResetTokenRecorder therefore never captured the token for HTTP-driven reset flows, raising 'No plaintext password reset token was captured for confirmation.' Mirror the plaintext through the shared Redis instance (both processes use the same REDIS_URL) so it survives the process boundary; in-memory capture still serves in-process flows. Production hashing is untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
Fail fast when the transient plaintext password-reset token is missing instead of silently publishing an empty token or emailing the stored hash. Document why the repository hashes inline (PHPMD cleancode forbids static access) and rely on the existing repository test to pin both sides to the same algorithm against drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
1 issue found across 5 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="tests/Unit/User/Application/CommandHandler/RequestPasswordResetCommandHandlerTest.php">
<violation number="1" location="tests/Unit/User/Application/CommandHandler/RequestPasswordResetCommandHandlerTest.php:90">
P3: Missing expectation on `passwordResetTokenRepository->save()` — the handler calls `save()` before the plain-token check, so this call is exercised during the test but never explicitly verified. This inconsistency with the existing success test's `setupTokenFactoryExpectations()` reduces the test's regression-detection value.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ->with($email) | ||
| ->willReturn($user); | ||
|
|
||
| $this->passwordResetTokenFactory |
There was a problem hiding this comment.
P3: Missing expectation on passwordResetTokenRepository->save() — the handler calls save() before the plain-token check, so this call is exercised during the test but never explicitly verified. This inconsistency with the existing success test's setupTokenFactoryExpectations() reduces the test's regression-detection value.
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/RequestPasswordResetCommandHandlerTest.php, line 90:
<comment>Missing expectation on `passwordResetTokenRepository->save()` — the handler calls `save()` before the plain-token check, so this call is exercised during the test but never explicitly verified. This inconsistency with the existing success test's `setupTokenFactoryExpectations()` reduces the test's regression-detection value.</comment>
<file context>
@@ -70,6 +70,43 @@ public function testRequestPasswordResetForExistingUser(): void
+ ->with($email)
+ ->willReturn($user);
+
+ $this->passwordResetTokenFactory
+ ->expects($this->once())
+ ->method('create')
</file context>
Extract the missing-plain-token arrange block in RequestPasswordResetCommandHandlerTest into a private helper so the test method stays under the 20-line FunctionLength limit, restoring the phpinsights tests architecture score to 100. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Security fix — Closes #313
Vulnerability (CWE-256, HIGH)
Password-reset tokens were persisted verbatim as the
password_reset_tokensMongoDB document id (
<id field-name="tokenValue" strategy="NONE">) and lookedup by exact equality on the raw value (
findOneBy(['tokenValue' => $token])).The stored value was identical to the bearer credential emailed to the user, so
any read access to the collection (backup, replica snapshot, query log, read-only
breach, or a NoSQL-injection sink) yielded directly usable, unexpired reset
tokens for every account with a pending reset — enabling account takeover with no
further interaction. The refresh-token subsystem already hashed at rest
(
AuthRefreshToken→hash('sha256', …)); the reset path did not follow it.Fix
PasswordResetTokennow stores onlyhash('sha256', $plainToken)as itsidentifier and keeps the plaintext transiently (never persisted) for the
reset link. Adds
hashToken()andmatchesToken()(constant-timehash_equals).MongoDBPasswordResetTokenRepository::findByToken()hashes the incomingcandidate before querying, so lookups are by stored hash, never raw plaintext.
re-attaches it to the reconstituted entity (
attachPlainToken) so the emailedreset link stays usable.
captured at creation time, keeping all flows green.
A DB-read attacker now only sees a SHA-256 hash; submitting it to the confirm
endpoint fails (hash-of-hash never matches). Generation still uses
random_bytes(256-bit entropy) with unchanged single-use / short-expiry semantics.
Out of scope: email-confirmation tokens (
ConfirmationTokenvia Redis) —low severity, different cache key/serialization contract; documented in the spec.
Local verification (one-off containers)
OK (2265 tests, 6234 assertions)Violations 0No errors found0 of 19 filesneed fixingBMAD spec
specs/security-313-reset-confirm-tokens-plaintext/(prd.md,stories.md)🤖 Generated with Claude Code
Summary by cubic
Hashes password-reset tokens at rest to remove plaintext exposure (CWE-256). Lookups use SHA-256; plaintext exists only transiently for email links, and E2E flows replay it across processes via a Redis-backed recorder.
Bug Fixes
PasswordResetTokenstores onlyhash('sha256', $plain)and keeps a transientplainToken(defaultnull); addshashToken(), constant-timematchesToken(), andattachPlainToken().MongoDBPasswordResetTokenRepository::findByToken()hashes the candidate (hash('sha256', $token)) before querying.LogicException; subscriber re-attaches the plaintext before sending.PasswordResetTokenRecorderthat mirrors the plaintext to Redis to cross web/CLI boundaries; wired inservices_test.yaml. Contexts read the recorder first, then transient/in-memory values.Refactors
RequestPasswordResetCommandHandlerTestto satisfy phpinsights FunctionLength; CI passes.Written for commit d479844. Summary will update on new commits.