Merge the webhook query-value and reason-phrase redaction fix

Author: claude

packages/tasks/src/util/WebhookPost.ts

@@ -65,6 +65,14 @@ export function redactWebhookUrl(url: string): string {
   }
 }
 
+/**
+ * Routing segments of the supported providers' webhook paths. Redacting these
+ * would mangle prose like `invalid webhooks payload` for no gain. Everything
+ * else long enough to be a token is redacted even when all-lowercase — a
+ * lowercase secret is still a secret.
+ */
+const STRUCTURAL_PATH_SEGMENTS: ReadonlySet<string> = new Set(["services", "webhooks"]);
+
 /**
  * Strips every trace of `url` out of `text`.
  *
@@ -73,17 +81,32 @@ export function redactWebhookUrl(url: string): string {
  * `Cannot POST /services/T0.../SECRETTOKEN` — the path, never the origin — and
  * a validation error may quote the token on its own. So after the full URL
  * collapses to its origin (which stays the useful diagnostic), the path, the
- * query and each individual path segment are replaced too.
+ * query, each individual path segment AND each query VALUE are replaced too.
+ *
+ * Query values are a real carrier here, not a hypothetical one: plenty of
+ * webhook endpoints take their token as `?token=…` rather than as a path
+ * segment, and an echoing endpoint quotes back the value on its own.
  *
  * Candidates are applied LONGEST FIRST: replacing a short segment first would
  * break a longer candidate that contains it, leaving the rest of that longer
  * fragment behind.
  *
- * A candidate is admitted only when it is long enough to plausibly be a token
- * AND is not an all-lowercase word. Both clauses are load-bearing: `services`
- * and `webhooks` are real segments of the Slack and Discord webhook paths, and
- * redacting them would corrupt ordinary prose like `invalid webhooks payload`
- * for no security gain.
+ * Admission is by LENGTH only. A candidate shorter than
+ * `SECURITY_LIMITS.webhookMinRedactableSegmentChars` is far likelier to be an
+ * ordinary word than a token, and replacing it would mangle the diagnostic this
+ * redaction exists to preserve. That floor applies to query values too, so a
+ * short `?t=abc` still leaks — the same policy as segments, stated rather than
+ * silently special-cased.
+ *
+ * There is deliberately no "all-lowercase words are safe" exemption: a
+ * lowercase token is still a token, and the endpoint that echoes it does not
+ * care about its character class. The two lowercase strings actually worth
+ * keeping are the providers' own routing segments, which are named in
+ * {@link STRUCTURAL_PATH_SEGMENTS} instead.
+ *
+ * Cost, accepted: a generic webhook whose path carries a long lowercase WORD
+ * (`/notifications/deploy`) now has that word redacted out of echoed
+ * diagnostics.
  */
 export function redactWebhookUrlIn(text: string, url: string): string {
   if (url.length === 0) {
@@ -100,20 +123,40 @@ export function redactWebhookUrlIn(text: string, url: string): string {
 
   const candidates = new Set<string>();
   const admit = (candidate: string): void => {
-    if (
-      candidate.length >= SECURITY_LIMITS.webhookMinRedactableSegmentChars &&
-      !/^[a-z]+$/.test(candidate)
-    ) {
+    if (candidate.length >= SECURITY_LIMITS.webhookMinRedactableSegmentChars) {
       candidates.add(candidate);
     }
   };
+  const admitVariants = (candidate: string): void => {
+    admit(candidate);
+    const encoded = encodeURIComponent(candidate);
+    if (encoded !== candidate) {
+      admit(encoded);
+    }
+  };
+
   admit(`${parsed.pathname}${parsed.search}`);
   admit(parsed.search);
   for (const segment of parsed.pathname.split("/")) {
-    admit(segment);
-    const encoded = encodeURIComponent(segment);
-    if (encoded !== segment) {
-      admit(encoded);
+    if (STRUCTURAL_PATH_SEGMENTS.has(segment)) {
+      continue;
+    }
+    admitVariants(segment);
+  }
+  // Split the raw query rather than reading `searchParams`: the decoder turns
+  // `+` into a space, so the decoded value would not match the bytes an
+  // endpoint echoes back. Both forms are admitted.
+  for (const pair of parsed.search.replace(/^\?/, "").split("&")) {
+    const equals = pair.indexOf("=");
+    if (equals < 0) {
+      continue;
+    }
+    const raw = pair.slice(equals + 1);
+    admit(raw);
+    try {
+      admit(decodeURIComponent(raw));
+    } catch {
+      // Malformed escape: the raw form is what appears in an echo anyway.
     }
   }
 
@@ -582,14 +625,20 @@ export async function postWebhookJson(request: WebhookPostRequest): Promise<Webh
     request.includeBodyInError && !isPrivate && failureBody.length > 0
       ? `: ${truncate(redactWebhookUrlIn(failureBody, url), MAX_ERROR_BODY_CHARS)}`
       : "";
+  // The reason phrase is caller-controlled text like the body, so it gets the
+  // same two treatments: withheld entirely for a private destination, and
+  // redacted otherwise. A server is free to put anything in it, and the well
+  // known phrases ("Not Found", "Bad Request") survive redaction unchanged.
+  const statusText = isPrivate ? "" : redactWebhookUrlIn(response.statusText, url);
 
   throw createFetchUrlJobError(
     httpStatusToFetchUrlErrorCode(response.status),
-    `Failed to post ${label} to ${redacted}: ${response.status} ${response.statusText}${bodySuffix}`,
+    `Failed to post ${label} to ${redacted}: ${response.status}` +
+      `${statusText === "" ? "" : ` ${statusText}`}${bodySuffix}`,
     {
       url: redacted,
       httpStatus: response.status,
-      httpStatusText: response.statusText,
+      httpStatusText: statusText === "" ? undefined : statusText,
       retryDate,
     }
   );

packages/test/src/test/task/NotifyTask.test.ts

@@ -471,8 +471,9 @@ describe("Webhook notification tasks", () => {
 
     // The guard against over-redaction. `webhooks` is a real path segment of
     // every Discord webhook URL and is exactly 8 characters, so a length floor
-    // alone would delete the word from ordinary prose. All-lowercase-alphabetic
-    // runs are words, not tokens.
+    // alone would delete the word from ordinary prose. It is exempt because it
+    // is a named ROUTING segment of a supported provider, not because it is
+    // lowercase — this pins the replacement for the deleted lowercase rule.
     test("an ordinary word that happens to be a path segment survives", async () => {
       mockFetch.mockImplementation(() =>
         Promise.resolve(
@@ -488,6 +489,86 @@ describe("Webhook notification tasks", () => {
       expect(error.message).not.toContain("SECRETTOKEN");
     });
 
+    // A token in the query string is a real deployment shape — plenty of
+    // endpoints authenticate with `?token=…` rather than a path segment — and
+    // nothing admitted a query VALUE as a redaction candidate. The whole
+    // `?token=…` pair was a candidate, but an endpoint echoes the value alone,
+    // so the pair never matched and the secret went out verbatim.
+    test("a token carried in the query string is redacted from an echoed body", async () => {
+      mockFetch.mockImplementation(() =>
+        Promise.resolve(
+          new Response("bad token SUPERSECRET1", { status: 403, statusText: "Forbidden" })
+        )
+      );
+
+      const error = (await slackNotify({
+        url: "https://hooks.example.com/notify?token=SUPERSECRET1",
+        text: "hi",
+      }).catch((e: unknown) => e)) as PermanentJobError;
+
+      expect(error.message).not.toContain("SUPERSECRET1");
+      expect(String(error.stack)).not.toContain("SUPERSECRET1");
+      // The surrounding diagnostic survives; only the token is removed.
+      expect(error.message).toContain("bad token");
+    });
+
+    // An all-lowercase path segment was exempted outright, on the theory that
+    // such a run is a word rather than a token. A lowercase token is still a
+    // token, and the endpoint echoing it does not care about its character
+    // class.
+    test("an all-lowercase token in the path is redacted from an echoed body", async () => {
+      mockFetch.mockImplementation(() =>
+        Promise.resolve(
+          new Response("rejected: supersecrettoken", { status: 403, statusText: "Forbidden" })
+        )
+      );
+
+      const error = (await slackNotify({
+        url: "https://hooks.example.com/hooks/supersecrettoken",
+        text: "hi",
+      }).catch((e: unknown) => e)) as PermanentJobError;
+
+      expect(error.message).not.toContain("supersecrettoken");
+      expect(String(error.stack)).not.toContain("supersecrettoken");
+      expect(error.message).toContain("rejected");
+    });
+
+    // The stated cost of dropping the lowercase exemption, pinned rather than
+    // discovered later: a long lowercase word in a generic webhook's path is
+    // now redacted out of echoed diagnostics, because nothing distinguishes it
+    // from a lowercase token.
+    test("a long lowercase path word in a generic webhook is redacted, the accepted cost", async () => {
+      mockFetch.mockImplementation(() =>
+        Promise.resolve(
+          new Response("unknown notifications route", { status: 404, statusText: "Not Found" })
+        )
+      );
+
+      const error = (await slackNotify({
+        url: "https://hooks.example.com/notifications/deploy",
+        text: "hi",
+      }).catch((e: unknown) => e)) as PermanentJobError;
+
+      expect(error.message).not.toContain("notifications");
+      expect(error.message).toContain("unknown");
+    });
+
+    // `statusText` is caller-controlled text just like the body, and it was
+    // interpolated into the message and stored as `httpStatusText` with no
+    // redaction pass over it at all.
+    test("a reason phrase echoing the token is redacted", async () => {
+      mockFetch.mockImplementation(() =>
+        Promise.resolve(new Response("nope", { status: 403, statusText: "token SECRETTOKEN bad" }))
+      );
+
+      const error = (await slackNotify({ url: SLACK_URL, text: "hi" }).catch(
+        (e: unknown) => e
+      )) as PermanentJobError & { httpStatusText?: string };
+
+      expect(error.message).not.toContain("SECRETTOKEN");
+      expect(String(error.httpStatusText)).not.toContain("SECRETTOKEN");
+    });
+
     test("the webhook URL is absent from every output schema", () => {
       for (const taskClass of [WebhookNotifyTask, SlackNotifyTask, DiscordNotifyTask]) {
         const schema = taskClass.outputSchema();
@@ -1347,6 +1428,33 @@ describe("Webhook notification tasks", () => {
       expect(error.httpStatus).toBe(400);
     });
 
+    // The body was withheld for a private destination but the reason phrase was
+    // not, and a server is free to put anything in it. That left the SSRF read
+    // open through a narrower channel: the internal service names its own index
+    // in the phrase, and it reached both the message and `httpStatusText`.
+    test("Slack does not echo a private endpoint's reason phrase", async () => {
+      mockFetch.mockImplementation(() =>
+        Promise.resolve(
+          new Response(ES_BODY, { status: 400, statusText: "index=cluster-secrets shard=3" })
+        )
+      );
+
+      const error = (await slackNotify({
+        url: PRIVATE_URL,
+        text: "x",
+        allow_private_destination: true,
+      }).catch((e: unknown) => e)) as PermanentJobError & {
+        httpStatus?: number;
+        httpStatusText?: string;
+      };
+
+      expect(error.message).not.toContain("cluster-secrets");
+      // The status still reports; only the caller-controlled text is withheld.
+      expect(error.message).toContain("400");
+      expect(error.httpStatus).toBe(400);
+      expect(error.httpStatusText).toBeUndefined();
+    });
+
     test("Discord does not echo a private endpoint's failure body", async () => {
       mockFetch.mockImplementation(() =>
         Promise.resolve(new Response(ES_BODY, { status: 400, statusText: "Bad Request" }))