mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 16:51:28 +02:00
fix(runner): preserve credential content in document saves (#14937)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The Runner sends authorized tool calls to the control plane. > - Agents use these calls to save plans and instruction files. > - The Runner used diagnostic secret detection to reject execution arguments. > - Ordinary credential-related prose could reject a document save before persistence. > - This pull request forwards the original arguments and leaves credential policy to the provider harness. > - The benefit is reliable saves with useful diagnostic records. ## Linked Issues or Issue Description Related foundation: Refs #12415 and #14430. No duplicate save-policy fix was found. **What happened?** A `write_document` call failed before the server saved its plan. The Runner reported `semantic tool input contains credential material; refusing to execute altered arguments`. The detector also masked ordinary phrases such as `secret manager` and `credential handling` in diagnostics. Both TypeScript dispatchers had equivalent execution gates. One dispatcher also rewrote structured approval and question payloads before execution. **Expected behavior** Paperclip forwards authorized arguments unchanged. The provider harness decides credential-content policy. Log and audit redaction does not reject or rewrite save input. **Steps to reproduce** 1. Send an authorized `write_document` call with a plan that discusses credential handling. 2. Include an intentional credential value in the body to exercise harness-owned policy. 3. The old Runner rejects the call. With this change, the document service stores the exact body. 4. Diagnostic records still mask explicit credential values. Qualified credential fields, short bearer values, opaque diagnostic pairs, and valid encoded JSON token headers have regression coverage. **Paperclip version or commit** Reproduced at `c46e41e81c03cd3c8b64cf993615b604d7fe8c62`. The branch is based on current `master`. **Deployment mode** Server deployment with the native Paperclip Runner. Local regression tests use the real document service and an embedded test database. ## What Changed - Remove credential-content vetoes from Rust admission and both TypeScript semantic dispatchers. - Preserve original structured approval and question arguments during execution. - Keep transport bounds, schema checks, authorization, idempotency, and audit masking. - Require explicit credential syntax or recognized formats for diagnostic masking. Preserve ordinary prose, metadata, and dotted identifiers. - Test exact document persistence, replay, nested argument identities, and masked audit copies. - Remove obsolete retry guidance and document harness-owned credential policy. ## Verification - `cargo test --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked -p paperclip-runner-core --lib --test acpx_event_payload --test acpx_provider_state --test acpx_provider_turns`: 355 tests passed. - Focused server and adapter tests: 189 tests passed after rebase. These include the real document save and the complete tool-gateway suite. - Semantic dispatcher and conformance tests: 34 tests passed. - Diagnostic redaction and MCP tests: 46 tests passed, including all six review examples. - `pnpm -r typecheck` and `pnpm build` passed on the repaired branch. - The broad local root suite was interrupted after database fixture setup failures. The focused database suites passed. CI runs the complete configured test lanes. ## Risks - Authorized tool arguments can intentionally contain credentials. The harness must enforce its content policy. - Diagnostic detection is narrower. Explicit assignments, credential fields, and recognized credential formats remain masked. - The change does not add a database migration or change company authorization. ## Model Used - OpenAI GPT-6 through Codex. The session exposes the GPT-6 model family; its exact runtime model identifier and context window size are not exposed. Used reasoning, tool use, and code execution. ## 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip authored and GitHub committed
1 parent
839cac1343
commit
9786f6df56
24 files changed
+444
-703
No files matched your search
@@ -12,9 +12,9 @@ describe("buildPaperclipTaskMarkdown", () => {
|
||||
const issue = { id: "task-id", identifier: "PAP-1", title: "Please investigate", description: "Please investigate sign-in failures", titleNeedsGeneration: true };
|
||||
const markdown = buildPaperclipTaskMarkdown({ issue });
|
||||
expect(markdown).toContain("As one of your first tool calls");
|
||||
expect(markdown).toContain("Check the title tool result");
|
||||
expect(markdown).toContain("retry once with a shorter, plain-language title");
|
||||
expect(markdown).toContain("new idempotency key for changed arguments");
|
||||
expect(markdown).toContain("Check the title tool result before claiming the title was saved");
|
||||
expect(markdown).not.toContain("rejected as credential material");
|
||||
expect(markdown).toContain("new idempotency key if retrying with changed arguments");
|
||||
expect(markdown).toContain("set_task_title");
|
||||
expect(markdown).toContain("onlyIfProvisional: true");
|
||||
expect(buildPaperclipTaskMarkdown({ issue: { ...issue, titleNeedsGeneration: false } })).not.toContain("Task title directive");
|
||||
|
||||
@@ -1,5 +1,18 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
describe("explicit diagnostic credential forms", () => {
|
||||
it("masks qualified credential fields while preserving metadata", () => {
|
||||
expect(redactEventPayload({ authorizationHeader: "abcdefghijkl", apiKeyProduction: "sensitivevalue", credentialHandling: "harness", tokenPolicy: "least privilege" }))
|
||||
.toEqual({ authorizationHeader: "***REDACTED***", apiKeyProduction: "***REDACTED***", credentialHandling: "harness", tokenPolicy: "least privilege" });
|
||||
});
|
||||
it("masks standalone bearer values and whitespace-prefixed JSON token headers", () => {
|
||||
const jwt = `${Buffer.from(' {"alg":"HS256","typ":"JWT"}').toString("base64url")}.abcdefghijk.abcdefghijkl`;
|
||||
expect(redactSensitiveText("provider said Bearer abcdefghijkl")).not.toContain("abcdefghijkl");
|
||||
expect(redactEventPayload({ diagnostic: jwt })?.diagnostic).toBe("***REDACTED***");
|
||||
expect(redactSensitiveText("Use bearer authentication and bearer tokens.")).toBe("Use bearer authentication and bearer tokens.");
|
||||
});
|
||||
});
|
||||
import {
|
||||
PRP_V1_EVENT_TYPES,
|
||||
PRP_V2_EVENT_TYPES,
|
||||
@@ -11,6 +24,22 @@ import {
|
||||
} from "../redaction.js";
|
||||
|
||||
describe("redaction", () => {
|
||||
it("preserves credential-related prose, metadata, and dotted filenames", () => {
|
||||
const input = {
|
||||
body: "Keep the private key in a secret manager. Document credential handling and token permissions. Use bearer tokens for authentication. Prefer bearer authentication.",
|
||||
tokenPolicy: "least privilege",
|
||||
secretStorage: "vault",
|
||||
credentialHandling: "harness",
|
||||
authorizationRequired: true,
|
||||
path: "deployment.credentials.example.md",
|
||||
};
|
||||
expect(sanitizeRecord(input)).toEqual(input);
|
||||
expect(redactSensitiveText(input.body)).toBe(input.body);
|
||||
expect(sanitizeRecord({ credentials: { provider: "opaque-value" }, passwordValue: "opaque-value", authorization_code: "opaque-value" }))
|
||||
.toEqual({ credentials: REDACTED_EVENT_VALUE, passwordValue: REDACTED_EVENT_VALUE, authorization_code: REDACTED_EVENT_VALUE });
|
||||
expect(redactSensitiveText("--token-budget 4000 SECRET_STORAGE=vault")).toBe("--token-budget 4000 SECRET_STORAGE=vault");
|
||||
});
|
||||
|
||||
it("keeps the discriminator allowlist in exact PRP v1 schema parity", () => {
|
||||
const schema = JSON.parse(
|
||||
readFileSync(
|
||||
@@ -86,7 +115,7 @@ describe("redaction", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("redacts unknown or mismatched PRP discriminators", () => {
|
||||
it("preserves diagnostic PRP discriminators while redacting their credential payloads", () => {
|
||||
const unknown = redactEventPayload({
|
||||
prpEvent: {
|
||||
schema: "paperclip.prp.event.v2",
|
||||
@@ -95,7 +124,7 @@ describe("redaction", () => {
|
||||
},
|
||||
});
|
||||
expect((unknown?.prpEvent as Record<string, unknown>).eventType).toBe(
|
||||
REDACTED_EVENT_VALUE,
|
||||
"session.not-a-real.event",
|
||||
);
|
||||
|
||||
const mismatched = redactEventPayload({
|
||||
@@ -106,7 +135,7 @@ describe("redaction", () => {
|
||||
},
|
||||
});
|
||||
expect((mismatched?.prpEvent as Record<string, unknown>).eventType).toBe(
|
||||
REDACTED_EVENT_VALUE,
|
||||
"session.capabilities.updated",
|
||||
);
|
||||
|
||||
const secretPayload = redactEventPayload({
|
||||
@@ -189,7 +218,7 @@ describe("redaction", () => {
|
||||
const result = sanitizeRecord(input);
|
||||
|
||||
expect(result.session).toBe(REDACTED_EVENT_VALUE);
|
||||
expect(result.opaque).toBe(REDACTED_EVENT_VALUE);
|
||||
expect(result.opaque).toBe("aaa.bbb.ccc");
|
||||
expect(result.normal).toBe("plain");
|
||||
});
|
||||
|
||||
@@ -208,7 +237,7 @@ describe("redaction", () => {
|
||||
nested: {
|
||||
schema: "paperclip.question_response.v1",
|
||||
runtimeSchema: "paperclip.runtime_request.v2",
|
||||
arbitraryProviderValue: REDACTED_EVENT_VALUE,
|
||||
arbitraryProviderValue: "paperclip.question_set.v1",
|
||||
},
|
||||
});
|
||||
});
|
||||
@@ -225,7 +254,7 @@ describe("redaction", () => {
|
||||
schema: input.schema,
|
||||
nested: {
|
||||
runtimeSchema: input.nested.runtimeSchema,
|
||||
providerValue: REDACTED_EVENT_VALUE,
|
||||
providerValue: input.nested.providerValue,
|
||||
},
|
||||
});
|
||||
expect(sanitizeRecord({ schema: "paperclip.native-execution-input.v4", runtimeSchema: "paperclip.native-model-envelope.v2" })).toEqual({
|
||||
@@ -258,28 +287,28 @@ describe("redaction", () => {
|
||||
schemaVersion: 1,
|
||||
eventType: "tool.execution.started",
|
||||
payload: {
|
||||
eventType: REDACTED_EVENT_VALUE,
|
||||
eventType: "run.result.accepted",
|
||||
credential: REDACTED_EVENT_VALUE,
|
||||
},
|
||||
},
|
||||
unrelated: {
|
||||
eventType: REDACTED_EVENT_VALUE,
|
||||
eventType: "workspace.file.referenced",
|
||||
},
|
||||
});
|
||||
expect(redactEventPayload(sanitized)).toEqual(sanitized);
|
||||
});
|
||||
|
||||
it("redacts unknown dotted event values even in a PRP-shaped envelope", () => {
|
||||
it("preserves unknown dotted event values instead of treating them as JWTs", () => {
|
||||
expect(
|
||||
redactEventPayload({
|
||||
schema: "paperclip.prp.event.v1",
|
||||
schemaVersion: 1,
|
||||
eventType: "attacker.supplied.token",
|
||||
})?.eventType,
|
||||
).toBe(REDACTED_EVENT_VALUE);
|
||||
).toBe("attacker.supplied.token");
|
||||
});
|
||||
|
||||
it("does not trust discriminators inside a forged unknown schema", () => {
|
||||
it("preserves dotted schema identifiers without interpreting their authority", () => {
|
||||
expect(
|
||||
redactEventPayload({
|
||||
schema: "paperclip.attacker.control.v1",
|
||||
@@ -288,14 +317,14 @@ describe("redaction", () => {
|
||||
eventType: "tool.execution.started",
|
||||
}),
|
||||
).toEqual({
|
||||
schema: REDACTED_EVENT_VALUE,
|
||||
runtimeSchema: REDACTED_EVENT_VALUE,
|
||||
schema: "paperclip.attacker.control.v1",
|
||||
runtimeSchema: "paperclip.attacker.runtime.v1",
|
||||
schemaVersion: 1,
|
||||
eventType: REDACTED_EVENT_VALUE,
|
||||
eventType: "tool.execution.started",
|
||||
});
|
||||
});
|
||||
|
||||
it("preserves native run span identities without weakening hostname redaction", () => {
|
||||
it("preserves native run span identities and ordinary hostnames", () => {
|
||||
const spanNames = [
|
||||
"environment.workspace.realize",
|
||||
"native.coordinator.claim",
|
||||
@@ -326,7 +355,7 @@ describe("redaction", () => {
|
||||
schema: input.schema,
|
||||
span,
|
||||
parentSpan: "native.session.execute",
|
||||
providerHostname: REDACTED_EVENT_VALUE,
|
||||
providerHostname: "api.openai.com",
|
||||
});
|
||||
expect(redactEventPayload(sanitized)).toEqual(sanitized);
|
||||
}
|
||||
@@ -336,13 +365,13 @@ describe("redaction", () => {
|
||||
schema: "paperclip.run-performance-span.v1",
|
||||
span: "api.openai.com",
|
||||
})?.span,
|
||||
).toBe(REDACTED_EVENT_VALUE);
|
||||
).toBe("api.openai.com");
|
||||
expect(
|
||||
redactEventPayload({
|
||||
schema: "paperclip.run-performance-span.v1",
|
||||
span: "runner.example.com",
|
||||
})?.span,
|
||||
).toBe(REDACTED_EVENT_VALUE);
|
||||
).toBe("runner.example.com");
|
||||
});
|
||||
|
||||
it("redacts payload objects while preserving null", () => {
|
||||
@@ -374,7 +403,7 @@ describe("redaction", () => {
|
||||
details: [
|
||||
"safe context",
|
||||
`proxyAuthorization ${REDACTED_EVENT_VALUE}`,
|
||||
REDACTED_EVENT_VALUE,
|
||||
"aaa.bbb.ccc",
|
||||
[`Bearer ${REDACTED_EVENT_VALUE}`],
|
||||
],
|
||||
},
|
||||
|
||||
@@ -2,11 +2,11 @@ import { describe, expect, it } from "vitest";
|
||||
import { redactEventPayload, redactSensitiveText } from "../redaction.js";
|
||||
|
||||
describe("public Executor selectors and secret redaction", () => {
|
||||
it("keeps only exact known public helper addresses readable in MCP results", () => {
|
||||
it("preserves dotted helper addresses without treating them as credentials", () => {
|
||||
const path = "executor.coreTools.integrations.list";
|
||||
expect(redactEventPayload({ path, text: `Call tools.${path}({})` })).toEqual({ path, text: `Call tools.${path}({})` });
|
||||
expect(redactSensitiveText(`${path}.privateSecretSuffix`)).toContain("REDACTED");
|
||||
expect(redactEventPayload({ path: "arbitrary.provider.path" })?.path).toContain("REDACTED");
|
||||
expect(redactSensitiveText(`${path}.privateSecretSuffix`)).toBe(`${path}.privateSecretSuffix`);
|
||||
expect(redactEventPayload({ path: "arbitrary.provider.path" })?.path).toBe("arbitrary.provider.path");
|
||||
});
|
||||
it("still redacts secret fields, bearer values, and JWT-shaped strings", () => {
|
||||
const value = "executor.coreTools.integrations.list";
|
||||
|
||||
@@ -2836,7 +2836,7 @@ rl.on("line", (line) => {
|
||||
.where(eq(activityLog.action, "tool_gateway.call_completed"));
|
||||
expect(activity.details).toMatchObject({
|
||||
headerSummary: {
|
||||
credentialHeaderNames: "***REDACTED***",
|
||||
credentialHeaderNames: ["authorization"],
|
||||
passthroughHeaderNames: ["x-client-request-id"],
|
||||
droppedPassthroughHeaderNames: expect.arrayContaining([
|
||||
"authorization",
|
||||
@@ -2930,7 +2930,7 @@ rl.on("line", (line) => {
|
||||
.where(eq(activityLog.action, "tool_gateway.call_completed"));
|
||||
expect(activity.details).toMatchObject({
|
||||
headerSummary: {
|
||||
credentialHeaderNames: "***REDACTED***",
|
||||
credentialHeaderNames: [],
|
||||
passthroughHeaderNames: ["x-client-request-id"],
|
||||
droppedPassthroughHeaderNames: expect.arrayContaining([
|
||||
"authorization",
|
||||
|
||||
Reference in new issue
Block a user