Repository navigation
feat(ai): enforce session ownership isolation - #1592
Conversation
a96b5e5 to
9ccbd17
Compare
9ccbd17 to
532bdc5
Compare
|
@larry-zy help review this PR. |
| query = query.Where("tenant_id = ?", access.Subject.TenantID) | ||
| if !access.Subject.HasScope(security.ScopeSessionAdmin) { | ||
| query = query.Where("owner_id = ?", access.Subject.UserID) |
There was a problem hiding this comment.
[P1] Use exact identity comparison for Store ownership checks
These predicates inherit the column collation, while AccessContext.CanAccessOwner uses exact Go string equality. With case-insensitive identity columns, the GORM-backed memory tool can therefore read another tenant's or user's history when the identity differs only in case: memory_all_by_session_id -> AllMemory -> findActiveSession -> scopeSessions has no policy recheck. I reproduced both tenant-only and owner-only variants through the real memory tool using SQLite NOCASE; the case-sensitive controls were denied. Please align Store ownership comparisons with CanAccessOwner. Direct HTTP reads and deletes remain blocked by Manager's secondary authorization; no MySQL instance was tested.
There was a problem hiding this comment.
Thanks for catching this. Fixed in 4c6bbad: GORM now rechecks tenant and owner identities using exact Go string comparisons after querying, including history access. List results are filtered using the same policy.
| if a.Subject.TenantID != tenantID { | ||
| return false | ||
| } | ||
| return a.Subject.UserID == ownerID || a.Subject.HasScope(ScopeSessionAdmin) |
There was a problem hiding this comment.
[P2] Keep partially owned sessions isolated from business access
This admin branch grants access even when ownerID is empty, and GORM's admin query also omits the owner predicate. Without running legacy adoption, I reproduced HTTP GET, LIST, and DELETE returning 200 for same-tenant legacy sessions with owner_id = '' or NULL, while an ordinary user's GET returned 404. This contradicts the ADR's stated policy that partially owned rows remain isolated for explicit repair. Please reject incomplete ownership in both the policy and GORM queries before applying the same-tenant admin override.
There was a problem hiding this comment.
Fixed in 4c6bbad. Business access now rejects sessions with an empty tenant or owner, including access by same-tenant admins. Legacy adoption still only claims sessions where both fields are absent.
robocanic
left a comment
There was a problem hiding this comment.
审查意见
Session 所有权隔离的整体设计清晰:身份只来自服务端 Authenticator,Get/List/Touch/Delete/BeginTurn/... 都通过 AccessContext 做租户 / 属主过滤,越权与不存在统一返回同一类错误,Memory 与 GORM 共用契约测试。我在 PR head 上跑了 go test ./security/... ./store/... ./component/...(在 ai/ 下),均通过。以下是几点需要关注的问题:
1. “system” 能力没有被限制在 MaintenanceStore 内(安全加固,中等)
ai/security/security.go:98、:115:SystemAccessContext()是导出函数,CanAccessOwner对 system 直接返回true;ai/store/gorm/store.go:699-701的scopeSessions在access.IsSystem()时不加任何租户 / 属主过滤,Memory 的validateSessionLocked(ai/store/memory/store.go:532)也走同一个CanAccessOwner。- 因此业务接口
SessionStore/MessageStore(Get/List/Delete/BeginTurn/AllMemory/...)在传入 system context 时可以跨租户读写任意 Session 和历史;PR 描述里“业务接口只暴露业务操作,特权清理 / 迁移走MaintenanceStore”在接口类型上成立,但在运行时语义上并不成立。目前仅靠 HTTP 中间件authenticationMiddleware拒绝IsSystem(),而security.AccessContextFromContext(security.go:148)会原样接受放进context的 system context,进程内任意代码(例如某个 tool)只要调用WithAccessContext(ctx, SystemAccessContext())就能绕过隔离。 - 建议:业务方法(非
DeleteExpired/AdoptLegacySessions)在access.IsSystem()时返回ErrResourceNotAccessible,或让scopeSessions/CanAccessOwner不再对 system 放行;AccessContextFromContext也应拒绝 system。并在store/test/contract.go里补一个“system context 不能经由业务接口访问他人 Session”的用例。
2. 带方括号的 IPv6 host 会通过校验但启动失败(回归,低)
ai/component/server/component.go:141:isLoopbackHost特意strings.Trim(host, "[]"),说明host: "[::1]"被当作合法的回环地址;但:236改成了net.JoinHostPort(s.host, ...),JoinHostPort会对含:的 host 再加一层方括号,得到[[::1]]:8888,Listen报missing port in address。我在本地验证了:[::1]→[[::1]]:8888监听失败,而未加括号的::1可以。改动前的fmt.Sprintf("%s:%d")对[::1]是可用的,所以这是一个回归。- 建议:在
Validate/NewServerComponent里统一去掉方括号(或只接受不带括号的写法并在校验中报错),并补一个 IPv6 回环地址的测试。
3. adopt_legacy_sessions 的接线没有测试
ai/component/server/component.go:211-220:Start()中根据adopt_legacy_sessions调用AdoptLegacySessions的逻辑(含失败时中止启动)没有任何测试覆盖;store层有测试,但配置开关 → 调用 → 仅认领 tenant/owner 同时为空的行 这条链路没有端到端验证,而该开关会把历史数据全部归属到静态身份,建议至少补一个组件级测试(开关关闭时不认领、开启时认领)。
532bdc5 to
4c6bbad
Compare
|
|
@robocanic Regarding point 1 (system capability): addressed in 4c6bbad by adding Reference: #1592 (review) |
|
@robocanic Regarding point 2 (IPv6 host): fixed in 4c6bbad. The constructor now normalizes bracketed IPv6 hosts before passing them to Reference: #1592 (review) |
|
@robocanic Regarding point 3 (legacy adoption tests): added wiring tests in 4c6bbad covering enabled and disabled adoption, the configured tenant/owner, system capability, and error propagation. These complement the existing Store tests for adopting only fully unowned sessions. Reference: #1592 (review) |
AlexStocks
left a comment
There was a problem hiding this comment.
审查结论
方向正确,可以合并。 这是 ai 分支上 7 层堆叠 PR 的第 1 层(#1592 → #1594 → #1596 → #1608 → #1609 → #1610 → #1611),把「Session 只靠 ID 访问」改成显式 AccessContext,改动面覆盖 Memory/GORM 两个后端和 HTTP 边界,是本轮 AI Phase 1 里质量最高的一块。没有发现可导致越权读写的缺陷;下面列的三条都是可维护性与未来风险,不阻塞合并。
本次审查的基准
PR 作者声明本 PR 需先于 #1594/#1596 合并,属于严格堆叠。因此我按 merge-base(git merge-base origin/ai pr/1592)取证,而不是直接对 ai 读 diff:
- 相对
ai的累计 diff:+2015 / −440,39 个文件 - 相对上一层的增量:
+1895 / −439,39 个文件(本 PR 即链条底层,两者一致) - 与当前
ai(97d2db47,含#1567/#1598)做真实三方合并:git merge-tree --write-tree origin/ai pr/1592返回 0 冲突
已核实的实现细节
所有权判定是收敛到单点的。 ai/security/security.go:123 的 CanAccessOwner 是唯一判定入口,access.Validate() 失败、跨租户、tenantID/ownerID 为空一律返回 false,只有同租户且(UserID == ownerID 或持有 ai:sessions:admin)才放行。PolicyEnforcer / OwnershipPolicy 只是对它的包装,Store 侧又在 ai/store/memory/store.go:90,114,137,157,185,537 和 ai/store/gorm/store.go:110,152,690 逐点复核,属于「API 层判一次 + 存储层再判一次」的双层防御,不是单点信任。
身份来源不可伪造。 AccessContext.systemCapability 是 unexported 字段,外部包无法构造 AccessContext{} 通过 Validate();SystemAccessContext() 是唯一出口,且 ValidateBusiness() 会主动拒绝 system 上下文,保证系统能力只走 MaintenanceStore。ADR 0001 结尾也明确写了 MaintenanceStore 与私有标记「不是同进程内的能力边界」,这个自我限定是诚实的,不是掩饰。
错误码不回显存在性。 未授权与不存在都返回 ErrResourceNotAccessible,不会因为「404 vs 403」泄露 Session 是否存在。
静态身份的默认值有安全兜底。 默认只听 127.0.0.1、无跨域、tenant default、user local-user;非 loopback 监听必须显式打开 allow_insecure_static_auth。ADR 里也写清了「static 模式不得被描述为生产级用户隔离」。
建议(不阻塞)
P2 — SystemAccessContext() 是包级公开函数,等于给全模块发了万能钥匙。 ADR 已经承认这一点,但在 ai/ 内部的任何包都可以一行拿到 system 上下文,后续 PR(#1611 的摘要清理、#1596 的 retention)会不断新增维护路径,误用面只会变大。建议把 system 能力的铸造收进 bootstrap:只暴露 MaintenanceStore 接口 + 一个由启动期注入的私有 token,业务包即便拿到接口也拿不到 AccessContext。至少要加 grep 级别的 CI 检查,限制 SystemAccessContext() 的调用点白名单。
P3 — CanAccessOwner 在 IsSystem() 分支提前返回 true,跳过了空值检查。 ai/security/security.go:123-136 的顺序是「system → 直接 true」,所以 CanAccessOwner("", "") 对 system 上下文返回 true。这是 legacy adoption(只接管 tenant/owner 都为空的行)需要的语义,但当前没有注释说明这个顺序是刻意的,后面很容易被人「顺手修正」成先判空、直接打断 adoption。建议在该分支上加一行注释,说明空值放行是 adoption 的依赖。
P3 — Subject.UserID == ownerID 是裸字符串比较。 没有归一化(大小写、空白)。如果后续 JWT/SSO 适配器(#1600/#1609)对 sub 的来源不一致,同一个用户会出现两个身份。建议在 NewAccessContext 里对 TenantID/UserID 做一次 TrimSpace 并明确大小写敏感策略。
合并前置
|
LGTM |
|
Thanks for the thorough review. I agree with the remaining P2/P3 observations. For this PR, I will keep the current design. Stricter system-capability issuance and identity canonicalization can be addressed in follow-up authentication and maintenance hardening work. No further changes are planned for this PR. Thanks for confirming that there are no blocking issues. |



Summary
Implements the Session ownership and tenant/user isolation foundation for Issue #1539.
Subjectand explicitAccessContextScope
This PR implements the Session ownership portion of requirement 4 and establishes the authentication and policy interfaces used by later work.
Static authentication is intentionally limited to local development:
127.0.0.1allow_insecure_static_auth: trueLegacy Session adoption is disabled by default. When explicitly enabled, it adopts only rows where both tenant and owner are absent. Partially owned rows remain isolated for manual repair.
Out of scope
This PR does not claim to fully complete Issue requirement 4.
AuthorizedResourceScopeare not implemented;ResourceReferenceremains untrusted inputUntil production authentication and resource-level authorization are delivered, this change provides Session ownership isolation but must not be treated as complete production identity isolation.
Compatibility and migration
AccessContext, so callers inside the AI module were updated together.SessionStoreandMessageStoreexpose business operations only; privileged cleanup and adoption useMaintenanceStore.Validation
go test ./...go vet ./...go test -race ./security ./store/test ./store/memory ./store/gorm ./component/server/...git diff --checkRelated to #1539.