Refactor Fortify two-factor dependencies#413
Conversation
Document the agreed Fortify two-factor refactor before the implementation commits. Capture the dependency research, architecture decisions, implementation outline, testing plan, and review context for replacing pragmarx/google2fa and Bacon QR generation with OTPHP and chillerlan.
Replace the old Fortify two-factor dependency set in the monorepo and subtree manifests. Remove bacon/bacon-qr-code and pragmarx/google2fa, then declare chillerlan/php-qrcode, spomky-labs/otphp, psr/clock, and Carbon where Fortify imports them directly.
Move Fortify's two-factor provider from a shared Google2FA engine to OTPHP with mandatory PSR clock injection. Generate base32-length-compatible secrets, build the explicit otpauth URL shape Fortify exposes today, sweep configured step windows with fresh per-secret TOTP objects, and cache accepted timecodes for the full accepted replay window. Bind the provider with Carbon's immutable clock factory and the cache repository so the singleton holds no per-secret or per-request mutable OTP state.
Replace Google2FA-oriented provider tests with OTPHP-focused coverage around Fortify's provider contract. Cover generated secret lengths, invalid lengths, otpauth URL shape and reserved-character encoding, configured window boundaries, replay key isolation, replay rejection, replay TTLs, no-cache verification, generated-secret verification, and RFC 6238 vectors. Add a shared fixed PSR clock fixture so time-sensitive provider tests stay deterministic without global clock state.
Add Hypervel-specific coverage for the new OTPHP provider shape under concurrent coroutines. Verify one shared provider can validate different secrets at the same time without retained per-secret OTP state, and verify Fortify's configured window is resolved per verification call instead of being stored on a mutable engine.
Move Fortify's HTTP and controller two-factor tests away from resolving Google2FA from the container. Mint valid challenge and confirmation codes with OTPHP and Carbon's immutable clock so the integration tests exercise the same clocked TOTP model used by the production provider binding.
Move two-factor QR SVG generation out of the authenticatable trait and into a concrete stateless renderer backed by chillerlan/php-qrcode. Create a fresh QRCode instance for every render to avoid retained segment state, map all chillerlan module types to Fortify's dark and light colors, emit raw SVG rather than a data URI, and keep Fortify's fixed 192px SVG output through a small SVG output class.
Add focused QR renderer tests for raw SVG output, fixed dimensions, XML validity, no quiet-zone padding, Fortify colors, no default black output, and fresh rendering without segment accumulation. Update Fortify package docs and Boost docs to describe the OTPHP provider, chillerlan QR renderer, 32-character TOTP secrets, step-based windows, and full-window replay-cache TTL behavior.
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces Fortify’s legacy 2FA libraries with OTPHP and chillerlan, rewires the provider around an injected clock, updates QR rendering, and refreshes the related docs and tests. ChangesFortify 2FA library migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FortifyServiceProvider
participant TwoFactorAuthenticationProvider
participant TOTP
participant Cache
FortifyServiceProvider->>TwoFactorAuthenticationProvider: singleton(clock, cache)
TwoFactorAuthenticationProvider->>TOTP: createFromSecret(secret)
TwoFactorAuthenticationProvider->>TwoFactorAuthenticationProvider: match timecodes in window
TwoFactorAuthenticationProvider->>Cache: add(replayKey, matchedTimecode, ttl)
Cache-->>TwoFactorAuthenticationProvider: accepted / rejected
sequenceDiagram
participant TwoFactorAuthenticatable
participant TwoFactorQrCodeRenderer
participant TwoFactorQrCodeSvgOutput
TwoFactorAuthenticatable->>TwoFactorQrCodeRenderer: svg(twoFactorQrCodeUrl())
TwoFactorQrCodeRenderer->>TwoFactorQrCodeSvgOutput: render QROptions
TwoFactorQrCodeSvgOutput-->>TwoFactorQrCodeRenderer: SVG string
TwoFactorQrCodeRenderer-->>TwoFactorAuthenticatable: trimmed SVG
Related Issues: None found. Related PRs: None found. Suggested labels: enhancement, dependencies, breaking-change Suggested reviewers: None found. Poem:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR replaces the Fortify two-factor dependency stack (
Confidence Score: 5/5Safe to merge — the refactor is complete, well-bounded, and backed by comprehensive tests covering all critical paths. All changed behavior is exercised by deterministic unit tests using FixedClock and ArrayStore. The atomic cache->add() replay strategy is correct and strictly safer than the previous get/put sequence. The TTL formula fix, URL-encoding improvements (including the secret parameter addressed in a previous cycle), and QR renderer extraction are all complete and consistent with the stated goals. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "test: make Fortify replay cache window e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (4)
src/fortify/src/TwoFactorAuthenticationProvider.php (1)
27-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNullable
$cachesilently disables replay protection.If
$cacheis omitted,verify()'s$this->cache?->get(...)/?->put(...)become no-ops, so replay protection is silently turned off with no warning. GivenFortifyServiceProvideralways injectsRepository, production is fine, but any other consumer constructing this class directly loses this security guarantee unnoticed.🛡️ Consider making cache mandatory or logging a warning
- private readonly ?Repository $cache = null, + private readonly Repository $cache,🤖 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/fortify/src/TwoFactorAuthenticationProvider.php` around lines 27 - 31, The nullable $cache in TwoFactorAuthenticationProvider lets verify() silently skip replay protection when no Repository is provided. Update the constructor and dependency usage so cache is required, or add an explicit warning/fail-fast path when it is missing; ensure verify() and the cache-backed replay checks cannot run as no-ops for direct consumers.src/fortify/src/FortifyServiceProvider.php (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider resolving
ClockInterfacefrom the container instead of hardcodingnew FactoryImmutable.
Carbon\FactoryImmutabledoes implementPsr\Clock\ClockInterface, so this works functionally. class FactoryImmutable extends Factory implements ClockInterface However, hardcoding the concrete class here bypasses the container, so the app can't swap in a differentClockInterfacebinding (e.g. for app-wide time-freezing in tests) without changing this provider directly — somewhat undercutting the "mandatory PSR clock injection" design goal.♻️ Bind and resolve ClockInterface
+ $this->app->bind(ClockInterface::class, FactoryImmutable::class); + $this->app->singleton(TwoFactorAuthenticationProviderContract::class, function ($app): TwoFactorAuthenticationProvider { return new TwoFactorAuthenticationProvider( - new FactoryImmutable, + $app->make(ClockInterface::class), $app->make(Repository::class), ); });🤖 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/fortify/src/FortifyServiceProvider.php` around lines 71 - 76, Resolve the clock through the container in FortifyServiceProvider’s TwoFactorAuthenticationProvider binding instead of instantiating FactoryImmutable directly. Update the singleton closure to request ClockInterface from $app and pass that into TwoFactorAuthenticationProvider, while keeping the Repository dependency resolved from the container. This preserves the mandatory PSR clock injection design and lets the app override the clock binding for tests or custom time behavior.tests/Fortify/AuthenticatedSessionControllerWithTwoFactorTest.php (1)
232-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReal-clock usage risks rare flakiness at period boundaries.
$previousTimestampand the request dispatch both depend on wall-clockFactoryImmutable::now()calls made at slightly different times. If the test happens to execute right as a 30-second TOTP period boundary rolls over between computing$previousTimestampand the provider's ownverify()call (which uses its own independently-instantiated clock), the "previous period" code could unexpectedly land in a different relative timecode than intended, causing an intermittent CI failure.Freezing the clock for this test (e.g. via
FactoryImmutable::setTestNow(), or binding a fixed clock like theFixedClockfixture used in the provider tests) would eliminate this boundary race.🤖 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 `@tests/Fortify/AuthenticatedSessionControllerWithTwoFactorTest.php` around lines 232 - 264, The two-factor challenge test uses real-time `FactoryImmutable::now()`/`TOTP::generate()` calls, which can drift across a TOTP boundary and make the `testTwoFactorChallengeFailsForOldOtpAndZeroWindow` case flaky. Freeze the clock for this test by using a fixed `FactoryImmutable` test time or a fixed clock fixture, then derive `$previousTimestamp`, `$userSecret`, and the OTP from that same frozen time before posting to `/two-factor-challenge`. Keep the assertions unchanged, but ensure the setup and the challenge request both rely on the same stable clock source.tests/Fortify/TwoFactorAuthenticationProviderTest.php (1)
119-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded OTP magic string could be derived instead.
'095740'is hardcoded intestReplayCacheKeyIncludesSecretAndCodeandtestAllowsSameCodeForDifferentSecretsrather than computed via the existingcodeAt()helper. This works only because it happens to coincide with the TOTP output for both secrets atself::TIMESTAMP; deriving it viacodeAt()(as done elsewhere in the file) would make the test self-documenting and immune to drift if the vectors/timestamp ever change.♻️ Example refactor
- $code = '095740'; $firstSecret = 'GBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'; $secondSecret = 'QVAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'; + $code = $this->codeAt($firstSecret, self::TIMESTAMP); + $this->assertSame($code, $this->codeAt($secondSecret, self::TIMESTAMP));🤖 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 `@tests/Fortify/TwoFactorAuthenticationProviderTest.php` around lines 119 - 153, The tests in TwoFactorAuthenticationProviderTest use a hardcoded OTP string instead of deriving it from the existing codeAt() helper. Update testReplayCacheKeyIncludesSecretAndCode and testAllowsSameCodeForDifferentSecrets to compute the OTP from the relevant secret and self::TIMESTAMP via codeAt(), so the assertions stay aligned with the provider behavior and remain easy to maintain.
🤖 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 `@src/fortify/src/TwoFactorAuthenticationProvider.php`:
- Around line 71-91: The replay protection in
TwoFactorAuthenticationProvider::verify() is subject to a TOCTOU race because it
uses cache->get() followed by cache->put() for the same replay key. Replace this
check-then-set flow with an atomic cache operation, using the existing
replayCacheKey() and replayTtl() logic, so only the first concurrent request can
record the accepted timecode. Keep the matchingTimecode() and window()
validation as-is, but ensure the cache write is conditional and atomic rather
than a separate read and write.
---
Nitpick comments:
In `@src/fortify/src/FortifyServiceProvider.php`:
- Around line 71-76: Resolve the clock through the container in
FortifyServiceProvider’s TwoFactorAuthenticationProvider binding instead of
instantiating FactoryImmutable directly. Update the singleton closure to request
ClockInterface from $app and pass that into TwoFactorAuthenticationProvider,
while keeping the Repository dependency resolved from the container. This
preserves the mandatory PSR clock injection design and lets the app override the
clock binding for tests or custom time behavior.
In `@src/fortify/src/TwoFactorAuthenticationProvider.php`:
- Around line 27-31: The nullable $cache in TwoFactorAuthenticationProvider lets
verify() silently skip replay protection when no Repository is provided. Update
the constructor and dependency usage so cache is required, or add an explicit
warning/fail-fast path when it is missing; ensure verify() and the cache-backed
replay checks cannot run as no-ops for direct consumers.
In `@tests/Fortify/AuthenticatedSessionControllerWithTwoFactorTest.php`:
- Around line 232-264: The two-factor challenge test uses real-time
`FactoryImmutable::now()`/`TOTP::generate()` calls, which can drift across a
TOTP boundary and make the `testTwoFactorChallengeFailsForOldOtpAndZeroWindow`
case flaky. Freeze the clock for this test by using a fixed `FactoryImmutable`
test time or a fixed clock fixture, then derive `$previousTimestamp`,
`$userSecret`, and the OTP from that same frozen time before posting to
`/two-factor-challenge`. Keep the assertions unchanged, but ensure the setup and
the challenge request both rely on the same stable clock source.
In `@tests/Fortify/TwoFactorAuthenticationProviderTest.php`:
- Around line 119-153: The tests in TwoFactorAuthenticationProviderTest use a
hardcoded OTP string instead of deriving it from the existing codeAt() helper.
Update testReplayCacheKeyIncludesSecretAndCode and
testAllowsSameCodeForDifferentSecrets to compute the OTP from the relevant
secret and self::TIMESTAMP via codeAt(), so the assertions stay aligned with the
provider behavior and remain easy to maintain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73dce62f-4c8e-49c6-b017-af53427d1927
📒 Files selected for processing (16)
composer.jsondocs/plans/2026-07-03-fortify-otphp-chillerlan-refactor.mdsrc/boost/docs/fortify.mdsrc/fortify/README.mdsrc/fortify/composer.jsonsrc/fortify/src/FortifyServiceProvider.phpsrc/fortify/src/TwoFactorAuthenticatable.phpsrc/fortify/src/TwoFactorAuthenticationProvider.phpsrc/fortify/src/TwoFactorQrCodeRenderer.phpsrc/fortify/src/TwoFactorQrCodeSvgOutput.phptests/Fortify/AuthenticatedSessionControllerWithTwoFactorTest.phptests/Fortify/Fixtures/FixedClock.phptests/Fortify/TwoFactorAuthenticationControllerTest.phptests/Fortify/TwoFactorAuthenticationProviderCoroutineSafetyTest.phptests/Fortify/TwoFactorAuthenticationProviderTest.phptests/Fortify/TwoFactorQrCodeRendererTest.php
There was a problem hiding this comment.
Pull request overview
Refactors Hypervel Fortify’s two-factor authentication implementation by replacing the underlying TOTP and QR generation dependencies, improving replay protection correctness, and updating tests/docs to match the new behavior while keeping Fortify’s public API shape familiar.
Changes:
- Swap TOTP implementation to OTPHP with mandatory PSR clock injection and step-window verification logic.
- Replace Bacon QR SVG generation with a dedicated chillerlan-based renderer that preserves Fortify’s raw 192px SVG output.
- Update tests, docs, and composer manifests to reflect the new dependencies and replay-cache TTL behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Fortify/TwoFactorQrCodeRendererTest.php | Adds focused SVG shape/color/regression coverage for the new QR renderer. |
| tests/Fortify/TwoFactorAuthenticationProviderTest.php | Reworks provider tests to validate OTPHP behavior, window boundaries, replay TTL, and QR URL encoding. |
| tests/Fortify/TwoFactorAuthenticationProviderCoroutineSafetyTest.php | Adds coroutine-safety coverage for concurrent verification and per-call window resolution. |
| tests/Fortify/TwoFactorAuthenticationControllerTest.php | Updates HTTP/controller tests to mint OTPs via OTPHP instead of Google2FA. |
| tests/Fortify/AuthenticatedSessionControllerWithTwoFactorTest.php | Updates login challenge tests to mint OTPs via OTPHP and handle window edge case logic. |
| tests/Fortify/Fixtures/FixedClock.php | Adds deterministic PSR clock fixture for stable TOTP tests. |
| src/fortify/src/TwoFactorQrCodeSvgOutput.php | Introduces an SVG output adapter to preserve fixed 192px Fortify SVG headers. |
| src/fortify/src/TwoFactorQrCodeRenderer.php | Adds a stateless QR renderer that creates fresh QRCode instances per render. |
| src/fortify/src/TwoFactorAuthenticationProvider.php | Replaces Google2FA engine with OTPHP + injected clock; fixes replay TTL semantics and window validation. |
| src/fortify/src/TwoFactorAuthenticatable.php | Delegates QR SVG rendering to the new renderer instead of inline Bacon generation. |
| src/fortify/src/FortifyServiceProvider.php | Updates provider binding to construct the 2FA provider with a clock implementation. |
| src/fortify/README.md | Updates Fortify “Differences From Laravel” to reflect OTPHP/chillerlan behavior and replay policy. |
| src/boost/docs/fortify.md | Updates canonical docs to describe step-window semantics and full-window replay caching. |
| src/fortify/composer.json | Swaps dependency set from Bacon/Google2FA to chillerlan/OTPHP plus clock deps. |
| composer.json | Updates monorepo dependencies to include OTPHP/chillerlan and remove old libs. |
| docs/plans/2026-07-03-fortify-otphp-chillerlan-refactor.md | Adds a detailed implementation/verification plan documenting the rationale and approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Require the Fortify two-factor provider to receive a cache repository so replay protection cannot be silently disabled by direct construction. Replace the replay-cache get/put sequence with Repository::add() so accepted TOTP codes are recorded with add-if-absent semantics and concurrent replays are rejected by cache stores with atomic add support. Encode the otpauth secret query parameter for externally supplied or padded secrets while preserving generated base32 secret URLs. Update provider tests to assert the exact replay cache keys and TTLs sent to add(), keep same-code/different-secret behavior covered, and pass in-memory cache repositories to direct provider constructions.
|
@greptile-apps @coderabbitai @copilot-pull-request-reviewer thanks for the review pass. Follow-up commit 517a733 addresses the actionable Fortify issues we agreed were valid:
A few reviewed items were intentionally left unchanged after checking them against the codebase:
Validation after the follow-up:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
gh pr view 413 --repo hypervel/components --json commits --jq '.commits[] | select(.oid | startswith("517a733")) | {oid, messageHeadline}'
echo "---"
git log --oneline -1 517a733fa 2>/dev/null || echo "commit not found locally"Length of output: 568
On the deferred items, your reasoning holds up:
Nice work getting the full suite green (21,160 tests / 60,040 assertions) with a clean CS/PHPStan pass. (っ˘̩╭╮˘̩)っ ✏️ Learnings added
|
|
The follow-up commit addresses everything that was agreed as valid. Here's a quick confirmation pass:
Test assertions on QR renderer test
The suite numbers (21,160 tests / 60,040 assertions, CS and Stan clean) give good coverage confidence. Nothing outstanding on my end — this is in good shape. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Fortify/TwoFactorAuthenticationProviderTest.php (1)
119-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest relies on implicit default
windowvalue.
$ttl = 90and the cache mock expectations assume the default feature window is1(giving TTL 90), but this test never callsFeatures::twoFactorAuthentication(['window' => ...]), unlike its sibling tests. If the default window changes, or Features config isn't reset between tests, this test will fail with a confusing mock argument-mismatch rather than a clear assertion failure.🔧 Proposed fix to make the assumption explicit
public function testReplayCacheKeyIncludesSecretAndCode(): void { + Features::twoFactorAuthentication(['window' => 1]); + $firstSecret = 'GBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA';🤖 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 `@tests/Fortify/TwoFactorAuthenticationProviderTest.php` around lines 119 - 139, The replay cache key test is relying on an implicit default two-factor window, which makes the expected cache TTL brittle. In testReplayCacheKeyIncludesSecretAndCode, explicitly configure Features::twoFactorAuthentication with the intended window value before creating the provider, so the CacheRepositoryContract::add expectations match the setup instead of depending on global defaults. Keep the assertions around fortify.2fa_codes and verify() unchanged.
🤖 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.
Nitpick comments:
In `@tests/Fortify/TwoFactorAuthenticationProviderTest.php`:
- Around line 119-139: The replay cache key test is relying on an implicit
default two-factor window, which makes the expected cache TTL brittle. In
testReplayCacheKeyIncludesSecretAndCode, explicitly configure
Features::twoFactorAuthentication with the intended window value before creating
the provider, so the CacheRepositoryContract::add expectations match the setup
instead of depending on global defaults. Keep the assertions around
fortify.2fa_codes and verify() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed0651b2-0723-4c68-9510-2a0570a755f3
📒 Files selected for processing (3)
src/fortify/src/TwoFactorAuthenticationProvider.phptests/Fortify/TwoFactorAuthenticationProviderCoroutineSafetyTest.phptests/Fortify/TwoFactorAuthenticationProviderTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Fortify/TwoFactorAuthenticationProviderCoroutineSafetyTest.php
Set the two-factor window explicitly in the replay cache key test so the expected 90-second TTL is tied to the test setup instead of relying on the provider default. This keeps the test intent clear if the default window ever changes.
Summary
For more details, see: docs/plans/2026-07-03-fortify-otphp-chillerlan-refactor.md
Tests
composer fixSummary by CodeRabbit