fix(session): decode canonical detail route identities - #1388
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthrough客户端现在会解码并校验 canonical Session ID 路由参数。测试覆盖解码后的 ID 传入 Changes会话 ID 规范化
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 631dfc18f5
ℹ️ 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".
| const decoded = decodeURIComponent(sessionId); | ||
| return decoded.startsWith("pfx:") || decoded.startsWith("sid:") ? decoded : sessionId; |
There was a problem hiding this comment.
When a valid client-supplied physical ID contains percent text that decodes to a reserved prefix, such as pfx%3Afoo, buildPublicSessionIdentity deliberately preserves it because the raw ID does not begin with pfx: or sid:. Next.js already percent-decodes dynamic route parameters (also documented in src/app/api/ip-geo/[ip]/route.ts:36), so a correctly encoded link for that ID supplies pfx%3Afoo here; this second decode changes it to pfx:foo, causing detail, request-list, export, and termination operations to target a different identity. Preserve the framework-decoded parameter rather than decoding it again.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx (1)
275-294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win补充
sid:和普通物理 Session ID 的回归用例。当前用例只覆盖编码后的
pfx:路由值。它没有覆盖sid:分支，也没有验证普通物理 Session ID 在规范化后保持不变。建议将该用例参数化，至少加入这两种输入。这样可以覆盖新增的接受前缀和原始值回退路径。该建议基于本文件新增的前缀判断和回退逻辑。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx around lines 275 - 294, 参数化“decodes an URL-encoded canonical Session ID before loading details”测试，至少覆盖 URL 编码后的 sid: 值和普通物理 Session ID 输入；分别验证 sid: 前缀被正确规范化，以及普通物理 ID 规范化后保持不变，同时保留现有 pfx: 编码场景和预期的 getSessionDetailsMock 调用参数。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/app/`[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx:
- Around line 275-294: 参数化“decodes an URL-encoded canonical Session ID before
loading details”测试，至少覆盖 URL 编码后的 sid: 值和普通物理 Session ID 输入；分别验证 sid:
前缀被正确规范化，以及普通物理 ID 规范化后保持不变，同时保留现有 pfx: 编码场景和预期的 getSessionDetailsMock 调用参数。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3595ef1-0a0b-437b-938c-e196f075ffdb
📒 Files selected for processing (2)
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsxsrc/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function normalizeCanonicalSessionRouteParam(sessionId: string): string { | ||
| try { | ||
| const decoded = decodeURIComponent(sessionId); | ||
| return decoded.startsWith("pfx:") || decoded.startsWith("sid:") ? decoded : sessionId; |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Double-decoding the route param can rewrite a valid physical Session ID
Why this is a problem: useParams() already exposes the decoded dynamic segment here; the repo relies on the same rule in src/app/api/ip-geo/[ip]/route.ts:36 ("Next.js already percent-decodes route params, so no manual decode."). Decoding a second time changes a raw physical ID like pfx%3Afoo into pfx:foo, which violates the identity contract in src/lib/request-identity.ts:45 where ordinary Session IDs are preserved unless they already start with pfx: or sid:. After that rewrite, detail loading, request lookup, exports, and termination all target the wrong identity.
Suggested fix:
const params = useParams<{ sessionId: string }>();
const sessionId = params.sessionId;Then update the regression test to mock the runtime value that Next actually provides ("pfx:...") and add a case proving a physical ID such as "pfx%3Afoo" stays unchanged.
There was a problem hiding this comment.
Code Review Summary
I found one high-confidence correctness issue in the new route-param normalization. The client now double-decodes sessionId, which can rewrite valid raw physical IDs into reserved canonical identities and send detail, request, export, and terminate flows to the wrong session.
PR Size: XS
- Lines changed: 39
- Files changed: 2
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx:59double-decodes a route param that Next.js already decodes, so a raw physical ID such aspfx%3Afoois rewritten topfx:fooand subsequent detail/request/export/terminate calls can target the wrong identity.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#1388from the actual checkout state (pull/1388/merge), not the unrelated latest open PR. - Applied the size label
size/XS. - Posted 1 inline review comment on
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx:59. - Submitted the required summary review on the PR.
- The validated finding is a high-confidence logic bug: the new helper double-decodes
sessionId, which can rewrite valid raw physical IDs likepfx%3Afoointo reserved canonical identities and send detail/request/export/terminate flows to the wrong session.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
pfx:或sid:开头的值，避免改变普通 physical Session IDpfx%3A...形态补充回归测试Root Cause
目标 Session 已成功写入
message_request和usage_ledger，但详情页收到 URL 编码后的pfx%3A...并原样传给 REST client。REST client 再次执行encodeURIComponent后请求%253A键，无法命中数据库中的pfx:...canonical identity，因此返回资源不存在。Validation
bun run lintbun run lint:fixbun run typecheckbun run buildbun run test(858files passed,8365tests passed)Greptile Summary
This PR normalizes URL-encoded reserved canonical session identities at the Session detail route boundary so the REST client does not double-encode them.
pfx:orsid:, preserving ordinary physical Session IDs.pfx%3A...route form.Confidence Score: 5/5
The PR appears safe to merge, with the canonical route normalization matching the identity grammar and API-client encoding contract.
Reserved canonical identities use fixed
pfx:orsid:prefixes and hexadecimal components, and the new normalization restores their canonical form before the REST client performs its required path-segment encoding without altering ordinary physical IDs.Important Files Changed
Sequence Diagram
Reviews (1): Last reviewed commit: "fix(session): decode canonical detail ro..." | Re-trigger Greptile
Context used: