Skip to content

fix(manager): redact secrets from job responses - #4828

Open
immanuwell wants to merge 1 commit into
dragonflyoss:mainfrom
immanuwell:fix/job-response-secret-redaction
Open

fix(manager): redact secrets from job responses#4828
immanuwell wants to merge 1 commit into
dragonflyoss:mainfrom
immanuwell:fix/job-response-secret-redaction

Conversation

@immanuwell

Copy link
Copy Markdown
Contributor

Description

Preheat job responses leak creds back in args. Kinda rough.
This strips password, headers, object storage secrets, and hdfs delegation tokens from create, get, list, and update job responses.

Related Issue

N/A. Same leak pattern is already visible in public reports like #3226 and #3928.

Motivation and Context

Repro:

  1. POST /oapi/v1/jobs with a preheat job that includes private registry creds, request headers, object storage creds, or an hdfs delegation token.
  2. GET /oapi/v1/jobs/{id} or GET /oapi/v1/jobs.
  3. Before this patch the response echoes those secrets in args.

The job still runs with the original data. This only stops sending secrets back to API clients.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)

Checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.17%. Comparing base (c9e2e6d) to head (2340693).
⚠️ Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
manager/handlers/job_response.go 88.88% 2 Missing and 2 partials ⚠️
manager/handlers/job.go 85.71% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4828      +/-   ##
==========================================
+ Coverage   28.08%   28.17%   +0.09%     
==========================================
  Files         232      233       +1     
  Lines       23153    23189      +36     
==========================================
+ Hits         6502     6534      +32     
- Misses      16211    16213       +2     
- Partials      440      442       +2     
Flag Coverage Δ
unittests 28.17% <88.37%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
manager/handlers/job.go 45.37% <85.71%> (ø)
manager/handlers/job_response.go 88.88% <88.88%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: immanuwell <pchpr.00@list.ru>
@immanuwell
immanuwell force-pushed the fix/job-response-secret-redaction branch from 10e62dc to 2340693 Compare June 17, 2026 15:16
@yxxhero yxxhero added the enhancement New feature or request label Jun 17, 2026
@pmady

pmady commented Jun 20, 2026

Copy link
Copy Markdown

looked through the diff — clean separation putting the sanitization in its own file. couple things i noticed:

username is not in jobArgsSecretKeys so it passes through to the response. is that intentional? for preheat jobs the username is usually a registry robot account like robot$dragonfly, so maybe its fine, but worth a comment in the code so the next person doesnt wonder about it.

also the sanitize functions strip the keys entirely rather than replacing with "[REDACTED]" or similar. that means a caller cant tell the difference between "this field was removed for security" vs "this field was never set". not a blocker, just something to think about — redacting to a sentinel might be friendlier for debugging.

tests look solid, good coverage of the nested object_storage and hdfs cases.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants