feat(auth): add passkey authentication#2849
Conversation
- enforce one-time verified WebAuthn ceremonies on the existing user model - support multiple named credentials with usage metadata, rename, and revocation - integrate local matching frontend builds and cover security invariants with focused tests Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3539fbf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I have a few concerns about scope, deployment documentation, and the existing WebAuthn settings. First, could the FRONTEND_DIST_DIR change be moved to a separate PR? This appears to be a general build feature rather than something required specifically for passkeys. It allows build.sh to replace the bundled frontend with the contents of a local directory, which could be useful for many unrelated backend/frontend changes. It also changes the trust boundary of the build process: when enabled, arbitrary files from the supplied directory may be included in the final backend artifact. Please document the intended use of this option and confirm that release workflows cannot set it from untrusted pull-request input, downloaded artifacts, or uncontrolled external paths. Keeping this change separate would make both the passkey implementation and the build/release security impact easier to review. Second, the PR describes several deployment constraints that should be documented before the feature is merged:
These details can directly affect registration and login behind reverse proxies or load balancers, especially when requests are routed between instances. Is there a corresponding documentation PR, or could the required documentation be added as part of this work? Finally, please clarify how this interacts with the existing WebAuthn login setting. In particular:
The PR mentions backward-compatible credential decoding, which addresses the storage format, but the user-visible behavior and migration expectations should also be documented. |
jyxjjj
added
enhancement
needs-triage/needs-confirm
PR: LOW-RISK
Module: User
- Evict the oldest challenge at capacity so public begin-login traffic cannot lock the store.\n- Cover capacity eviction with a focused regression test.\n- Remove the unrelated local frontend build hook from this contribution.\n\nCo-authored-by: Codex <267193182+codex@users.noreply.github.com>
|
Addressed in 02ab2b2 and the coordinated documentation PR OpenListTeam/OpenList-Docs#348:
Evidence: focused backend tests pass ( |
|
Final Oracle ARM64 verification for head
GitHub's |
|
I rechecked the current head and the resolved P1 thread. The change mitigates the original failure mode, but I do not think the unauthenticated challenge-store DoS has been fully addressed. At the 10,000-entry ceiling, every public begin-login request acquires the global lock, scans the entire challenge map, and evicts the oldest live challenge. Sustained unauthenticated traffic can therefore:
Evicting the oldest live challenge prevents the store from rejecting every new insertion, but it does not provide admission control and can replace the original lockout with challenge churn and a CPU-heavy hot path. Please add rate or admission controls that cannot be monopolized by anonymous traffic, avoid an O(n) scan on every insertion at capacity, and add a regression test that demonstrates a legitimate ceremony can still complete during sustained hostile begin-login traffic. Please also remove the feature-specific changes to the repository-level agent instructions. Those instructions are not part of the passkey implementation or operator documentation and make the PR scope harder to review. After these changes, this PR and the coordinated frontend PR should receive another security review before merge. |
|
After reviewing the updated scope, I suggest considering a split between the existing WebAuthn security hardening and the broader passkey product changes. The current PR contains two categories of work with different review and maintenance requirements:
Separating them could make the security-sensitive portion easier to audit and allow it to proceed independently of the product decision around the expanded passkey experience. It would also provide a clearer rollback boundary and keep the coordinated frontend and documentation changes attached to the feature portion. A possible split would be:
This is a design suggestion rather than a request to split at this stage. Before making further changes, please provide your own assessment of:
The recent responses across the coordinated PRs are highly uniform and read more like generated status reports than a design discussion. Under the repository's automated-contribution policy, another automated acknowledgement or implementation summary is not sufficient. Please confirm that you have personally reviewed the implementation and explain the trade-offs of splitting it in your own words. |
| If you are not one of the listed assistants, do not add a `Co-authored-by` trailer. | ||
|
|
||
| Instead, ask the human collaborator to provide the exact `Co-authored-by` trailer to use. Do not invent, infer, or generate one yourself. | ||
|
|
There was a problem hiding this comment.
The passkey is unrelated to the agent's instructions and should not be specifically mentioned.
Passkey 与 AGENT 指导无关，不应特别提示。
| oldestExpiry = existing.Expires | ||
| } | ||
| } | ||
| // ponytail: O(n) only at the 10k-entry ceiling; use an expiry heap if this becomes hot. |
There was a problem hiding this comment.
Non-standard ponytail comments, and using suboptimal solutions cannot be justified by the comments themselves.
不标准的ponytail注释，且使用次优方案并不能用注释为自己辩护。
Summary / 摘要
Userauth owner; password, LDAP, SSO, recovery, and current session/token behavior remain available.authnfield with created/last-used metadata, rename, revocation, signature-counter updates, and backward-compatible decoding of existing credentials.last_used_atare durable.Configuration and topology notes:
Production passkeys require an absolute HTTPS
site_url; the RP ID is derived from that canonical host and the exact origin is enforced.Passkey login and management remain gated by the existing
webauthn_login_enabledsetting. Disabling it rejects new ceremonies without deleting stored credentials; re-enabling it restores access.Existing WebAuthn credentials remain valid through backward-compatible decoding. No database migration, credential re-registration, or administrator action is required.
No parallel account table or new database column is added. The existing user
authnJSON storage format gains metadata around each credential.The challenge store is intentionally in-memory for the documented single-process topology. Multi-replica deployments must provide a shared atomic challenge store before enabling passkeys.
This PR has breaking changes.
/ 此 PR 包含破坏性变更。
This PR changes public API, config, storage format, or migration behavior.
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
This PR requires corresponding changes in related repositories.
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Testing / 测试
go test ./...— upstreammaincurrently fails unrelated vet checks for non-constant format strings across existing drivers. With vet disabled, unrelated environment-sensitive tests still fail becauseinternal/netreceives the harness*net.safeTransport, and Aria2 RPC tests expect a service onlocalhost:6800.go test ./internal/authn ./internal/db ./server/handles— passed.git diff --check upstream/main...HEAD— passed.Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。