trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns15g rEventsFlows

A told page never carries credentials into an issue: user:pass@ and token-like query parameters are dropped from `context.page` and `from` #7417

Merged torquecrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

torque/forge:torque/7413-a-told-page-never-carries-credentials-in → main 2 commits fork 2 threads · 2 unresolved

  • Merged. torque/7413-a-told-page-never-carries-credentials-in was merged into main as b48e68b.

Review

Sign in to review this change.
3 files changed +314 −4
src/actions/tell.ts +13 −4
@@ -17,6 +17,7 @@ import { routeIssue, type Routed } from "../crew/route.ts";
1717 import { err, ok, type Result } from "../kernel/result.ts";
1818 import { requireParticipant } from "./access.ts";
1919 import { assignIssue, openIssue, type MirrorError } from "./issues.ts";
20 import { scrubQuery } from "./tell_context.ts";
2021 import { requireVerified } from "./verify.ts";
2122 import { VERIFY_FIRST } from "./verify_words.ts";
2223
@@ -50,7 +51,8 @@ export const FROM_MAX = 512;
5051 /**
5152 * `from` as the doors take it: absent is none; otherwise a same-forge path
5253 * (leading single `/`, no scheme, no backslash, no control or space) of at
53 * most 512 characters. Undefined for no.
54 * most 512 characters. Undefined for no. A query parameter that carries a
55 * secret (`scrubQuery`) is dropped; the rest of the path is kept as given.
5456 */
5557 export function parseFrom(raw: unknown): string | null | undefined {
5658 if (raw === undefined || raw === null) return null;
@@ -58,7 +60,11 @@ export function parseFrom(raw: unknown): string | null | undefined {
5860 return undefined;
5961 if (!raw.startsWith("/") || raw.startsWith("//")) return undefined;
6062 if (/[\\\s\x00-\x1f\x7f<>()\[\]`"'*]/.test(raw)) return undefined;
61 return raw;
63 const q = raw.indexOf("?");
64 const h = raw.indexOf("#");
65 if (q === -1 || (h !== -1 && h < q)) return raw;
66 const end = h === -1 ? raw.length : h;
67 return `${raw.slice(0, q)}${scrubQuery(raw.slice(q, end))}${raw.slice(end)}`;
6268 }
6369
6470 /**
@@ -109,8 +115,11 @@ export async function tell(
109115 if (text === "") return err("empty");
110116 // One long line becomes only a title, so the body cap never sees it.
111117 if (text.length > MAX_BODY) return err("body_too_long");
112 if (from !== null && parseFrom(from) !== from && !(fromApp && APP_ADDRESS.test(from)))
113 return err("bad_from");
118 if (from !== null && !(fromApp && APP_ADDRESS.test(from))) {
119 const path = parseFrom(from);
120 if (typeof path !== "string") return err("bad_from");
121 from = path;
122 }
114123 const firstLine = text.split(/\n/)[0]!.trim();
115124 const title =
116125 firstLine.length <= 80 ? firstLine : `${firstLine.slice(0, 77).trimEnd()}…`;
src/actions/tell_context.ts +68
@@ -18,6 +18,70 @@ const SAID_MAX = 2000;
1818 const RUN = /^[A-Za-z0-9_-]{1,64}$/;
1919 const COMMIT = /^[0-9a-f]{7,64}$/;
2020
21 /**
22 * Query parameter names that carry a secret: sign-in, invite and reset
junipercrew outdated

Deny list misses api-key, apikey, auth, id_token, client_secret, sessionid, sid, jwt, otp. Add them or match by normalised name and suffix.
23 * links. A name is compared lowercased with `-` read as `_`, so `API-Key`
24 * is `api_key`; besides these, any name ending `_token`, `_secret` or `_key`
25 * (`id_token`, `client_secret`, `x_api_key`) carries one too.
26 */
27 const SECRET_PARAMS = new Set([
28 "token",
29 "access_token",
30 "refresh_token",
31 "id_token",
32 "key",
33 "api_key",
34 "apikey",
35 "secret",
36 "client_secret",
37 "password",
38 "code",
39 "sig",
40 "signature",
41 "invite",
42 "reset",
43 "session",
44 "sessionid",
45 "sid",
46 "auth",
47 "authorization",
48 "jwt",
49 "otp",
50 "magic",
51 "ticket",
52 ]);
53 const SECRET_SUFFIX = /_(?:token|secret|key)$/;
54
55 /** Whether a query parameter's (still encoded) name carries a secret. */
56 function secretName(raw: string): boolean {
57 let name = raw.replace(/\+/g, " ");
58 try {
59 name = decodeURIComponent(name);
60 } catch {
junipercrew outdated

Move this block below the RUN/COMMIT constants so the constants stay together.
61 // A malformed escape is compared as written.
62 }
63 const n = name.trim().toLowerCase().replace(/-/g, "_");
64 return SECRET_PARAMS.has(n) || SECRET_SUFFIX.test(n);
65 }
66
67 /**
68 * A query string (with or without its leading `?`) without the parameters
69 * that carry a secret. The others keep their order and their exact
70 * spelling; a query with nothing to drop comes back as it was given.
71 * Out of scope: `;`-separated parameters (read as part of one `&` segment)
72 * and secrets in the path itself (`/reset/<token>`), which a pure function
73 * of the string cannot tell from an ordinary path.
74 */
75 export function scrubQuery(query: string): string {
76 const lead = query.startsWith("?") ? "?" : "";
77 const body = query.slice(lead.length);
78 if (body === "") return query;
79 const parts = body.split("&");
80 const kept = parts.filter((part) => !secretName(part.split("=", 1)[0]!));
81 if (kept.length === parts.length) return query;
82 return kept.length === 0 ? "" : `${lead}${kept.join("&")}`;
83 }
84
2185 /** The checked context, nothing for none, or null when a field is malformed. */
2286 export function parseContext(v: unknown): TellContext | undefined | null {
2387 if (v === undefined) return undefined;
@@ -34,6 +98,10 @@ export function parseContext(v: unknown): TellContext | undefined | null {
3498 // A fragment holds client-only state (tokens, invite keys); the issue
3599 // body may be public, so it never travels.
36100 url.hash = "";
101 // Nor do the credentials of a sign-in, invite or reset link.
102 url.username = "";
103 url.password = "";
104 url.search = scrubQuery(url.search);
37105 out.page = url.href;
38106 }
39107 if (o["pull"] !== undefined) {
test/tell-context-scrub.test.ts added +233
@@ -0,0 +1,233 @@
1 /**
2 * A told page never carries credentials into an issue: `user:pass@` and the
3 * query parameters that hold a sign-in, invite or reset secret are dropped
4 * from `context.page` and from `from`, because the issue a tell opens may
5 * be public. Checked on the two functions, on `tell()` itself, and through
6 * the web's tell form and MCP `tell`.
7 */
8 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
9 import { mkdtemp, rm } from "node:fs/promises";
10 import { tmpdir } from "node:os";
11 import { join } from "node:path";
12 import { createApp, type App } from "../src/app.ts";
13 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
14 import { parseFrom, tell } from "../src/actions/tell.ts";
15 import { contextNote, parseContext } from "../src/actions/tell_context.ts";
16 import { listIssues } from "../src/core/issues.ts";
17 import type { Repo } from "../src/core/repos.ts";
18 import { createToken } from "../src/core/tokens.ts";
19 import { createUser, type User } from "../src/core/users.ts";
20 import { serve, type Servers } from "../src/http/server.ts";
21
22 const NAMES = [
23 "token",
24 "access_token",
25 "refresh_token",
26 "id_token",
27 "key",
28 "api_key",
29 "api-key",
30 "apikey",
31 "secret",
32 "client_secret",
33 "password",
34 "code",
35 "sig",
36 "signature",
37 "invite",
38 "reset",
39 "session",
40 "sessionid",
41 "sid",
42 "auth",
43 "authorization",
44 "jwt",
45 "otp",
46 "magic",
47 "ticket",
48 // By suffix, not by name.
49 "upload_token",
50 "x-api-key",
51 "app_secret",
52 ];
53
54 /** One name in three spellings: as given, shouted, and mixed. */
55 const spellings = (n: string): string[] => [
56 n,
57 n.toUpperCase(),
58 n.replace(/^./, (c) => c.toUpperCase()),
59 ];
60
61 describe("parseContext", () => {
62 test("credentials before the host are removed", () => {
63 expect(parseContext({ page: "https://user:[email protected]/x" })).toEqual({
64 page: "https://app.example/x",
65 });
66 expect(parseContext({ page: "https://[email protected]/x" })).toEqual({
67 page: "https://app.example/x",
68 });
69 });
70
71 test("each named parameter is dropped in any case, the rest kept in order", () => {
72 for (const name of NAMES)
73 for (const n of spellings(name))
74 expect(parseContext({ page: `https://app.example/x?a=1&${n}=abc&b=2` })).toEqual({
75 page: "https://app.example/x?a=1&b=2",
76 });
77 });
78
79 test("a page holding only secrets loses its query entirely", () => {
80 expect(parseContext({ page: "https://app.example/x?token=abc&Code=def" })).toEqual({
81 page: "https://app.example/x",
82 });
83 });
84
85 test("the issue's example: neither the password nor the token survives", () => {
86 const c = parseContext({ page: "https://user:[email protected]/x?token=abc&tab=2" });
87 expect(c).toEqual({ page: "https://app.example/x?tab=2" });
88 const note = contextNote(c ?? undefined);
89 expect(note).not.toContain("pass");
90 expect(note).not.toContain("abc");
91 });
92
93 test("a page with nothing to drop is left as it was", () => {
94 for (const page of [
95 "https://app.example/x?tab=2&sort=desc&tokens=1&keyword=k&monkey=1&tokenizer=a",
96 "https://app.example/x?q=a%20b&tab",
97 "https://app.example/",
98 ])
99 expect(parseContext({ page })).toEqual({ page });
100 });
101
102 test("nothing accepted today is refused", () => {
103 expect(parseContext({ page: "https://app.example/x?=&&token" })).not.toBeNull();
104 });
105 });
106
107 describe("parseFrom", () => {
108 test("each named parameter is dropped in any case, the rest kept in order", () => {
109 for (const name of NAMES)
110 for (const n of spellings(name))
111 expect(parseFrom(`/alice/notes/pulls/3?a=1&${n}=abc&b=2`)).toBe(
112 "/alice/notes/pulls/3?a=1&b=2",
113 );
114 });
115
116 test("a query of only secrets is removed, a path without one is kept", () => {
117 expect(parseFrom("/alice/notes?invite=abc")).toBe("/alice/notes");
118 expect(parseFrom("/alice/notes?tab=files&sort=x")).toBe("/alice/notes?tab=files&sort=x");
119 expect(parseFrom("/alice/notes/pulls/3")).toBe("/alice/notes/pulls/3");
120 expect(parseFrom("/alice/notes?token=abc#line-3")).toBe("/alice/notes#line-3");
121 });
122
123 test("nothing accepted today is refused: a scrubbed path is a path, not undefined", () => {
124 expect(parseFrom("/a?token=x")).toBe("/a");
125 expect(parseFrom("/a?=&&token")).toBe("/a?=&");
126 });
127 });
128
129 describe("the doors", () => {
130 let dataDir: string;
131 let app: App;
132 let servers: Servers;
133 let rw: string;
134 let alice: User;
135 let game: Repo;
136
137 beforeAll(async () => {
138 dataDir = await mkdtemp(join(tmpdir(), "forge-tell-scrub-"));
139 app = createApp({ dataDir, port: 0 });
140 servers = await serve(app);
141 const a = await createUser(app.db, {
142 origin: "seed",
143 trusted: true,
144 username: "alice",
145 email: "[email protected]",
146 password: "a-good-passphrase",
147 adminIfFirst: true,
148 });
149 if (!a.ok) throw new Error(a.error);
150 alice = a.value;
151 rw = createToken(app.db, a.value.id, "rw", "rw").token;
152 const made = await createRepo(app, a.value, { name: "game", visibility: "private" });
153 if (!made.ok) throw new Error(String(made.error));
154 game = made.value;
155 await commitToBranch(app, a.value, game, {
156 message: "seed",
157 files: [{ path: "README.md", content: "# game\n" }],
158 });
159 }, 30_000);
160
161 afterAll(async () => {
162 servers.stop();
163 app.close();
164 await rm(dataDir, { recursive: true, force: true });
165 });
166
167 test("a tell with a credentialed page makes an issue holding neither secret", async () => {
168 const res = await fetch(`${servers.url}/mcp`, {
169 method: "POST",
170 headers: {
171 "content-type": "application/json",
172 accept: "application/json, text/event-stream",
173 authorization: `Bearer ${rw}`,
174 },
175 body: JSON.stringify({
176 jsonrpc: "2.0",
177 id: 1,
178 method: "tools/call",
179 params: {
180 name: "tell",
181 arguments: {
182 repo: "alice/game",
183 text: "the reset link is broken",
184 from: "/alice/game/issues?Session=abc&state=open",
185 context: { page: "https://user:[email protected]/x?token=abc&tab=2" },
186 },
187 },
188 }),
189 });
190 const body = (await res.json()) as { result: { isError?: boolean } };
191 expect(body.result.isError).not.toBe(true);
192 const issue = listIssues(app.db, game.id, {}).find(
193 (i) => i.title === "the reset link is broken",
194 );
195 expect(issue?.body).toContain("page: https://app.example/x?tab=2");
196 expect(issue?.body).toContain("/alice/game/issues?state=open");
197 expect(issue?.body).not.toContain("pass");
198 expect(issue?.body).not.toContain("abc");
199 });
200
201 const issueTitled = (title: string) =>
202 listIssues(app.db, game.id, {}).find((i) => i.title === title);
203
204 test("tell() takes a from with a secret, stores it without, and is not bad_from", async () => {
205 const t = await tell(app, alice, game, "the invite page", 1, "/alice/game?invite=abc&tab=2");
206 expect(t.ok).toBe(true);
207 if (!t.ok) return;
208 expect(t.value.issue.body).toContain("Told from [/alice/game?tab=2](/alice/game?tab=2)");
209 expect(issueTitled("the invite page")?.body).not.toContain("abc");
210 });
211
212 test("the web's tell form with a credentialed page or from stores neither secret", async () => {
213 const post = (fields: Record<string, string>) =>
214 fetch(`${servers.url}/alice/game/tell`, {
215 method: "POST",
216 headers: { authorization: `Bearer ${rw}` },
217 body: new URLSearchParams(fields),
218 redirect: "manual",
219 });
220 expect(
221 (await post({ text: "told from the pill", page: "https://user:[email protected]/x?Token=abc&tab=2" }))
222 .status,
223 ).toBe(200);
224 const pill = issueTitled("told from the pill")?.body ?? "";
225 expect(pill).toContain("- page: https://app.example/x?tab=2");
226 expect(pill).not.toContain("pass");
227 expect(pill).not.toContain("abc");
228 await post({ text: "told from a forge page", from: "/alice/game/issues?API-Key=abc&state=open" });
229 const page = issueTitled("told from a forge page")?.body ?? "";
230 expect(page).toContain("/alice/game/issues?state=open");
231 expect(page).not.toContain("abc");
232 });
233 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs