mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(server): redact invitation bearer paths from HTTP logs (#20343)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - People join a company through an invitation link. The link carries a bearer token in its URL path. > - The HTTP logger removes query secrets and URL user information. It keeps the path. > - So invitation bearer tokens reach the HTTP log in the request URL, the log message, and the referrer header. > - PR #17848 fixed this, but it did not merge. Since then, #13742 changed the same redactor file. > - This pull request carries the #17848 commits onto current master. The original author is kept on both commits. > - The benefit is that invitation tokens do not stay in HTTP logs. The chat eval for membership management can then accept a real invitation safely. ## Linked Issues or Issue Description Supersedes #17848 (same commits, rebased onto current master after #13742). **What happened?** The HTTP logger writes invitation bearer tokens to logs. A synthetic POST to `/api/invites/<token>/accept` puts the token in `req.url`, in the log message, and in the `Referer`/`Referrer` header record. **Expected behavior** Logs keep the invitation route, the method, and the HTTP status. Logs do not keep the bearer token. **Steps to reproduce** 1. Check out master. 2. Run `server/src/__tests__/logger-invitation-redaction.test.ts` from this PR. 3. The synthetic token appears in the request URL, the message, and the referrer. 12 tests fail on master. **Paperclip version or commit** Master at the merge base of this branch. **Deployment mode** Source server. The tests use a disposable local HTTP listener. They use no database and no provider. ## What Changed - Remove the bearer segment from `/api/invites/<token>/...`, `/invites/<token>/...`, and `/invite/<token>` URLs, including mounted paths and absolute URLs. - Apply the same URL policy to `referer` and `referrer` request headers. - Keep record IDs on the non-token revoke route `/invites/<inviteId>/revoke` and on company-scoped invite routes. - Add regression tests through the real HTTP logger for 202, 400 and 500 responses, with both referrer header spellings. - Original work by @cryppadotta in #17848. This PR only rebases it onto master. ## Verification - From `server`: `node ../node_modules/vitest/vitest.mjs run --config vitest.config.ts src/__tests__/logger-invitation-redaction.test.ts src/__tests__/redact-sensitive.test.ts src/__tests__/http-log-policy.test.ts` — 46 passed. - With the master `logger.ts` and `redact-sensitive.ts`, the same new tests fail: 12 failed. - `http-log-aborted-response`, `cloud-principal-log` and `setup-token-session` tests pass locally. Some other HTTP-log suites could not load locally because workspace package builds were missing in this worktree. CI covers them. ## Risks - Low risk. Referrer query and fragment values are now removed from request logs, as they already are for request URLs. This removes some diagnostic detail to protect secrets. - A route that uses `/invites/<id>/...` for a non-bearer record ID, other than `revoke`, now shows `[REDACTED]` in logs for that segment. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution, used to rebase and verify. The original change was authored in #17848. ## 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: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
79bf733012
commit
a67b936885
4 files changed
+57
-2
No files matched your search
@@ -0,0 +1,30 @@
|
||||
import { Writable } from "node:stream";
|
||||
import express from "express";
|
||||
import pino from "pino";
|
||||
import request from "supertest";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { createHttpLogger } from "../middleware/logger.js";
|
||||
import { HTTP_LOG_REDACT_PATHS } from "../middleware/http-log-redaction.js";
|
||||
|
||||
describe("invitation bearer HTTP logs", () => {
|
||||
it.each([202, 400, 500].flatMap(status => ["Referer", "Referrer"].map(header => ({ status, header }))))("keeps bearer paths private in $status request records with $header", async ({ status, header }) => {
|
||||
const canary = "synthetic-invitation-bearer-canary";
|
||||
const chunks: string[] = [];
|
||||
const stream = new Writable({ write(chunk, _encoding, callback) { chunks.push(chunk.toString()); callback(); } });
|
||||
const app = express();
|
||||
app.use(createHttpLogger(pino({ redact: [...HTTP_LOG_REDACT_PATHS] }, stream)));
|
||||
const router = express.Router();
|
||||
router.post("/invites/:token/accept", (_req, res) => res.status(status).end());
|
||||
app.use("/api", router);
|
||||
const response = await request(app).post(`/api/invites/${canary}/accept`).set(header, `https://board.example/invite/${canary}?source=email`);
|
||||
expect(response.status).toBe(status);
|
||||
const output = chunks.join("");
|
||||
expect(output).not.toContain(canary);
|
||||
const record = JSON.parse(output.trim());
|
||||
expect(record.req.method).toBe("POST");
|
||||
expect(record.req.url).toBe("/api/invites/[REDACTED]/accept");
|
||||
expect(record.req.headers[header.toLowerCase()]).toBe("https://board.example/invite/[REDACTED]");
|
||||
expect(record.res.statusCode).toBe(status);
|
||||
expect(record.msg).toContain(`POST /invites/[REDACTED]/accept ${status}`);
|
||||
});
|
||||
});
|
||||
@@ -271,3 +271,19 @@ it("redacts Grok routine URL and key in setup failure logs", () => {
|
||||
expect(collectSensitiveStringValues(payload)).toContain(payload.webhookUrl);
|
||||
expect(collectSensitiveStringValues(payload)).toContain(payload.webhookKey);
|
||||
});
|
||||
|
||||
describe("invitation bearer URL paths", () => {
|
||||
it.each([
|
||||
["/api/invites/invite-bearer-canary/accept?token=query-canary", "/api/invites/[REDACTED]/accept"],
|
||||
["/invites/invite-bearer-canary/onboarding.txt", "/invites/[REDACTED]/onboarding.txt"],
|
||||
["/invite/invite-bearer-canary", "/invite/[REDACTED]"],
|
||||
["/API/INVITES/encoded%2Fbearer/skills/paperclip", "/API/INVITES/[REDACTED]/skills/paperclip"],
|
||||
["https://user:password@board.example/api/invites/invite-bearer-canary/accept#fragment", "https://board.example/api/invites/[REDACTED]/accept"],
|
||||
["/api/invites/invite-record-id/revoke", "/api/invites/invite-record-id/revoke"],
|
||||
["/invites/invite-record-id/revoke/", "/invites/invite-record-id/revoke/"],
|
||||
["/invite/invite-bearer-canary/revoke", "/invite/[REDACTED]/revoke"],
|
||||
["/api/companies/company/invites/invite-record-id", "/api/companies/company/invites/invite-record-id"],
|
||||
])("sanitizes %s without hiding the route", (input, expected) => {
|
||||
expect(stripSecretBearingUrlParts(input)).toBe(expected);
|
||||
});
|
||||
});
|
||||
@@ -119,8 +119,14 @@ export function createHttpLogger(baseLogger: Logger) {
|
||||
url: privateWebhookLogUrl(req.url),
|
||||
};
|
||||
}
|
||||
const headers = req.headers as Record<string, unknown> | undefined;
|
||||
return {
|
||||
...req,
|
||||
...(headers ? { headers: {
|
||||
...headers,
|
||||
...Object.fromEntries(["referer", "referrer"].filter(key => typeof headers[key] === "string")
|
||||
.map(key => [key, stripSecretBearingUrlParts(headers[key] as string)])),
|
||||
} } : {}),
|
||||
url:
|
||||
typeof req.url === "string"
|
||||
? stripSecretBearingUrlParts(req.url)
|
||||
|
||||
@@ -138,11 +138,14 @@ function isUrlishKey(key: string): boolean {
|
||||
export function stripSecretBearingUrlParts(value: string): string {
|
||||
const suffixStart = value.search(/[?#]/);
|
||||
const withoutQueryOrFragment =
|
||||
suffixStart === -1 ? value : value.slice(0, suffixStart);
|
||||
(suffixStart === -1 ? value : value.slice(0, suffixStart)).replace(
|
||||
/(^|^[a-z][a-z\d+.-]*:\/\/[^/]*)(\/(?:(?:api\/)?invites\/(?![^/]+\/revoke(?:\/|$))|invite\/))[^/]+/i,
|
||||
"$1$2[REDACTED]",
|
||||
);
|
||||
|
||||
try {
|
||||
const url = new URL(withoutQueryOrFragment);
|
||||
if (!url.username && !url.password && suffixStart === -1) return value;
|
||||
if (!url.username && !url.password) return withoutQueryOrFragment;
|
||||
url.username = "";
|
||||
url.password = "";
|
||||
return url.toString();
|
||||
|
||||
Reference in new issue
Block a user