fix: rate-limit known-device probe and TOTP QR#33
fix: rate-limit known-device probe and TOTP QR#33VanceHud wants to merge 1 commit intoshuaiplus:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances security and user experience by moving TOTP QR code generation to the client-side and adding rate limiting to prevent enumeration attacks on the known-device probe endpoint.
Changes:
- QR codes for TOTP setup are now generated in the browser using the qrcode-generator library from CDN, replacing the previous server-side API call to qrserver.com
- Added strict rate limiting (10 requests per minute per IP) to the unauthenticated GET /api/devices/knowndevice endpoint to prevent device enumeration attacks
- Refactored IP detection logic by splitting getClientIp from getClientIdentifier for better code reusability
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/setup/pageTemplate.ts | Adds client-side QR code generation with qrcode-generator library, displays otpauth URI for manual import, and implements fallback placeholder when script fails to load |
| src/services/ratelimit.ts | Adds consumeKnownDeviceProbeBudget method, extracts getClientIp function for IP detection, and adds configuration for known-device probe rate limit |
| src/router.ts | Implements rate limiting check before handling known-device requests, returns 429 with retry-after header when limit exceeded, and imports getClientIp function |
| src/config/limits.ts | Adds knownDeviceProbeRequestsPerMinute limit configuration set to 10 requests per minute |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
| </div> | ||
|
|
||
| <script src="https://cdn.jsdelivr.net/npm/qrcode-generator@1.4.4/qrcode.min.js" referrerpolicy="no-referrer"></script> |
There was a problem hiding this comment.
Loading external scripts from a CDN without Subresource Integrity (SRI) creates a security risk. If the CDN is compromised or serves malicious content, it could execute arbitrary code in users' browsers during the setup process. Consider either: (1) adding an integrity attribute with the SHA hash of the expected file, or (2) bundling the qrcode-generator library locally instead of loading it from a CDN.
| <script src="https://cdn.jsdelivr.net/npm/qrcode-generator@1.4.4/qrcode.min.js" referrerpolicy="no-referrer"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/qrcode-generator@1.4.4/qrcode.min.js" referrerpolicy="no-referrer" integrity="sha384-REPLACE_WITH_ACTUAL_HASH_FOR_QRCODE_GENERATOR_1_4_4" crossorigin="anonymous"></script> |
| if (!qr) return; | ||
|
|
||
| try { | ||
| if (typeof qrcode === 'function') { |
There was a problem hiding this comment.
The check 'typeof qrcode === "function"' may not be reliable across all scenarios. While qrcode-generator does expose a constructor function, a more defensive check would be 'typeof qrcode !== "undefined" && qrcode' to handle edge cases where the library loads but the global is malformed. Alternatively, check for the specific API: 'typeof qrcode === "function" && typeof qrcode(0, "M").addData === "function"' to ensure the expected API is available.
| if (typeof qrcode === 'function') { | |
| if (typeof qrcode !== 'undefined' && qrcode) { |
| qr.src = qrCode.createDataURL(4, 0); | ||
| return; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
The empty catch block silently swallows all errors that occur during QR code generation. While the code does fall back to the placeholder, logging the error would help with debugging issues in production. Consider adding a console.error or console.warn statement to log the exception.
| } catch { | |
| } catch (err) { | |
| console.error('Failed to render TOTP QR code from URI.', err); |
| </div> | ||
| </div> | ||
|
|
||
| <script src="https://cdn.jsdelivr.net/npm/qrcode-generator@1.4.4/qrcode.min.js" referrerpolicy="no-referrer"></script> |
There was a problem hiding this comment.
Loading the QR code library from cdn.jsdelivr.net may fail in regions where jsdelivr is blocked (e.g., some corporate networks or countries with strict internet policies). When the CDN is blocked, the code falls back to a placeholder, but users won't be able to scan the QR code. Consider: (1) documenting this limitation in user-facing messaging, or (2) bundling the library locally to ensure it always loads.
前端生成二维码:src/setup/pageTemplate.ts
在浏览器端用 qrcode-generator 直接把 otpauth://... 生成二维码,不再走API;并保留本地占位图兜底(脚本异常时)。
为 GET /api/devices/knowndevice 添加了限流 每ip每分钟10次 避免被枚举