Throttle WebAuthn option endpoint - #4702
Conversation
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
📝 WalkthroughWalkthroughThe WebAuthn login options flow now masks unknown identified users with randomized responses. The registration options route now uses the named ChangesWebAuthn Options Protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟠 High · up to The PR changes public WebAuthn option handling, but unknown-user responses remain distinguishable from real-user responses, enabling account and credential metadata enumeration; the login-options route is also unthrottled, and the added registration throttle may not enforce the intended per-IP policy. These concrete security and availability risks should be fixed before merge. Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 008ab7a2-d1f0-4ce5-a960-a65e34fba805
📒 Files selected for processing (1)
routes/api_v2.php
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: d53702f5-5e8e-4a91-9068-32f1673bcc7d
📒 Files selected for processing (2)
app/Http/Controllers/WebAuthn/WebAuthnLoginController.phproutes/api_v2.php
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| $count = random_int(1, 3); | ||
| $credentials = []; | ||
| for ($i = 0; $i < $count; $i++) { | ||
| $credentials[] = [ | ||
| 'id' => strtr(base64_encode(random_bytes(96)), '+/', '-_'), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- controller ---'
sed -n '20,95p' app/Http/Controllers/WebAuthn/WebAuthnLoginController.php
printf '%s\n' '--- route ---'
sed -n '238,255p' routes/api_v2.php
printf '%s\n' '--- package lock entries ---'
rg -n -A12 -B3 '"name": "laragear/webauthn"' composer.lock
printf '%s\n' '--- relevant tests ---'
rg -n -i -A4 -B4 'WebAuthn|login/options|allowCredentials|generateFakeResponse' tests app routes composer.json composer.lockRepository: LycheeOrg/Lychee
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact locked AssertionRequest implementation ---'
curl -fsSL 'https://raw.githubusercontent.com/Laragear/WebAuthn/719ba978a18c95118e1ac5bba810522d0768ebb8/src/Http/Requests/AssertionRequest.php' | nl -ba | sed -n '1,220p'
printf '%s\n' '--- repository knowledge files ---'
find /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- WebAuthn test files ---'
git ls-files 'tests/*' | rg -i 'webauthn|auth'Repository: LycheeOrg/Lychee
Length of output: 3529
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact locked AssertionRequest implementation ---'
curl -fsSL 'https://raw.githubusercontent.com/Laragear/WebAuthn/719ba978a18c95118e1ac5bba810522d0768ebb8/src/Http/Requests/AssertionRequest.php' |
awk '{printf "%d:%s\n", NR, $0}' | sed -n '1,220p'
printf '%s\n' '--- feature-test conventions ---'
cat /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf/conventions/tests-feature-v2.md
printf '%s\n' '--- general test conventions ---'
cat /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf/conventions/tests.md
printf '%s\n' '--- existing WebAuthn feature test ---'
sed -n '1,260p' tests/Feature_v2/WebAuthTest.phpRepository: LycheeOrg/Lychee
Length of output: 14014
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining WebAuthn feature tests ---'
sed -n '245,430p' tests/Feature_v2/WebAuthTest.php
printf '%s\n' '--- locked AssertionCreation ---'
curl -fsSL 'https://raw.githubusercontent.com/Laragear/WebAuthn/719ba978a18c95118e1ac5bba810522d0768ebb8/src/Assertion/Creator/AssertionCreation.php' |
awk '{printf "%d:%s\n", NR, $0}' | sed -n '1,260p'
printf '%s\n' '--- locked AssertionCreator ---'
curl -fsSL 'https://raw.githubusercontent.com/Laragear/WebAuthn/719ba978a18c95118e1ac5bba810522d0768ebb8/src/Assertion/Creator/AssertionCreator.php' |
awk '{printf "%d:%s\n", NR, $0}' | sed -n '1,300p'Repository: LycheeOrg/Lychee
Length of output: 8500
🏁 Script executed:
#!/bin/bash
set -eu
for path in \
'src/Assertion/Creator/Pipes/MayRetrieveCredentialsIdForUser.php' \
'src/JsonTransport.php'
do
printf '%s\n' "--- $path ---"
curl -fsSL "https://raw.githubusercontent.com/Laragear/WebAuthn/719ba978a18c95118e1ac5bba810522d0768ebb8/$path" |
awk '{printf "%d:%s\n", NR, $0}' | sed -n '1,260p'
doneRepository: LycheeOrg/Lychee
Length of output: 4335
Information Disclosure (CWE-203)
Reachability: External · Exploitability: Trivial
Make fake credential lists indistinguishable.
toVerify() returns stable persisted credential IDs for existing users and omits allowCredentials when none exist. This path returns random IDs and always includes one to three credentials, so repeated unauthenticated requests reveal account existence. Derive a stable keyed fake set and match the real response shape. Add regression coverage for repeated known-user and unknown-user responses.
| // Special Webauthn operations | ||
| Route::post('/WebAuthn::register/options', [WebAuthn\WebAuthnRegisterController::class, 'options']) | ||
| ->name('webauthn.register.options'); | ||
| ->name('webauthn.register.options')->middleware(['throttle:10,1,login']); // Limit to 10 requests per minute per IP to prevent abuse |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Security Misconfiguration (CWE-770): Allocation of Resources Without Limits or Throttling
Reachability: External · Exploitability: Moderate
Use the named limiter syntax.
throttle:10,1,login does not select a named login limiter in Laravel 12. With three arguments, login is only a key prefix. The fixed limiter still keys authenticated requests by user identity before falling back to IP, so the route does not enforce the per-IP limit claimed by the comment. (raw.githubusercontent.com)
Change this to throttle:login and register RateLimiter::for('login', ...) with Limit::perMinute(10)->by($request->ip()). Confirm that the limiter exists outside the supplied RouteServiceProvider context before switching.
Suggested fix
- ->name('webauthn.register.options')->middleware(['throttle:10,1,login']);
+ ->name('webauthn.register.options')->middleware(['throttle:login']);RateLimiter::for('login', function (Request $request) {
return Limit::perMinute(10)->by($request->ip());
});Verify the route and limiter definitions:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
"webauthn\.register\.options|throttle:login|RateLimiter::for\(['\"]login|Limit::perMinute" \
routes app tests
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit