Conversation
|
One concern on the user-based filtering for GET /api/v1/sessions — we already have GET /api/v1/admin/sessions in AdminResource with user filtering support, and BatchesResource doesn't enforce user filtering either, so this would be inconsistent. Also without authentication enabled, anyone can claim any identity, so the filtering provides no real security. |
On auth, I agree that if auth isn’t on, you can’t really trust “current user,” so filtering isn’t a strong security fix by itself. The important part for everyone is redacting sensitive config in responses. When auth is on, listing only that user’s sessions still matches what we’d expect. On consistency with admin/sessions and batches, I treated GET /api/v1/sessions as the normal client API (not admin), so limiting the list to the logged-in user should be fine there. I get that other endpoints don’t all behave the same yet, if you prefer not changing session listing, I’m fine dropping it. We can figure out filtering for sessions/batches in a follow-up once there’s a clean plan. |
|
can we make the returned configs content configurable? e.g., |
Why are the changes needed?
Sensitive settings in session APIs
Session APIs were returning full session configs as-is. Some keys (passwords, tokens, etc.) should not be shown to clients or in traces. This patch applies the same redaction pattern the server already uses elsewhere (SERVER_SECRET_REDACTION_PATTERN + Utils.redact) so list/detail responses don’t expose those values in plain text.
Over-broad session listing
GET /api/v1/sessions returned every session on the server. With auth turned on, it should only return sessions for the logged-in user, not everyone else’s
How was this patch tested?
Added a REST test that opens a session with a sensitive Spark key, calls GET /api/v1/sessions, and checks the value is redacted.
Updated the session control CLI test so the session is opened with the same user the test client uses for auth, so the “list sessions” output matches real usage.
Verified tests run locally
Was this patch authored or co-authored using generative AI tooling?
No