Skip to content

Improve session REST security #7391

Open
Yutong-1424 wants to merge 1 commit intoapache:masterfrom
Yutong-1424:Feature/yutong/redact_credential
Open

Improve session REST security #7391
Yutong-1424 wants to merge 1 commit intoapache:masterfrom
Yutong-1424:Feature/yutong/redact_credential

Conversation

@Yutong-1424
Copy link
Copy Markdown

Why are the changes needed?

  1. 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.

  2. 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

@wangzhigang1999
Copy link
Copy Markdown
Contributor

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.

@Yutong-1424
Copy link
Copy Markdown
Author

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.

@pan3793
Copy link
Copy Markdown
Member

pan3793 commented Apr 10, 2026

can we make the returned configs content configurable? e.g., ORIGINAL, REDACTED, NONE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants