mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix(server): redact the credentialValues envelope in HTTP logs (#13742)
> **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":"<TOKEN>"}, ... }
```
**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 <arthur.dent@paperclip.local>
This commit is contained in:
1 parent
1c165e9f1e
commit
131c0c4609
4 files changed
+73
No files matched your search
@@ -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"');
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown>;
|
||||
|
||||
// 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(
|
||||
|
||||
@@ -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;
|
||||
@@ -27,6 +27,12 @@ const SENSITIVE_KEYS = new Set<string>([
|
||||
"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",
|
||||
|
||||
Reference in new issue
Block a user