mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(sentry): stop treating short host settings as credentials in run-failure redaction (#20013)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - When an agent run fails, Paperclip sends a redacted failure report to Sentry so the team can fix the cause > - Redaction treats the server's own environment values as possible secrets and removes them wherever they appear in the report > - Since #19461, every value under an environment name the allowlist does not know counts as a secret, even very short ones such as `vi`, `RUN`, `off` or `1` > - On a host with such settings, the report text is damaged: `service` becomes `ser***REDACTED***ce`, and a `1` anywhere can vanish. One server test failed on every such host, while CI (which has no such values) stayed green > - This pull request keeps the fail-closed rule for an agent's configured environment, and requires a plausible credential in the server's own environment > - The benefit is that Sentry reports stay readable on every host, and real credentials stay private ## Linked Issues or Issue Description Refs #19461 (introduced the catch-all), #14585 (added the failing test). **What happened?** `heartbeat-process-recovery.test.ts` › "redacts opaque environment-bound credentials from Sentry diagnostics: result" failed locally on `master`: `expected 'ser***REDACTED***ce' to be 'service'`. Instrumentation showed that the server's own `process.env` contributed these redaction values: `["false","67591","true","0x0","RUN","off","vi","64","1","0","8"]`. They came from a desktop agent session's environment. **Expected behavior** Short host settings are not credentials. Declared, secret-named and credential-shaped values stay private. An agent's configured environment keeps its fail-closed rule. **Steps to reproduce** Run that test on a machine whose environment contains `SOME_UNKNOWN_NAME=vi`. ## What Changed - `packages/adapter-utils/src/command-redaction.ts`: `collectDiagnosticEnvironmentSecrets` gets an option, `{ hostProcess: true }`. With it, an undeclared value under an unrecognized name is collected only in these cases: - it has at least 8 characters and is not a bare number, hex number or boolean-like word; or - its name has a credential segment: `TOKEN`, `SECRET`, `PASSWORD`, `BEARER` or `PRIVATE`, or a final `KEY` or `KEY_ID`. Without the option, behavior is unchanged. - `server/src/services/run-failure-report.ts`: the report collects the server's own `process.env` with `hostProcess: true`. The agent's configured environment (`heartbeat.ts`) and the provider environment (`terminal-session-failure.ts`) keep the default. ## Verification - `command-redaction.test.ts` (42 pass). The new test fails on `master` and passes here. It checks three things: the host's short values are not collected; a short value under `AWS_ACCESS_KEY_ID` and a long opaque value stay private; and an agent binding `CUSTOM_BINDING: "1"` stays private. - `run-failure-diagnostics.test.ts` (135 pass, unchanged). These tests pin the agent-environment fail-closed rule. - `heartbeat-process-recovery.test.ts` › "redacts opaque environment-bound credentials…": both modes pass here. The `result` mode failed on unchanged `master` on this host. - The adapter-utils and server typechecks pass. ## Risks - The server's own environment now leaves out a non-credential-named value that is shorter than 8 characters or is a bare number or boolean, for example `MY_PIN=1234`. Values under credential names, declared secrets and credential-shaped values are still redacted. - No Telemetry path changes; this is the Sentry run-failure report. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, with tool use and code execution in Claude Code. ## 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 - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] 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 - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
484170d13b
commit
46f32ed33f
5 files changed
+118
-9
No files matched your search
@@ -12,6 +12,7 @@ describe("explicit diagnostic credential forms", () => {
|
||||
import {
|
||||
REDACTED_COMMAND_TEXT_VALUE,
|
||||
collectDiagnosticEnvironmentSecrets,
|
||||
collectHostEnvironmentTokenSecrets,
|
||||
looksLikeCredentialJwt,
|
||||
redactDiagnosticText,
|
||||
redactCommandText,
|
||||
@@ -213,6 +214,29 @@ describe("diagnostic environment secret selection", () => {
|
||||
expect(new Set(selected)).toEqual(new Set([jwt, pem, key, "safe"]));
|
||||
});
|
||||
|
||||
// Host env of a desktop agent session: short ordinary values under names
|
||||
// the allowlist does not know. Treating "vi" as a credential redacted it
|
||||
// inside "service" in Sentry reports.
|
||||
it("does not treat short or bare-number host process values under unknown names as credentials", () => {
|
||||
expect(collectDiagnosticEnvironmentSecrets({ CHILD_EDITOR: "vi", RUN_PHASE: "RUN", FEATURE_STATE: "off",
|
||||
SESSION_PID: "67591", XPC_FLAGS: "0x0", ASK_TOOL: "true", BATCH_SIZE: "8", LONG_NUMBER: "123456789012",
|
||||
CLAUDE_CODE_SDK_HAS_HOST_AUTH_REFRESH: "1" }, [], { hostProcess: true })).toEqual([]);
|
||||
// Credential names keep short values private, and long opaque values stay private.
|
||||
expect(collectDiagnosticEnvironmentSecrets({ AWS_ACCESS_KEY_ID: "1", CUSTOM_BINDING: "opaque-host-secret" }, [],
|
||||
{ hostProcess: true }).sort()).toEqual(["1", "opaque-host-secret"]);
|
||||
// An agent's configured environment keeps the fail-closed rule.
|
||||
expect(collectDiagnosticEnvironmentSecrets({ CUSTOM_BINDING: "1" })).toEqual(["1"]);
|
||||
});
|
||||
|
||||
it("keeps short host values private as whole tokens", () => {
|
||||
const env = { CHILD_EDITOR: "vi", OPAQUE_SHORT: "k9Q", RUN_PHASE: "RUN", FEATURE_STATE: "off", LEVEL: "2",
|
||||
SESSION_PID: "67591", LONG_SECRET: "opaque-host-secret", AWS_ACCESS_KEY_ID: "1" };
|
||||
// Substring-private values are not repeated; only flag values (booleans, 0, 1) are exempt.
|
||||
expect(collectHostEnvironmentTokenSecrets(env)).toEqual(["67591", "k9Q", "RUN", "vi", "2"]);
|
||||
expect(collectHostEnvironmentTokenSecrets({ SHORT_PIN: "482", COUNT_FLAG: "1", OFF_FLAG: "0", MODE_FLAG: "yes" })).toEqual(["482"]);
|
||||
expect(collectHostEnvironmentTokenSecrets(null)).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps unknown opaque values private without declarations", () => {
|
||||
expect(collectDiagnosticEnvironmentSecrets({ CUSTOM_BINDING: "opaque-host-secret" })).toEqual(["opaque-host-secret"]);
|
||||
});
|
||||
|
||||
@@ -192,10 +192,58 @@ function isOrdinaryDiagnosticEnvSetting(key: string, value: string): boolean {
|
||||
(name === "PAPERCLIP_SENTRY_RUN_FAILURES" && /^(?:off|unexpected|all)$/.test(value));
|
||||
}
|
||||
|
||||
/** In the host process environment, an undeclared value under an
|
||||
* unrecognized name is redacted as a substring only if it could be a
|
||||
* credential: at least 8 characters, and not a bare number, hex number or
|
||||
* boolean-like word. Shorter values ("vi", "RUN") are redacted only as whole
|
||||
* tokens (`collectHostEnvironmentTokenSecrets`): as substrings they corrupted
|
||||
* unrelated report text ("service" became "ser***REDACTED***ce"). Declared,
|
||||
* secret-named and credential-shaped values are always substring-private. An
|
||||
* agent's configured environment keeps the fail-closed substring rule. */
|
||||
function plausibleUndeclaredCredential(key: string, value: string): boolean {
|
||||
if (credentialNameSegments(key)) return true;
|
||||
return value.length >= 8 && !/^(?:\d+|0x[0-9a-f]+|true|false|on|off|yes|no)$/i.test(value);
|
||||
}
|
||||
|
||||
const CREDENTIAL_SEGMENTS = new Set(["TOKEN", "TOKENS", "SECRET", "SECRETS", "PASSWORD", "PASSWD", "PASSPHRASE",
|
||||
"CREDENTIAL", "CREDENTIALS", "PRIVATE", "BEARER", "AUTHORIZATION", "APIKEY"]);
|
||||
|
||||
/** A name with a credential segment (AWS_SECRET_ACCESS_KEY, AWS_BEARER_TOKEN_BEDROCK),
|
||||
* or a KEY segment that is last or followed only by ID (KEY, AWS_ACCESS_KEY_ID).
|
||||
* Such a value stays private whatever its length. */
|
||||
function credentialNameSegments(key: string): boolean {
|
||||
const segments = key.toUpperCase().split(/[^A-Z0-9]+/).filter(Boolean);
|
||||
return segments.some((segment, index) => CREDENTIAL_SEGMENTS.has(segment) ||
|
||||
(segment === "KEY" && (index === segments.length - 1 || (index === segments.length - 2 && segments[index + 1] === "ID"))));
|
||||
}
|
||||
|
||||
const BOOLEAN_WORD = /^(?:true|false|on|off|yes|no)$/i;
|
||||
|
||||
/**
|
||||
* Host process values under unknown names that are too short or too plain to
|
||||
* redact as substrings (see `plausibleUndeclaredCredential`). They are still
|
||||
* private: callers redact them where they appear as a whole token, numbers
|
||||
* included. Only flag values are exempt (boolean words, and `0` or `1`): they
|
||||
* cannot hold a credential, and as tokens they would erase ordinary words
|
||||
* ("on", "no") and exit codes from every report.
|
||||
*/
|
||||
export function collectHostEnvironmentTokenSecrets(env: unknown): string[] {
|
||||
if (!env || typeof env !== "object") return [];
|
||||
const substringValues = new Set(collectDiagnosticEnvironmentSecrets(env, [], { hostProcess: true }));
|
||||
const values = new Set<string>();
|
||||
for (const [key, value] of Object.entries(env)) {
|
||||
if (typeof value !== "string" || !value.trim() || substringValues.has(value)) continue;
|
||||
if (isOrdinaryDiagnosticEnvSetting(key, value) || BOOLEAN_WORD.test(value) || /^[01]$/.test(value)) continue;
|
||||
values.add(value);
|
||||
}
|
||||
return [...values].sort((a, b) => b.length - a.length);
|
||||
}
|
||||
|
||||
/** Select private values while preserving explicit ordinary process settings. */
|
||||
export function collectDiagnosticEnvironmentSecrets(
|
||||
env: unknown,
|
||||
secretKeys: Iterable<string> = [],
|
||||
options: { hostProcess?: boolean } = {},
|
||||
): string[] {
|
||||
if (!env || typeof env !== "object") return [];
|
||||
const declared = new Set(secretKeys);
|
||||
@@ -206,7 +254,7 @@ export function collectDiagnosticEnvironmentSecrets(
|
||||
if (declared.has(key) || secretName.test(key) ||
|
||||
redactDiagnosticText(value) !== value || looksLikeCredentialJwt(value) ||
|
||||
/-----BEGIN (?:[A-Z]+ )*PRIVATE KEY-----/.test(value) ||
|
||||
!isOrdinaryDiagnosticEnvSetting(key, value)) {
|
||||
(!isOrdinaryDiagnosticEnvSetting(key, value) && (!options.hostProcess || plausibleUndeclaredCredential(key, value)))) {
|
||||
values.add(value);
|
||||
}
|
||||
try {
|
||||
|
||||
@@ -2,7 +2,7 @@ import { captureEnvironmentAcquisitionDiagnostic } from "../environment-acquisit
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { heartbeatRuns } from "@paperclipai/db";
|
||||
import { attachRemoteRunnerRecoveryDiagnostic, RemoteRunnerRecoveryError } from "../native-runtime/remote-runner-recovery-diagnostics.js";
|
||||
import { collectRunFailureDiagnostics, collectRunFailureSecretValues, redactRunFailureSecretValues, sanitizeRunFailureDiagnostics, sanitizeRunFailureText } from "../run-failure-diagnostics.js";
|
||||
import { collectRunFailureDiagnostics, collectRunFailureHostTokenSecrets, collectRunFailureSecretValues, redactRunFailureSecretValues, sanitizeRunFailureDiagnostics, sanitizeRunFailureText } from "../run-failure-diagnostics.js";
|
||||
|
||||
type Run = typeof heartbeatRuns.$inferSelect;
|
||||
const run = (overrides: Partial<Run> = {}) => ({ resultJson: null, ...overrides }) as Run;
|
||||
@@ -595,6 +595,20 @@ describe("run failure diagnostics", () => {
|
||||
expect(result.text).not.toContain("***[REDACTED]");
|
||||
});
|
||||
|
||||
it("redacts short host values as whole tokens without corrupting words that contain them", () => {
|
||||
const tokens = collectRunFailureHostTokenSecrets({ CHILD_EDITOR: "vi", OPAQUE_SHORT: "k9Q", SHORT_PIN: "482", LEVEL: "1" });
|
||||
const result = redactRunFailureSecretValues({ category: "service", text: "editor vi used k9Q and 482; k9Qx and 4821 stay, exit 1",
|
||||
nested: ["(vi)", "vim"] }, [], tokens);
|
||||
expect(result).toEqual({ category: "service",
|
||||
text: "editor ***REDACTED*** used ***REDACTED*** and ***REDACTED***; k9Qx and 4821 stay, exit 1",
|
||||
nested: ["(***REDACTED***)", "vim"] });
|
||||
});
|
||||
|
||||
it("lets a longer whole-token host value win over a shorter declared secret at the same position", () => {
|
||||
expect(redactRunFailureSecretValues("upstream rejected 1aB9xQ; code 1", ["1"], ["1aB9xQ"]))
|
||||
.toBe("upstream rejected ***REDACTED***; code ***REDACTED***");
|
||||
});
|
||||
|
||||
it("preserves ordinary settings while retaining declared secrets", () => {
|
||||
const env = { PAPERCLIP_DB_BACKUP_ENABLED: "false", PAPERCLIP_DB_BACKUP_RETENTION_DAYS: "1" };
|
||||
expect(collectRunFailureSecretValues(env)).toEqual([]);
|
||||
|
||||
@@ -5,7 +5,11 @@ import { readControllerLeaseLoss, readControllerLeaseLossCause, readRunCancellat
|
||||
import { readProcessLossDiagnostic } from "./process-loss-diagnostics.js";
|
||||
import { readReviewedChatBindingDenialReason } from "./reviewed-chat-binding-diagnostics.js";
|
||||
import { WORKSPACE_RESTORE_FAILURE_CODES } from "@paperclipai/shared";
|
||||
import { collectDiagnosticEnvironmentSecrets, redactDiagnosticText } from "@paperclipai/adapter-utils/command-redaction";
|
||||
import {
|
||||
collectDiagnosticEnvironmentSecrets,
|
||||
collectHostEnvironmentTokenSecrets,
|
||||
redactDiagnosticText,
|
||||
} from "@paperclipai/adapter-utils/command-redaction";
|
||||
import { directoryMergeLockTimeoutDiagnosticsFor } from "@paperclipai/adapter-utils/directory-merge-lock-diagnostics";
|
||||
import { sanitizeWorkspaceRestoreDiagnostic } from "@paperclipai/adapter-utils/workspace-restore-diagnostics";
|
||||
import {
|
||||
@@ -168,15 +172,31 @@ function scalars(value: unknown, fields: readonly string[]): Context {
|
||||
/** Select credential fields, credential-shaped values, and declared bindings. */
|
||||
export const collectRunFailureSecretValues = collectDiagnosticEnvironmentSecrets;
|
||||
|
||||
/** Short host values that stay private as whole tokens only. */
|
||||
export const collectRunFailureHostTokenSecrets = collectHostEnvironmentTokenSecrets;
|
||||
|
||||
/** Redact literal, JSON-escaped and URI-encoded forms without rewriting markers. */
|
||||
export function redactRunFailureSecretValues<T>(input: T, values: readonly string[]): T {
|
||||
const forms = [...new Set(values.filter(Boolean).flatMap((value) => {
|
||||
export function redactRunFailureSecretValues<T>(input: T, values: readonly string[],
|
||||
tokenValues: readonly string[] = []): T {
|
||||
const variants = (list: readonly string[]) => [...new Set(list.filter(Boolean).flatMap((value) => {
|
||||
const forms = [value, JSON.stringify(value).slice(1, -1)];
|
||||
try { forms.push(encodeURIComponent(value)); } catch { /* malformed Unicode */ }
|
||||
return forms;
|
||||
}))].sort((a, b) => b.length - a.length);
|
||||
if (forms.length === 0) return input;
|
||||
const pattern = new RegExp(forms.map((value) => value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")).join("|"), "gu");
|
||||
const escape = (value: string) => value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
const forms = variants(values);
|
||||
// Short host values are private only as whole tokens: "vi" is redacted in
|
||||
// "editor vi", never inside "service". One alternation ordered by length
|
||||
// across both lists, so a longer complete match always wins over a shorter
|
||||
// prefix that starts at the same position.
|
||||
const tokenForms = variants(tokenValues).filter((value) => !forms.includes(value));
|
||||
const alternatives = [
|
||||
...forms.map((value) => ({ value, pattern: escape(value) })),
|
||||
...tokenForms.map((value) => ({ value,
|
||||
pattern: `(?<![\\p{L}\\p{N}_])${escape(value)}(?![\\p{L}\\p{N}_])` })),
|
||||
].sort((a, b) => b.value.length - a.value.length);
|
||||
if (alternatives.length === 0) return input;
|
||||
const pattern = new RegExp(alternatives.map((entry) => entry.pattern).join("|"), "gu");
|
||||
const redact = (value: unknown): unknown => {
|
||||
if (typeof value === "string") return value.replace(pattern, () => REDACTED_EVENT_VALUE);
|
||||
if (Array.isArray(value)) return value.map(redact);
|
||||
|
||||
@@ -4,6 +4,7 @@ import { agents, dotRunnerAssignments, grokBotDispatches, heartbeatRuns, nativeR
|
||||
import { captureRunFailure, type RunFailureStatus } from "../sentry.js";
|
||||
import {
|
||||
collectRunFailureDiagnostics,
|
||||
collectRunFailureHostTokenSecrets,
|
||||
collectRunFailureSecretValues,
|
||||
redactRunFailureSecretValues,
|
||||
sanitizeRunFailureDiagnostics,
|
||||
@@ -376,9 +377,11 @@ async function captureTerminalRunFailure(
|
||||
errorCode: run.errorCode ?? null,
|
||||
diagnostics: collectRunFailureDiagnostics(run, options),
|
||||
}, [...new Set([
|
||||
...collectRunFailureSecretValues(process.env),
|
||||
// The server's own environment: short host settings under unknown
|
||||
// names are private only as whole tokens (see collectHostEnvironmentTokenSecrets).
|
||||
...collectRunFailureSecretValues(process.env, [], { hostProcess: true }),
|
||||
...(options.secretValues ?? []),
|
||||
])].sort((a, b) => b.length - a.length));
|
||||
])].sort((a, b) => b.length - a.length), collectRunFailureHostTokenSecrets(process.env));
|
||||
const agent = await db
|
||||
.select({ adapterType: agents.adapterType })
|
||||
.from(agents)
|
||||
|
||||
Reference in new issue
Block a user