fix(health): keep credentials out of the response sample we persist - #1596
Merged
RhysSullivan merged 4 commits intoAug 28, 2026
Merged
Conversation
A health check stores its response sample in connection.last_health, and the operation being probed is picked by the user from the plugin's catalog. Point it at a key-listing endpoint, or a /me that returns an api_key, and those values land in the database. The field right next to it already knew about this: `detail` is scrubbed of every credential value, with a comment saying upstream text can echo the request back. `responseSample` was left out of that scrub. Two directions, because they catch different things. Leaves whose KEY names a credential are redacted in the walker — that covers secrets we have never seen, which is exactly what a key-listing endpoint returns and what no scrub of a known value could find. The connection's own credential value is then scrubbed from what remains, covering a secret under an innocent key. The row is kept and its value replaced, so the preview still shows the shape.
This was referenced Aug 15, 2026
Scrub before truncating. The sample cut values to 120 chars inside the walk and the OpenAPI check scrubbed the connection's credential out afterwards, so a credential longer than the cap survived as a prefix the exact-value scrub could no longer match. The scrub now runs inside the walk, ahead of the cut. Match camelCase key names. accessToken, refreshToken, clientSecret, privateKey and sessionId carry no separator before the credential word, so the pattern read them as innocent. Both spellings are tested, since apiKey only matches as one contiguous word. author and authorName stay visible. Read an enclosing array container. A key listing returns api_keys.0.value, whose nearest named segment is the innocent value. A secret-named segment that directly contains an array now covers what is under it. names.0 is untouched. Redact the identity too. It is read straight off the raw body, so it passed through neither pass, and identityField is user-chosen from whatever the picker listed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A health check's response sample is written to
connection.last_health, so whatever it carries is persisted. The operation being probed is chosen by the user from the plugin's catalog — so it can just as easily be a key-listing endpoint as a/me.extractResponseFieldswalks every scalar leaf of the body with no filtering at all. A/methat returns anapi_key, or aGET /keys, puts that value straight into the database.The field immediately next to it already knew about this problem.
detailis scrubbed of every credential value, and the comment there says why — upstream text can echo the request back. Error bodies are excluded from sampling for the same reason. So the surface was thought about;responseSamplewas the one left out.The admin plane also already handles it, narrowing its read to
{status, checkedAt}with a comment naming this exact leak. This brings the stored value in line with what the admin read assumed was safe.The fix
Two directions, because they catch genuinely different things:
By key. Leaves whose key names a credential (
token,secret,api_key,session, …) are redacted in the walker. This is the half that matters most: it covers secrets we have never seen. A key-listing endpoint returns different secrets than the one we authenticated with, and no scrub of a known value can possibly find those.By value. The connection's own credential value is then scrubbed from what remains, using the same helper
detailalready uses. That covers a credential sitting under an innocent-looking key.The row is kept and only its value replaced, so the preview still shows the response shape and the identity picker keeps working.
How I tested it
packages/core/sdk: 602 passed, 45 files.packages/plugins/openapi: 268 passed, 45 files./me, a nested key list, an array of{name, token}pairs — rather than synthetic shapes.email,login,id) still come through untouched, since blanking those would make the feature useless.authoris not redacted whileauthorizationis. An over-eager substring match is the obvious way to get this wrong, so it has its own test.authorboundary one; the positive control correctly stayed green.oxlinton all three changed files: 0 warnings, 0 errors.