Skip to content

fix(secrets): stop JSON-encoding secret values#162

Merged
v1r3n merged 3 commits into
mainfrom
fix/secrets-shouldnt-force-json
Jul 20, 2026
Merged

fix(secrets): stop JSON-encoding secret values#162
v1r3n merged 3 commits into
mainfrom
fix/secrets-shouldnt-force-json

Conversation

@chrishagglund-ship-it

@chrishagglund-ship-it chrishagglund-ship-it commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Fix for #152 : stop JSON-encoding secret values in PutSecret/GetSecret

Summary

PutSecret was running the secret value (an already-plain string) through JSON serialization, which wrapped it in quotes before sending it to the server (secret_value -> "secret_value"). The server stores the body verbatim, so the extra quotes became part of the stored secret. GetSecret then JSON-decoded on read, which stripped the quotes again and made the SDK's own get-after-put round trip look correct while the value was actually corrupted at rest.

This PR sends the raw string on write and returns the raw response body on read, matching the Java/Go/Python/Rust SDKs.

Changes

  • PutSecretWithHttpInfo: send the string body as-is instead of Serialize(body).
  • GetSecret now returns string (was object).
  • GetSecretAsync now returns Task<string> (was Task<object>).
  • GetSecretWithHttpInfo now returns ApiResponse<string> with the raw response body in .Data (no JSON deserialization).
  • Updated the PutSecret_CanBeRetrieved integration test to assert an exact round-trip match (previously it only asserted non-null, so it could not catch this bug).

Why this is a bugfix, not a major version bump

Technically this changes public signatures (GetSecret: object -> string, GetSecretWithHttpInfo: ApiResponse<object> -> ApiResponse<string>), which is API-breaking in the strict semver sense. In practice:

  • string is assignable to object, so the vast majority of callers keep compiling.
  • Secrets are conceptually strings everywhere in the platform (all other SDKs type them as strings), so this is the correct type.
  • The prior behavior was simply broken.

Given that, a full major version release seems disproportionate. Proposing this ship as a patch/minor with a clear changelog note rather than a major bump.

Server-side follow-up required

This fixes the client going forward, but secrets written by the previous (buggy) client are still stored with the extra quotes and will not be corrected automatically. Any affected secrets need to be re-saved (re-PutSecret) after this change so the stored value no longer contains wrapping quotes. Consumers that read the raw stored value (e.g. auth headers, HTTP tasks using ${workflow.secrets.x}) are the ones impacted by the corrupted values.

Possibly affected in other SDKs (unconfirmed)

While comparing implementations, the JavaScript and Ruby SDKs appear to do the same JSON-encoding of the string body (JSON.stringify / JSON.generate respectively), so they may exhibit the same corruption. This has not been empirically confirmed against a live server, and both also strip quotes on read, which can mask it in their own round trip. Worth verifying separately (raw GET / curl) and fixing if reproduced.

Test plan

  • dotnet build succeeds.
  • dotnet test --filter "FullyQualifiedName~SecretTests.PutSecret_CanBeRetrieved" passes against a live server (exact round-trip match).

@chrishagglund-ship-it
chrishagglund-ship-it force-pushed the fix/secrets-shouldnt-force-json branch from 88b3d10 to e42e688 Compare July 20, 2026 17:26
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 3.31% <100.00%> (+0.81%) ⬆️

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

Files with missing lines Coverage Δ
Conductor/Api/SecretResourceApi.cs 19.39% <100.00%> (+19.39%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@v1r3n
v1r3n merged commit 1614395 into main Jul 20, 2026
7 checks passed
@v1r3n
v1r3n deleted the fix/secrets-shouldnt-force-json branch July 20, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants