fix(RHOAIENG-52949): scope session listing by X-Ambient-Project header#895
fix(RHOAIENG-52949): scope session listing by X-Ambient-Project header#895
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughCentralized project scoping: a new ApplyProjectScope helper reads project_id from query or X-Ambient-Project, validates it, and injects a project_id filter into list query args. Handlers (sessions, projectSettings) delegate project-scoping to this helper and propagate any validation errors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/ambient-api-server/plugins/sessions/handler.go (1)
201-210: 🛠️ Refactor suggestion | 🟠 MajorExtract project-scope resolution into a shared helper.
The validation and
listArgs.Searchcomposition here already exists incomponents/ambient-api-server/plugins/projectSettings/handler.go:91-107, but with different header behavior. This is the same policy drift that caused the current bug. Please centralize precedence, validation, and filter injection in one helper and reuse it across handlers.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ambient-api-server/plugins/sessions/handler.go` around lines 201 - 210, Create a shared helper (e.g., ResolveProjectScopeFilter) that takes the incoming projectID, the existing search string (or a pointer to ListArgs.Search), and a flag describing header-vs-param precedence, validates projectID using safeProjectIDPattern, and returns the composed search filter or a validation error; then replace the inline validation/assignment in handlers (references: projectID, safeProjectIDPattern, listArgs.Search) with calls to this helper so both sessions and projectSettings handlers reuse identical precedence, validation, and filter-injection logic while preserving each handler's header-behavior via the flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/ambient-api-server/plugins/sessions/handler.go`:
- Around line 201-210: Create a shared helper (e.g., ResolveProjectScopeFilter)
that takes the incoming projectID, the existing search string (or a pointer to
ListArgs.Search), and a flag describing header-vs-param precedence, validates
projectID using safeProjectIDPattern, and returns the composed search filter or
a validation error; then replace the inline validation/assignment in handlers
(references: projectID, safeProjectIDPattern, listArgs.Search) with calls to
this helper so both sessions and projectSettings handlers reuse identical
precedence, validation, and filter-injection logic while preserving each
handler's header-behavior via the flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2ec076f-c441-49ea-a4b3-2e7670724d6b
📒 Files selected for processing (1)
components/ambient-api-server/plugins/sessions/handler.go
The session and project-settings list endpoints returned all records regardless of the X-Ambient-Project header, breaking multi-tenant isolation. Both handlers already supported project_id as a query parameter but did not read the header. Extract shared ApplyProjectScope helper in plugins/common that reads the project from the query param (precedence) or X-Ambient-Project header, validates it, and injects the filter into ListArguments.Search. Both handlers now use this shared helper. Jira: RHOAIENG-52949 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
848995e to
b04e4b7
Compare
19 test cases covering: - Header-only and query-param-only filtering - Query param precedence over header - No project returns no filter (backward compatible) - Combines with existing search expressions - Rejects SQL injection payloads via both header and query param - Accepts valid project ID patterns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary
The session list endpoint (
GET /api/ambient/v1/sessions) returned all sessions from the database regardless of theX-Ambient-Projectheader, breaking multi-tenant isolation.Root Cause
The handler already supported
project_idas a query parameter filter, but the SDK and CLI send the project via theX-Ambient-Projectheader — which was not read by the list handler.Fix
Read
X-Ambient-Projectheader as fallback when the?project_idquery param is not set. The same validatedproject_id = 'X'filter is applied to scope results. Query param takes precedence over header if both are set.Test plan
acpctl get sessionswith project set toAshould only return sessions from projectAGET /sessions?project_id=Astill works (query param precedence)GET /sessionswithout header or param returns all sessions (backward compatible)Jira: RHOAIENG-52949
🤖 Generated with Claude Code