From 131c0c4609b218c6bcb6e07170e47e586b33775d Mon Sep 17 00:00:00 2001 From: Stefan Date: Thu, 5 Nov 2026 21:39:15 +0100 Subject: [PATCH] fix(server): redact the credentialValues envelope in HTTP logs (#13742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > **Attribution:** this pull request was written and opened by the Paperclip agent **Arthur Dent** (Platform & Reliability Engineer, Tharsia) from an automated run under the host GitHub credential. GitHub records the author as `stefanriegel`; Stefan Riegel did not write it. No other edit was made to this description. ## Thinking Path > - Paperclip is the open source app that people use to manage AI agents for work. > - The server writes request details to the user journal. A middleware module names the fields that are secret. > - The connect endpoint accepts a `credentialValues` object. The object maps a provider config path to a secret string. > - One provider config path is `credentials.authorization`. The caller chooses the key names. Therefore the key is one flat string, and it is not a nested path. > - The redaction path list did not cover the `credentialValues` envelope. The recursive redactor compares keys by exact match, so `credentials` and `authorization` in the key set did not match the flat key `credentials.authorization`. > - A failed connect request therefore wrote a GitHub token to the journal in cleartext. > - This pull request redacts the `credentialValues` envelope in both layers. > - The benefit is that no provider secret reaches the journal, for any key name that a caller chooses. ## Linked Issues or Issue Description No public issue exists. The report follows the bug report template. **What happened?** A `POST /api/companies/{companyId}/tools/apps/connect` request returned HTTP 502. The server logs request bodies for non-2xx responses. The journal line contained a GitHub fine-grained personal access token in cleartext. The token was 93 characters long and started with `github_pat_`. The line contained no redaction marker. The logged body had this shape: ``` "reqBody":{"galleryKey":"github","connectionMethodKey":"mcp-key","name":"GitHub", "credentialValues":{"credentials.authorization":""}, ... } ``` **Expected behavior** The journal line shows a redaction marker in place of the token. No cleartext credential reaches the journal on any response status. **Steps to reproduce** 1. Run a Paperclip server on version 2026.916.0. 2. Send a `POST` request to `/api/companies/{companyId}/tools/apps/connect`. 3. Put a secret under `credentialValues` with the key `credentials.authorization`. 4. Make the request fail, so that the server returns a non-2xx status. 5. Read the journal line for the request. Result: the secret appears in cleartext. The two redaction layers both miss it. **Paperclip version or commit** 2026.916.0. Reproduced against `master` at `c65fc9e3c81c41aafe421aa90a00514b84343285`. The two middleware files are unchanged at `master` `0f5fafe16b069b8f0c7f17eb9080d010aa6cd513`, so the defect is present at the current head of `master` as well. **Deployment mode** Self-hosted. **Installation method** npm package. **Access context** Authenticated user, company-scoped route. **Relevant logs or output** Token value removed. The journal line has the shape above. ## What Changed - Add `reqBody.credentialValues` to `HTTP_LOG_REDACT_PATHS` in `server/src/middleware/http-log-redaction.ts`. This redacts the whole envelope at the pino serialization boundary. - Add `credentialvalues` to `SENSITIVE_KEYS` in `server/src/middleware/redact-sensitive.ts`. This redacts the envelope by name in the recursive pass. - Add a test that sends a `credentialValues` envelope through a real pino logger with the production path list. The test asserts that the secret is absent. - Add a test for the flat-dotted-key shape that leaked, and for the nested shape as a regression. - Add a test that the envelope is redacted below the top level. The key-set change is the durable half. The path list only covers body shapes that someone has already seen leak. The recursive pass matches the envelope by name for any shape. ## Verification Run these commands: ``` npx vitest run server/src/__tests__/http-log-redaction.test.ts server/src/__tests__/redact-sensitive.test.ts ``` Against the patched source, both files pass: **72 tests passed, 0 failed**. That is 54 tests in `http-log-redaction.test.ts` and 18 tests in `redact-sensitive.test.ts`. The same two files were then run against the **unpatched** source, which is the parent commit of this branch. **Four tests fail there.** Those four are exactly the tests this pull request adds: - `http-log-redaction.test.ts` — "defines the HTTP auth and cookie header paths that must be redacted". The path list does not contain `reqBody.credentialValues`. - `http-log-redaction.test.ts` — "redacts a credentialValues envelope at the serialization boundary". This is the leak: a real pino logger writes the token in cleartext. - `redact-sensitive.test.ts` — "redacts the connector credentialValues envelope on an app-connect body". - `redact-sensitive.test.ts` — "redacts a credentialValues envelope nested below the top level". The four failures show that each half of the change is load-bearing, and that both layers failed, not one. The control run also confirms that the 68 tests which pass on the unpatched source still pass on the patched source, so the change regresses nothing that already held. The tests assert that the secret string is absent, and they do not pin a redaction marker. This is deliberate: the two layers write different markers. `fast-redact` writes `[Redacted]`. `redactSensitive` writes `[REDACTED]`. An assertion that requires `[REDACTED]` alone fails on a correct path-list-only fix. ## Risks Low risk. The change adds two redaction paths. It removes no existing path. One behavioral shift is intentional. The recursive pass now replaces the whole `credentialValues` subtree with one marker, instead of replacing individual leaves. Any non-secret sibling field placed inside `credentialValues` is also redacted. This is correct, because the envelope holds only provider secrets by definition. One adjacent pull request, `#10784`, adds header paths to the same array in `http-log-redaction.ts`. The two changes touch different regions of the array. The second one to merge needs a trivial rebase at most. ## Model Used - Provider and model: Anthropic Claude, via the Paperclip `claude_local` agent adapter - Capability details: tool use and code execution in a sandboxed workspace - The patch was authored and verified by automated agent runs ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [ ] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge Left unchecked on purpose: the CI and Greptile items cannot be true before the pull request exists. "I have run tests locally and they pass" stays unchecked as a whole-workspace statement. The two test files named under Verification were run locally, against both the patched and the unpatched source, and the numbers above are from those runs. The full workspace suite has not run. ## Related pull requests Refs `#10784`, `#6373`, `#8841`. Co-authored-by: Arthur Dent --- .../src/__tests__/http-log-redaction.test.ts | 34 +++++++++++++++++++ server/src/__tests__/redact-sensitive.test.ts | 27 +++++++++++++++ server/src/middleware/http-log-redaction.ts | 6 ++++ server/src/middleware/redact-sensitive.ts | 6 ++++ 4 files changed, 73 insertions(+) diff --git a/server/src/__tests__/http-log-redaction.test.ts b/server/src/__tests__/http-log-redaction.test.ts index bee75c9128d..15a738ee89b 100644 --- a/server/src/__tests__/http-log-redaction.test.ts +++ b/server/src/__tests__/http-log-redaction.test.ts @@ -424,6 +424,7 @@ describe("HTTP logger redaction", () => { 'req.headers["x-telegram-bot-api-secret-token"]', ); expect(HTTP_LOG_REDACT_PATHS).toContain("reqBody.credentials"); + expect(HTTP_LOG_REDACT_PATHS).toContain("reqBody.credentialValues"); expect(HTTP_LOG_REDACT_PATHS).toContain("errorContext.details.credentials"); }); @@ -878,4 +879,37 @@ describe("HTTP logger redaction", () => { }); }, ); + + it("redacts a credentialValues envelope at the serialization boundary", async () => { + const chunks: string[] = []; + const stream = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(chunk.toString()); + callback(); + }, + }); + const logger = pino({ redact: [...HTTP_LOG_REDACT_PATHS] }, stream); + + // No recursive redactor runs here on purpose: this exercises the path list + // as the backstop it is documented to be. fast-redact's censor is + // "[Redacted]" (pino's default), not redactSensitive's "[REDACTED]", so + // assert the secret is gone rather than pinning the marker. + logger.error( + { + errorContext: { name: "Error", message: "OAuth authorization expired." }, + reqBody: { + galleryKey: "github", + name: "GitHub", + credentialValues: { "credentials.authorization": "github_pat_canary" }, + }, + reqParams: {}, + }, + "POST /api/companies/company-1/tools/apps/connect 502 — OAuth authorization expired.", + ); + await new Promise((resolve) => setImmediate(resolve)); + + const output = chunks.join(""); + expect(output).not.toContain("github_pat_canary"); + expect(output).toContain('"galleryKey":"github"'); + }); }); diff --git a/server/src/__tests__/redact-sensitive.test.ts b/server/src/__tests__/redact-sensitive.test.ts index e3cb39280b5..633d4adb9bd 100644 --- a/server/src/__tests__/redact-sensitive.test.ts +++ b/server/src/__tests__/redact-sensitive.test.ts @@ -229,6 +229,33 @@ describe("redactSensitive", () => { }); describe("stripSecretBearingUrlParts", () => { + it("redacts the connector credentialValues envelope on an app-connect body", () => { + const out = redactSensitive({ + galleryKey: "github", + name: "GitHub", + credentialValues: { + "credentials.authorization": "authorization-canary", + "headers.X-Api-Key": "header-canary", + }, + }) as Record; + + // Diagnostics that make the failed connect debuggable must survive. + expect(out.galleryKey).toBe("github"); + expect(out.name).toBe("GitHub"); + expect(out.credentialValues).toBe("[REDACTED]"); + expect(JSON.stringify(out)).not.toContain("canary"); + }); + + it("redacts a credentialValues envelope nested below the top level", () => { + const out = redactSensitive({ + connection: { + credentialValues: { "credentials.authorization": "nested-canary" }, + }, + }); + + expect(JSON.stringify(out)).not.toContain("nested-canary"); + }); + it("keeps a request path legible while dropping its complete query and fragment", () => { expect( stripSecretBearingUrlParts( diff --git a/server/src/middleware/http-log-redaction.ts b/server/src/middleware/http-log-redaction.ts index 3817a40886e..88750f8d120 100644 --- a/server/src/middleware/http-log-redaction.ts +++ b/server/src/middleware/http-log-redaction.ts @@ -29,5 +29,11 @@ export const HTTP_LOG_REDACT_PATHS = [ // at the final serialization boundary in case a future custom serializer // bypasses the recursive redactor. "reqBody.credentials", + // The app-connect/reconnect body groups every provider credential under + // `credentialValues`: a map of provider config paths + // (`credentials.authorization`, `headers.X-Api-Key`) to the secret string for + // each. The key names are chosen by the caller, so no fixed list of leaf + // names can cover them. Redact the whole envelope instead. + "reqBody.credentialValues", "errorContext.details.credentials", ] as const; diff --git a/server/src/middleware/redact-sensitive.ts b/server/src/middleware/redact-sensitive.ts index 8817bac59b0..9401287ba27 100644 --- a/server/src/middleware/redact-sensitive.ts +++ b/server/src/middleware/redact-sensitive.ts @@ -27,6 +27,12 @@ const SENSITIVE_KEYS = new Set([ "configuration_token", "credential", "credentials", + // The app-connect/reconnect envelope. Its keys are caller-chosen provider + // config paths (`credentials.authorization`, `headers.X-Api-Key`), so they + // never match a leaf name in this set; redact the envelope by name. This is + // the durable half: the path list in http-log-redaction.ts only ever covers + // shapes someone has already seen leak. + "credentialvalues", "password", "currentpassword", "newpassword",