Repository navigation
Conversation
AlexStocks
left a comment
AlexStocks
left a comment
There was a problem hiding this comment.
审查结论
技术实现可用，但存在一个必须先解决的跨 PR 冲突：本 PR 与 #1600 是两套互斥的认证实现，不能同时合并。 单看本 PR，HS256 校验的参数设置是正确的，用量/审计只记元数据不记内容，这部分质量不错。真正的问题在选型，不在代码。
本次审查的基准
- 相对
ai的累计 diff：+9138 / −887，107 个文件（含 #1592/#1594/#1596/#1608 四层） - 相对上一层的增量：
+1792 / −48，28 个文件 - 与当前
ai（97d2db47）三方合并：0 冲突
先说冲突（P1，合并前置）
同时打开的两个 PR 各自实现了 AI 服务的 HTTP 认证，配置结构互不兼容：
| #1609（本 PR） | #1600 | |
|---|---|---|
| 算法 | HS256，共享密钥 | RS256 + JWKS |
| 实现位置 | ai/security/jwt.go |
ai/component/server/engine/auth.go + jwks_client.go |
| YAML | authentication: {allow_insecure_static_auth, jwt_secret, jwt_issuer, jwt_audience} |
auth: {jwks_url, issuer, audience} |
| 签发方 | 外部 IdP，Admin 不参与 | Admin 自己签发 POST /api/v1/auth/token |
| 影响面 | 仅 ai/ |
ai/ + pkg/console/auth/* + ui-vue3 |
两者都要改 ai/component/server/server.yaml 和 ai/schema/json/server.schema.json。Git 层面它们不冲突（我实测 merge-tree 均为 0 冲突），但合并后会出现「两套认证配置块 + 两条身份提取路径」，运维无法判断以哪个为准。请维护者先定选型。从工程角度，#1600 的 Admin 自签发 + JWKS 更贴合「不引入外部 IdP」的现状，且有密钥轮换路径；#1609 的 HS256 静态密钥在多副本部署时需要分发共享密钥，运维成本更高。
已核实的实现细节（本 PR 自身）
JWT 校验参数是完整的。 ai/security/jwt.go:57-62：WithValidMethods([]string{HS256}) + WithIssuer + WithAudience + WithExpirationRequired()，四个要素齐全；:64-67 还额外手工比对 token.Method != jwt.SigningMethodHS256，在 ValidMethods 之后属于冗余但无害的纵深防御。
密钥长度有硬下限。 NewJWTAuthenticator 要求 len(cfg.Secret) >= 32，等于强制 256 bit，和 HS256 的密钥要求一致；secret 拷贝一份存进来，不会因为调用方复用 slice 而被改。
身份不从请求头推导。 只认 Authorization: Bearer，tenant_id/sub 全部来自已验签的 claims，Subject.AuthMethod = AuthMethodJWT。这与 #1592 的「identity 只来自可信 authenticator」是一致的。
审计只记元数据。 ai/component/hooks/usage_audit.go:63 的 AuditEvent 只包含 ID/TenantID/UserID/SessionID/InteractionID/CallID/ToolCallID/EventType/Status/ReasonCode/各版本号/CreatedAt，没有任何模型或工具内容字段 —— 从结构体上而非靠约定来保证不落密文，这点做得对。
缺失的用量不假装有。 usage.AmountSource = UsageAmountUnknown / CostStatus: UsageCostUnknown 在 provider 没返回 usage 时保持 unknown，而不是补 0，对账时不会被误导。
建议（不阻塞）
P2 — 审计在拿不到 AccessContext 时静默丢弃。 usage_audit.go:73-76：
access, err := security.AccessContextFromContext(ctx)
if err != nil || store == nil {
return ctx
}store == nil（未配置）跳过是合理的。但 err != nil（ctx 里没有 access）也走同一条静默返回，既不记事件也不打日志。审计/计费子系统的失败模式应该是「显式可见」而不是「安静地少一条记录」；一条丢失的用量记录在月结对账时几乎无法回溯。建议拆开两个分支：store == nil 直接返回，err != nil 至少 logger.Warn("usage audit skipped: no access context", ...)，或用 system subject 落一条 status=skipped 的记录。
P3 — 配置错误被映射成 ErrAuthenticationRequired。 jwt.go:37-39：len(cfg.Secret) < 32、Issuer 为空、Audience 为空，全部返回 ErrAuthenticationRequired。这三种都是部署期配置错误，返回 401 语义上等于「客户端没带 token」，会让排查方向完全跑偏（运维看成认证问题，实际是 YAML 写错）。建议单独定义 ErrAuthMisconfigured，或直接 fmt.Errorf 带上具体字段名，让启动期日志一眼能定位。
P3 — 无密钥轮换与 kid 支持。 单一静态 HS256 密钥意味着轮换必须「停服改配置重启」，期间无法新旧密钥并存。如果最终选定 HS256 路线，至少应支持 Secret + SecretPrevious 双密钥验签；如果选 RS256 路线（#1600），这条自动消解。
P3 — tenant_id 回退 tenant 无文档。 jwt.go:80-83 先取 tenant_id，为空再取 tenant。两者同时存在时 tenant_id 优先，这是个隐式约定，签发方一旦同时使用两个 claim 会造成租户归属歧义。建议在 JWTConfig 的注释里写明优先级，或干脆只接受一个 claim 名（配置化）。
P3 — UsageAuditOptions 会被就地改写。 usage_audit.go:56 的 *options = options.withVersions() 修改的是调用方传入的结构体。如果同一个 options 被多个 registration 共享，第一次调用就会把版本号写进去，后续行为依赖调用顺序。建议改为值传递或返回新实例。
合并前置
|
[P2] Preserve unknown usage when the provider omits the usage object Reviewed head: The presence check in if response, ok := output.(*ai.ModelResponse); ok && response != nil && response.Usage != nil {
endState.InputTokens = response.Usage.InputTokens
endState.OutputTokens = response.Usage.OutputTokens
endState.TotalTokens = response.Usage.TotalTokens
endState.UsagePresent = true
}With the currently pinned Genkit v1.2.0 OpenAI-compatible adapter, a successful completion without a I reproduced this at the reviewed head using a local HTTP provider fixture whose successful response contains Please preserve explicit usage-field availability at the provider/adapter boundary and propagate it to the accounting hook. Add an HTTP-level regression covering missing/null usage and genuinely reported zero usage; checking whether all token counts are zero would also conflate those cases. |
|
Updated in
Validation: AI module tests, focused race checks, Admin authentication tests, 19 related frontend tests, and the frontend build passed. Frontend type-check errors were reproduced on the pre-change #1609 code. Dependency: #1600 should merge before #1609. This branch currently includes #1600's commits; after #1600 merges, #1609 should sync with |
larry-zy
left a comment
larry-zy
left a comment
There was a problem hiding this comment.
Re-reviewed head 0f0a069846761effded4dc75878819aad98eed1a, including the integration of #1600 at 0f4a9c0b. The RS256/JWKS implementation is present and JWT identity propagation passes the focused checks. Two additional integration/availability issues are described inline: cancellation of one unverified request can block valid clients through the shared JWKS cooldown, and the actual Router protects the public health endpoint.
Dependency validation before merging
This head still lacks the latest prerequisite fixes in #1594 (fe1f7552) and #1596 (5e7d951c). The following are already reported upstream, so I am recording them as synchronization/validation requirements rather than duplicating their inline findings:
- Prepare A, fail A, prepare B with a live lease, then retry A: both Memory and GORM/SQLite accept A and permit writes to both active Turns. The same regression passes on the current #1596 head.
- Complete A, delete its Session, then retry A before asynchronous cleanup: GORM/SQLite returns the retained answer. The same regression is rejected on the current #1596 head. This reproduces a Store-contract violation; it is not a claim of a demonstrated HTTP authorization bypass.
- The ordinary JSON credential-key fix from #1594 is also absent from this head.
- The UI retains the collected context object directly. Mutating the page context between an initial failed request and its retry changes the retry payload from version 1 to version 2 under the same key; the same test preserves version 1 on current #1596.
Please synchronize the latest #1594/#1596/#1608 prerequisites and rerun integrated validation before this PR and its descendants merge. This is a statement about the currently tested branch; it does not imply that a correctly resolved merge in the documented order must reintroduce those fixes.
The existing 20 targeted frontend tests pass using a temporary test configuration that disables PostCSS processing to isolate the local Tailwind dependency mismatch. The additional context-mutation regression fails on this head. Database reproductions use SQLite; production MySQL/PostgreSQL behavior was not exercised. The PR description also still describes HS256 and excludes Admin/JWKS integration, so it should be updated to match this implementation.
| err := v.fetchKeys(ctx) | ||
| v.mu.Lock() | ||
| flight.err = err | ||
| v.nextRefresh = v.clock().Add(jwksRefreshInterval) |
There was a problem hiding this comment.
[P2] Keep request cancellation from starting a shared authentication cooldown
The refresh uses the initiating HTTP request context and then advances nextRefresh by 30 seconds for every error, including that request being canceled; the shared keys have already been cleared. After cache expiry, a parseable, unsigned JWT with an unknown kid can reach this path before authentication succeeds. If that client disconnects during the fetch, subsequent valid-token requests fail for the whole cooldown without trying the healthy JWKS endpoint. I reproduced this with a local endpoint that succeeds at startup, blocks only the canceled refresh, and is healthy thereafter: a previously accepted token is rejected and no third JWKS request occurs. Please give the shared refresh its own bounded lifetime, or allow a fresh caller to retry immediately after request-scoped cancellation, while preserving the expired-key rejection.
There was a problem hiding this comment.
Fixed in 8266dfd. The shared refresh now runs on a detached, bounded context (context.WithTimeout(context.WithoutCancel(ctx), 10s)), so an initiating request's cancellation no longer aborts the fetch or advances the 30s failure cooldown. Expired keys are still cleared and rejected when a genuine refresh fails.
I replaced the old cancellation test with TestJWKSRefreshSurvivesRequestCancellation, which follows your reproduction: the startup load succeeds, the initiator cancels while the refresh is blocked, and then a previously accepted token must succeed immediately. I verified the test fails against the previous implementation (valid token -> 401) and passes with the fix.
| func (v *JWTVerifier) Authenticate(r *http.Request) (security.AccessContext, error) { | ||
| claims, err := v.verify(r) | ||
| if err != nil { | ||
| return security.AccessContext{}, err | ||
| } |
There was a problem hiding this comment.
[P2] Apply the public-probe exception on the actual Router authentication path
The production Router installs authenticationMiddleware(r.authenticator) globally and invokes this Authenticate method; it never installs JWTVerifier.Middleware(). Consequently, the public /health exception in Middleware() is unused. ServerComponent.Start registers /health on the same engine, so enabling JWT makes a tokenless health probe return 401 and can make a healthy deployment fail its readiness/liveness checks. I reproduced the 401 using NewRouter with a real JWKS verifier and the health registration used by Start. Please exempt the explicit public probes in the middleware actually used by the Router and add an integration test through that Router; the standalone verifier-middleware tests do not cover this wiring.
There was a problem hiding this comment.
Fixed in 8266dfd. The public-probe exemption now lives in authenticationMiddleware, the middleware the production Router actually installs, using a shared isPublicProbe helper. JWTVerifier.Middleware() uses the same helper, so the two paths cannot drift.
Added TestRouterHealthProbesArePublic: it builds the real NewRouter with a JWKS verifier and registers /health, /healthz, and /readyz the same way ServerComponent.Start does. Tokenless probes return 200 while /api/v1/ai/sessions still returns 401.
The quoted-value branch of the credential assignment rule stopped at an escaped quote, leaving the remaining credential suffix visible to the model. Match escape pairs inside double- and single-quoted values so the whole value is redacted. Add regressions for SanitizeText and the modelInputView boundary covering escaped quotes and backslashes.
# Conflicts: # ai/store/memory/store_test.go
Run shared JWKS refreshes on a detached, bounded context so a client disconnecting cannot abort the fetch or start the failure cooldown for other callers. Apply the public-probe exemption in the authentication middleware actually used by the Router, and add regressions through the real Router and a canceled refresh initiator.
|
@Qiao-yq please merge the ai branch and resolve conflicts. |
…lifecycle # Conflicts: # ai/component/agent/react/react.go # ai/component/memory/component.go # ai/component/server/component.go # ai/component/server/config_test.go # ai/component/server/engine/handlers.go # ai/component/server/engine/models.go # ai/component/server/engine/router.go # ai/component/server/engine/session/session.go # ai/store/gorm/models.go # ai/store/gorm/store.go # ai/store/gorm/store_test.go # ai/store/memory/store.go # ai/store/memory/store_test.go # ai/store/store.go # ai/store/validation.go
…into fix/ai-1539-context-window
… feat/ai-1539-production-auth-usage-audit
|
Please provide a description of this PR:
Draft. This PR must merge after #1594, #1596, the prerequisite snapshot fix #1608, and the Admin token/JWKS work in #1600. Merge order: #1594 → #1596 → #1608, with #1600 merged before this PR. The branch currently includes the latest heads of #1594 (
8eaa867), #1596 (90e4127), #1608 (ac732c9), and #1600 (0f4a9c0).Add the Phase 1 PR4 production authentication and usage/audit foundations for #1539.
Full pricing, Source attribution, and Context Compiler work remain outside this PR. The Admin-issued token/JWKS integration from #1600 is included. Persistence remains best effort when storage fails or capacity is exhausted. This PR does not declare the Phase 1 G1 gate passed.
While the dependencies are open, GitHub's comparison against
aiincludes their changes. Review the PR4-only changes here.Validation from
ai/:go test ./... -timeout 120sgo test -race ./security ./component/hooks ./component/server/... ./component/agent/react ./component/memory/... ./store/... -timeout 180sgo vet ./...git diff --check fix/ai-1539-context-window..HEADCoverage includes JWT identity isolation, cancelled-call persistence, unknown usage, call correlation with Usage disabled, independent flags, version metadata, cleanup permissions, capacity concurrency, and existing-table migration. GORM tests use SQLite; production MySQL/PostgreSQL concurrency and migration acceptance remain to be performed.
Part of #1539. Blocked by #1594, #1596, #1600, and #1608.
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.