fix(daemon): render non-image https evidenceRef as a plain link, not an image block (#89)

* fix(daemon): render non-image https evidenceRef as a plain link, not an image block * fix(daemon): validate Slack evidence rendering boundaries
2026-10-02 08:35:15 +08:00 · 2026-10-01 00:39:34 -07:00
parent 274e05fdc2
commit 94a68a3d77
4 changed files with 246 additions and 13 deletions
@@ -32,6 +32,11 @@ export interface OutboundMessageOpts {
  extraBlocks?: unknown[];
  /** M1 A5b: outbound image attachments, rendered as Block Kit `image` blocks (the wired seam). */
  mediaRefs?: SlackMediaRef[];
+  /** #47 — an https evidenceRef that is NOT an image (GitLab issue link, PROOF.md URL, …),
+   *  rendered as a plain link in the message text + a context block. NEVER a Block Kit
+   *  `image` block: Slack rejects the ENTIRE message with `invalid_blocks` when an image
+   *  block's URL is not a real image, silently and permanently breaking delivery. */
+  evidenceLink?: string | null;
  /** S10 / A1.2 — the structured seat-attribution header (rig/host/seat/session), rendered as
   *  one sender context line in ONE honest bot identity. Authorship lives in OUR record;
   *  Slack's transport actor stays the app. NEVER a per-message username/icon override. */
@@ -76,7 +81,7 @@ export function buildImageBlocks(mediaRefs: readonly SlackMediaRef[] | undefined
  for (const m of mediaRefs) {
    const url = String(m.imageUrl || "");
    // Only forward a clean https URL that carries no secret (defense-in-depth, item 7).
-    if (!/^https:\/\/\S+$/.test(url) || containsSecret(url)) continue;
+    if (!isSafeHttpsUrl(url)) continue;
    blocks.push({
      type: "image",
      image_url: url,
@@ -128,6 +133,31 @@ function inert(text: string): string {
  return escapeSlackText(redactSecrets(text));
 }

+function isSafeHttpsUrl(url: string): boolean {
+  if (!/^https:\/\/\S+$/.test(url) || containsSecret(url)) return false;
+  try {
+    const parsed = new URL(url);
+    return !parsed.username && !parsed.password;
+  } catch {
+    return false;
+  }
+}
+
+/** #47 — render a non-image https evidenceRef as a plain link instead of a Block Kit
+ *  `image` block. The URL arrives pre-validated as `^https://\S+$` from the delivery
+ *  layer; item-7 hygiene still applies (a secret-bearing URL is refused, never
+ *  forwarded), and a URL carrying mrkdwn-breaking `<`, `>`, `|` degrades to escaped
+ *  plain text rather than a link. Returns null when there is nothing safe to render. */
+function buildEvidenceLink(url: string | null | undefined): { text: string; block: unknown } | null {
+  const ref = typeof url === "string" ? url.trim() : "";
+  if (!ref || !isSafeHttpsUrl(ref)) return null;
+  const escapedRef = inert(ref);
+  const text = `Evidence: ${escapedRef}`;
+  const context = bounded(/[<>|]/.test(ref) ? text : `Evidence: <${escapedRef}|evidence>`, SLACK_SECTION_CAP, "evidence context");
+  const block = { type: "context", elements: [{ type: "mrkdwn", text: context }] };
+  return { text, block };
+}
+
 /** S10 fix-r3 (R2 exactly-once) — the STRUCTURAL reconciliation identity: a bounded,
 *  decision-scoped token. decisionId is daemon-minted per decision (never settable through
 *  queue rows) and stable across retries of the same decision, so ONLY the target posted
@@ -168,13 +198,16 @@ export function buildOutboundMessage(q: QitemLike, opts: OutboundMessageOpts): S
  const attr = bounded(`from ${inert(opts.attribution?.session || opts.sourceLabel)}`, 2000, "sender");
  const imageBlocks = buildImageBlocks(opts.mediaRefs);
  const attachmentText = imageBlocks.map((b) => `Image: ${(b as { alt_text: string }).alt_text}`).join("\n");
+  const evidence = buildEvidenceLink(opts.evidenceLink);
  if (opts.extraBlocks?.length) {
    throw new HumanMessageShapeError("Extra blocks have no complete accessible fallback. Use mediaRefs for images or author supplemental human detail.");
  }
-  const text = bounded([headline, body, attr, attachmentText, opts.reconcileMarker].filter(Boolean).join("\n"), SLACK_TEXT_CAP, "complete fallback");
+  const text = bounded([headline, body, attr, evidence ? evidence.text : null, attachmentText, opts.reconcileMarker].filter(Boolean).join("\n"), SLACK_TEXT_CAP, "complete fallback");
  const blocks: unknown[] = [{ type: "section", text: { type: "mrkdwn", text: headline } }];
  if (body.trim()) blocks.push({ type: "section", text: { type: "mrkdwn", text: body } });
-  blocks.push(...imageBlocks, { type: "context", elements: [{ type: "mrkdwn", text: attr }] });
+  blocks.push(...imageBlocks);
+  if (evidence) blocks.push(evidence.block);
+  blocks.push({ type: "context", elements: [{ type: "mrkdwn", text: attr }] });
  if (blocks.length > 50) throw new HumanMessageShapeError("Message exceeds 50 Slack blocks. Reduce attachments before sending.");
  return { text, blocks };
 }
@@ -103,6 +103,43 @@ export function defaultReadLocalImage(refPath: string): { bytes: Uint8Array; fil
  }
 }

+/** #47 — only an https evidenceRef with an image-like extension may ride as a Block Kit
+ *  `image` block (extension set mirrors LOCAL_IMAGE_EXT). Slack rejects the ENTIRE
+ *  message with `invalid_blocks` when an image block's URL is not a real image (e.g. a
+ *  GitLab issue link or a PROOF.md URL — both explicitly documented evidenceRef uses),
+ *  so a non-image https ref must never become an image block. Query strings and
+ *  fragments are stripped before the extension check. */
+export function isHttpsImageRef(ref: unknown): boolean {
+  if (typeof ref !== "string") return false;
+  const url = ref.trim();
+  if (!/^https:\/\/\S+$/.test(url)) return false;
+  try {
+    return LOCAL_IMAGE_EXT.has(path.extname(new URL(url).pathname).toLowerCase());
+  } catch {
+    return false;
+  }
+}
+
+/** #47 — split an evidenceRef into an image attachment vs. a plain link. An explicit
+ *  `media` array stays fully caller-controlled; otherwise an image-looking https
+ *  evidenceRef becomes a Block Kit image and a non-image https evidenceRef becomes a
+ *  plain link (rendered by buildEvidenceLink, never an image block). Local refs keep
+ *  their existing handling (image upload flow / clean skip). */
+export function evidenceAttachment(
+  media: unknown,
+  evidenceRef: unknown,
+  summary: string | null | undefined,
+): { mediaRefs: SlackMediaRef[] | undefined; evidenceLink: string | undefined } {
+  if (Array.isArray(media)) return { mediaRefs: media as SlackMediaRef[], evidenceLink: undefined };
+  if (typeof evidenceRef !== "string") return { mediaRefs: undefined, evidenceLink: undefined };
+  const ref = evidenceRef.trim();
+  if (isHttpsImageRef(ref)) {
+    return { mediaRefs: [{ imageUrl: ref, altText: summary ?? "attachment" }], evidenceLink: undefined };
+  }
+  if (/^https:\/\/\S+$/.test(ref)) return { mediaRefs: undefined, evidenceLink: ref };
+  return { mediaRefs: undefined, evidenceLink: undefined };
+}
+
 /** Build the subsystem DeliverFn. Contract mirrors the retired connector handleDecision. */
 function deliverSinglePart(opts: SubsystemSlackDeliveryOpts, markEpisode = true): SubsystemDeliverFn {
  const log = opts.log ?? (() => {});
@@ -114,13 +151,10 @@ function deliverSinglePart(opts: SubsystemSlackDeliveryOpts, markEpisode = true)
    }
    const q = (decision.payload ?? {}) as OutboundPostPayload & { media?: SlackMediaRef[] };
    // M1 A5b (carried over from the retired sweep): an alert's evidenceRef IS the artifact the
-    // human judges — an https image URL rides as a Block Kit image. buildImageBlocks stays the
-    // single hygiene gate (drops non-https / secret-bearing), so the predicate lives in ONE place.
-    const mediaRefs: SlackMediaRef[] | undefined = Array.isArray(q.media)
-      ? q.media
-      : q.evidenceRef
-        ? [{ imageUrl: String(q.evidenceRef), altText: q.summary ?? "attachment" }]
-        : undefined;
+    // human judges. #47 — it rides as a Block Kit image ONLY when it looks like an image;
+    // a non-image https ref rides as a plain link instead (Slack's invalid_blocks rejects
+    // the whole message when an image block's URL is not a real image).
+    const { mediaRefs, evidenceLink } = evidenceAttachment(q.media, q.evidenceRef, q.summary);
    const payload = buildOutboundMessage(
      {
        qitemId: q.qitemId ?? decision.decisionId,
@@ -132,6 +166,7 @@ function deliverSinglePart(opts: SubsystemSlackDeliveryOpts, markEpisode = true)
        sourceLabel: opts.sourceLabel,
        bodyExcerpt: opts.bodyExcerpt,
        mediaRefs,
+        evidenceLink,
        // A1.2 — attribution rides every post; identity stays the app's own (postChatMessage
        // structurally cannot carry username/icon overrides — the customize-absence rail).
        attribution: attributionFromSession(q.sourceSession),
@@ -338,12 +373,16 @@ export function subsystemSlackDeliver(opts: SubsystemSlackDeliveryOpts): Subsyst
    const partId = (index: number) => parts.length === 1 ? decision.decisionId : `${decision.decisionId}:part:${index + 1}`;
    try {
      for (const [index, part] of parts.entries()) {
+        // #47 — preflight must mirror deliverSinglePart exactly: the same evidenceRef
+        // split (image attachment vs. plain link) so the shape check sees the true payload.
+        const partEvidence = evidenceAttachment(part.media, part.evidenceRef, part.summary);
        buildOutboundMessage(part, {
          sourceLabel: opts.sourceLabel,
          attribution: attributionFromSession(part.sourceSession),
          mentionUserId: index === 0 ? opts.resolveMentionUserId?.(q) : undefined,
          reconcileMarker: reconcileToken(partId(index)),
-          mediaRefs: Array.isArray(part.media) ? part.media : part.evidenceRef ? [{ imageUrl: part.evidenceRef, altText: part.summary ?? "attachment" }] : undefined,
+          mediaRefs: partEvidence.mediaRefs,
+          evidenceLink: partEvidence.evidenceLink,
        });
      }
    } catch (error) {
@@ -3,7 +3,7 @@
 // thread_ts) are captured at the fetch boundary. The live phone render is the named external
 // door; these receipts prove the mechanical path.
 import { describe, it, expect } from "vitest";
-import { subsystemSlackDeliver } from "../src/domain/gateway/slack/slack-delivery.js";
+import { subsystemSlackDeliver, isHttpsImageRef, evidenceAttachment } from "../src/domain/gateway/slack/slack-delivery.js";
 import { SeenStore, type StateFsOps } from "../src/domain/gateway/slack/state-store.js";
 import type { OutboundDecision } from "../src/domain/gateway/protocol.js";
 import type { FetchImpl } from "../src/domain/gateway/slack/slack-api.js";
@@ -110,3 +110,92 @@ describe("S10 outbound images — external-upload flow (founder screenshot class
    }
  });
 });
+
+describe("#47 — a non-image https evidenceRef never becomes a Block Kit image block", () => {
+  it("isHttpsImageRef: image extension (query/fragment stripped) → image; anything else → not", () => {
+    expect(isHttpsImageRef("https://example.invalid/shot.png")).toBe(true);
+    expect(isHttpsImageRef("https://example.invalid/shot.PNG?width=800#frag")).toBe(true);
+    expect(isHttpsImageRef("https://gitlab.com/acme/team/-/work_items/10")).toBe(false);
+    expect(isHttpsImageRef("https://example.invalid/PROOF.md")).toBe(false);
+    expect(isHttpsImageRef("http://example.invalid/shot.png")).toBe(false);
+    expect(isHttpsImageRef("/tmp/local-shot.png")).toBe(false);
+    expect(isHttpsImageRef(null)).toBe(false);
+    expect(isHttpsImageRef(42)).toBe(false);
+  });
+
+  it("evidenceAttachment: image https ref → media ref; non-image https ref → plain link; explicit media wins", () => {
+    const img = evidenceAttachment(undefined, "https://example.invalid/a.jpg", "s");
+    expect(img.mediaRefs).toEqual([{ imageUrl: "https://example.invalid/a.jpg", altText: "s" }]);
+    expect(img.evidenceLink).toBeUndefined();
+    const link = evidenceAttachment(undefined, "https://gitlab.com/acme/team/-/work_items/10", "s");
+    expect(link.mediaRefs).toBeUndefined();
+    expect(link.evidenceLink).toBe("https://gitlab.com/acme/team/-/work_items/10");
+    const explicit = evidenceAttachment([{ imageUrl: "https://example.invalid/x.png", altText: "a" }], "https://gitlab.com/y", "s");
+    expect(explicit.mediaRefs).toHaveLength(1); // explicit media stays caller-controlled
+    expect(explicit.evidenceLink).toBeUndefined();
+    const local = evidenceAttachment(undefined, "/tmp/shot.png", "s");
+    expect(local.mediaRefs).toBeUndefined();
+    expect(local.evidenceLink).toBeUndefined(); // local refs keep the upload-flow handling
+  });
+
+  it.each([
+    ["https://example.png", false],
+    ["https://example.invalid/PROOF.md?image=shot.png#preview.png", false],
+    ["https://example.invalid/shot%20one.PNG?caption=%3F%23#part%2F", true],
+    ["https://example.invalid/shot%2Epng", false],
+    ["https://example.invalid/shot.png%3Fdownload=1", false],
+    ["https://[invalid]/shot.png", false],
+    ["https://example.invalid:99999/shot.png", false],
+    ["https:///shot.png", false],
+  ])("classifies only the parsed URL pathname: %s", (url, expected) => {
+    expect(isHttpsImageRef(url)).toBe(expected);
+  });
+
+  it.each([
+    "https://user:password@example.invalid/proof",
+    "https://user:password@example.invalid/shot.png",
+    "https://user%3Apassword@example.invalid/shot.png",
+  ])("omits URL userinfo before posting evidence: %s", async (url) => {
+    const { fetchImpl, calls } = slackFetch();
+    const out = await makeDeliver(fetchImpl)(decision(url));
+    expect(out.ok).toBe(true);
+    expect(calls.map((call) => call.url)).toEqual(["https://slack.com/api/chat.postMessage"]);
+    expect(JSON.stringify(calls[0]!.body)).not.toContain(url);
+    expect(JSON.stringify(calls[0]!.body)).not.toContain("Evidence:");
+    expect((calls[0]!.body as { blocks: { type: string }[] }).blocks.some((block) => block.type === "image")).toBe(false);
+  });
+
+  it.each([
+    { name: "evidence context", evidenceRef: "https://example.invalid/" + "a".repeat(3000), body: "b", error: /evidence context/ },
+    { name: "escaped evidence context", evidenceRef: "https://example.invalid/?" + "&".repeat(600), body: "b", error: /evidence context/ },
+    { name: "complete fallback", evidenceRef: "https://example.invalid/" + "a".repeat(1500), body: "b".repeat(2500), error: /complete fallback/ },
+  ])("rejects an oversized $name in preflight before any Slack request", async ({ evidenceRef, body, error }) => {
+    const { fetchImpl, calls } = slackFetch();
+    const outbound = decision(evidenceRef);
+    outbound.payload = { ...(outbound.payload as Record<string, unknown>), body };
+    const out = await makeDeliver(fetchImpl)(outbound);
+    expect(out).toMatchObject({ ok: false, class: "human-message-unrenderable", detail: expect.stringMatching(error) });
+    expect(calls).toHaveLength(0);
+  });
+
+  it("a GitLab issue-link evidenceRef posts with NO image block and a plain evidence link", async () => {
+    const { fetchImpl, calls } = slackFetch();
+    const out = await makeDeliver(fetchImpl)(decision("https://gitlab.com/acme/team/-/work_items/10"));
+    expect(out.ok).toBe(true);
+    const body = calls[0]!.body as { blocks: { type: string; image_url?: string }[]; text: string };
+    expect(body.blocks.filter((b) => b.type === "image")).toHaveLength(0);
+    const contexts = body.blocks.filter((b) => b.type === "context");
+    expect(JSON.stringify(contexts)).toContain("https://gitlab.com/acme/team/-/work_items/10");
+    expect(body.text).toContain("Evidence: https://gitlab.com/acme/team/-/work_items/10");
+  });
+
+  it("an https image URL with a query string still rides as a Block Kit image (no behavior change)", async () => {
+    const { fetchImpl, calls } = slackFetch();
+    await makeDeliver(fetchImpl)(decision("https://example.invalid/board.png?width=800"));
+    const blocks = (calls[0]!.body as { blocks: { type: string; image_url?: string }[] }).blocks;
+    const images = blocks.filter((b) => b.type === "image");
+    expect(images).toHaveLength(1);
+    expect(images[0]!.image_url).toBe("https://example.invalid/board.png?width=800");
+    expect((calls[0]!.body as { text: string }).text).not.toContain("Evidence:");
+  });
+});
@@ -1,5 +1,5 @@
 import { describe, it, expect } from "vitest";
-import { buildOutboundMessage, buildImageBlocks, containsSecret, redactSecrets, SLACK_TEXT_CAP } from "../src/domain/gateway/slack/message.js";
+import { buildOutboundMessage, buildImageBlocks, containsSecret, redactSecrets, SLACK_SECTION_CAP, SLACK_TEXT_CAP } from "../src/domain/gateway/slack/message.js";

 describe("Slice-11 outbound message — content hygiene (item 7)", () => {
  const opts = { sourceLabel: "vm-openrig-build" };
@@ -50,6 +50,78 @@ describe("Slice-11 outbound message — content hygiene (item 7)", () => {
  });
 });

+describe("#47 evidence link boundaries", () => {
+  const qitem = { qitemId: "q-evidence", summary: "s", body: "b" };
+  const opts = { sourceLabel: "vm" };
+
+  it.each([
+    "https://user:password@example.invalid/shot.png",
+    "https://user@example.invalid/shot.png",
+    "https://:password@example.invalid/shot.png",
+    "https://user%3Apassword@example.invalid/shot.png",
+    "https://[invalid]/shot.png",
+    "https://example.invalid:99999/shot.png",
+    "https://hooks.slack.com/services/T00/B00/SECRETPART",
+    "https://example.invalid/proof?token=xoxb-EXAMPLE-000000-leak",
+  ])("never renders an unsafe URL in evidence or image blocks: %s", (url) => {
+    const message = buildOutboundMessage(qitem, { ...opts, evidenceLink: url });
+    expect(message.text).not.toContain("Evidence:");
+    expect(JSON.stringify(message)).not.toContain(url);
+    expect(buildImageBlocks([{ imageUrl: url, altText: "attachment" }])).toEqual([]);
+  });
+
+  it("preserves percent-encoded URLs and escapes ampersands in the final context and fallback", () => {
+    const url = "https://example.invalid/a%20b/%3Cproof%3E?next=%7Cevidence%7C&view=full";
+    const escaped = url.replaceAll("&", "&amp;");
+    const message = buildOutboundMessage(qitem, { ...opts, evidenceLink: url });
+    expect(message.text).toContain(`Evidence: ${escaped}`);
+    expect(message.blocks).toContainEqual({
+      type: "context", elements: [{ type: "mrkdwn", text: `Evidence: <${escaped}|evidence>` }],
+    });
+  });
+
+  it.each([
+    { name: "link", character: "a", escaped: "a", prefix: "Evidence: <", suffix: "|evidence>" },
+    { name: "plain text", character: "|", escaped: "|", prefix: "Evidence: ", suffix: "" },
+    { name: "escaped link", character: "&", escaped: "&amp;", prefix: "Evidence: <", suffix: "|evidence>" },
+    { name: "escaped plain text", character: "<", escaped: "&lt;", prefix: "Evidence: ", suffix: "" },
+  ])("bounds the final $name context at 3000 units without clipping", ({ character, escaped, prefix, suffix }) => {
+    const base = "https://example.invalid/";
+    const budget = SLACK_SECTION_CAP - prefix.length - base.length - suffix.length;
+    const count = Math.floor(budget / escaped.length);
+    const padding = "a".repeat(budget % escaped.length);
+    const url = base + character.repeat(count) + padding;
+    const context = prefix + base + escaped.repeat(count) + padding + suffix;
+    const message = buildOutboundMessage(qitem, { ...opts, evidenceLink: url });
+    expect(context.length).toBe(SLACK_SECTION_CAP);
+    expect(message.blocks).toContainEqual({ type: "context", elements: [{ type: "mrkdwn", text: context }] });
+    expect(() => buildOutboundMessage(qitem, { ...opts, evidenceLink: url + character })).toThrow(/evidence context.*maximum 3000/);
+  });
+
+  it("includes evidence, images, attribution, mentions and the reconcile marker in the complete fallback budget", () => {
+    const evidenceLink = "https://example.invalid/proof?view=full&revision=1";
+    const fullOpts = {
+      ...opts,
+      evidenceLink,
+      attribution: { seat: "dev@rig", session: "dev@rig@host" },
+      mentionUserId: "U-EVIDENCE",
+      reconcileMarker: "(or-mark:d-evidence)",
+      mediaRefs: [{ imageUrl: "https://example.invalid/shot.png", altText: "Screenshot & evidence" }],
+    };
+    const brief = { ...qitem, summary: "s".repeat(1000) };
+    const baseline = buildOutboundMessage(brief, fullOpts);
+    const body = "b".repeat(1 + SLACK_TEXT_CAP - baseline.text.length);
+    const message = buildOutboundMessage({ ...brief, body }, fullOpts);
+    expect(message.text.length).toBe(SLACK_TEXT_CAP);
+    expect(message.text).toContain("Evidence: " + evidenceLink.replaceAll("&", "&amp;"));
+    expect(message.text).toContain("Image: Screenshot &amp; evidence");
+    expect(message.text).toContain("from dev@rig@host");
+    expect(message.text).toContain("<@U-EVIDENCE>");
+    expect(message.text.endsWith(fullOpts.reconcileMarker)).toBe(true);
+    expect(() => buildOutboundMessage({ ...brief, body: body + "b" }, fullOpts)).toThrow(/complete fallback.*maximum 3900/);
+  });
+});
+
 describe("M1 A5b — outbound image attachments (the wired T1076 seam)", () => {
  const opts = { sourceLabel: "vm-openrig-build" };
  const imageBlocks = (m: { blocks: unknown[] }) => m.blocks.filter((b) => (b as { type?: string }).type === "image") as { type: string; image_url: string; alt_text: string }[];