fix: Filter junk from GitHub attachment import, and turn it on by default

.changeset/ingest-junk-filters.md

@@ -0,0 +1,5 @@
+---
+"@buildinternet/uploads": minor
+---
+
+GitHub attachment import is on by default for linked repos, with junk filters: attachments authored by `[bot]` accounts and images under 200px on either side are skipped. A new `.uploads.yml` key, `ingestBotAttachments: true`, re-admits bot media on the webhook path; `ingestGithubAttachments: false` turns importing off per repo or per workspace as before.

apps/api/src/github-ingest.test.ts

@@ -27,6 +27,7 @@ import type { WorkspaceRecord } from "./workspace";
 import { FakeKv } from "../test/fake-kv";
 import { GITHUB_APP_CFG_ENV } from "../test/github-app-env";
 import { fakeFetch, pngRoute, withGlobalFetch } from "../test/helpers/github-fetch-fakes";
+import { gifOf } from "../test/helpers/image-fixtures";
 import { UsageFakeD1 } from "../test/usage-fake-d1";
 
 const REPO = "acme/app";
@@ -336,6 +337,90 @@ describe("reconcileIngestSource", () => {
     expect(summary.skipped).toEqual([{ url: ASSET_URL, reason: "unsupported_media_type" }]);
   });
 
+  it("bot filter: an asset authored by a [bot] login is skipped without fetching", async () => {
+    const { env } = baseEnv();
+    const ref: IngestSourceRef = { repo: REPO, kind: "issues", num: 3, source: "comment:9" };
+    const fetchImpl = vi.fn(fakeFetch({ [ASSET_ID]: pngRoute(PNG) }));
+    const { putImpl, calls } = spyPut();
+
+    const summary = await reconcileIngestSource(
+      env,
+      ws,
+      WS,
+      ref,
+      `see ${ASSET_URL}`,
+      "claude[bot]",
+      { fetchImpl, putImpl },
+    );
+
+    expect(fetchImpl).not.toHaveBeenCalled();
+    expect(calls).toHaveLength(0);
+    expect(summary.skipped).toEqual([{ url: ASSET_URL, reason: "bot_author" }]);
+    expect(await ledgerRow(env.DB, REPO, ASSET_ID)).toBeNull();
+  });
+
+  it("bot filter: ingestBotAuthors deliberately re-admits bot-authored assets", async () => {
+    const { env } = baseEnv();
+    const ref: IngestSourceRef = { repo: REPO, kind: "issues", num: 3, source: "comment:9" };
+    const fetchImpl = fakeFetch({ [ASSET_ID]: pngRoute(PNG) });
+    const { putImpl, calls } = spyPut();
+
+    const summary = await reconcileIngestSource(
+      env,
+      ws,
+      WS,
+      ref,
+      `see ${ASSET_URL}`,
+      "claude[bot]",
+      { fetchImpl, putImpl, ingestBotAuthors: true },
+    );
+
+    expect(calls).toHaveLength(1);
+    expect(summary.skipped).toEqual([]);
+    expect(summary.ingested).toHaveLength(1);
+  });
+
+  it("small-image filter: an image under the minimum dimension is a permanent too_small skip", async () => {
+    const { env } = baseEnv();
+    const ref: IngestSourceRef = { repo: REPO, kind: "pull", num: 7, source: "body" };
+    // Real GIF89a header with a 128×128 logical screen — the emoji/badge
+    // junk this gate exists for.
+    const tinyGif = gifOf(128, 128);
+    const fetchImpl = fakeFetch({
+      [ASSET_ID]: () =>
+        new Response(tinyGif, { status: 200, headers: { "content-type": "image/gif" } }),
+    });
+    const { putImpl, calls } = spyPut();
+
+    const summary = await reconcileIngestSource(env, ws, WS, ref, `see ${ASSET_URL}`, "octocat", {
+      fetchImpl,
+      putImpl,
+    });
+
+    expect(calls).toHaveLength(0);
+    expect(summary.skipped).toEqual([{ url: ASSET_URL, reason: "too_small" }]);
+    expect(await ledgerRow(env.DB, REPO, ASSET_ID)).toBeNull();
+  });
+
+  it("small-image filter: an image at or above the minimum dimension ingests", async () => {
+    const { env } = baseEnv();
+    const ref: IngestSourceRef = { repo: REPO, kind: "pull", num: 7, source: "body" };
+    const bigGif = gifOf(800, 600);
+    const fetchImpl = fakeFetch({
+      [ASSET_ID]: () =>
+        new Response(bigGif, { status: 200, headers: { "content-type": "image/gif" } }),
+    });
+    const { putImpl, calls } = spyPut();
+
+    const summary = await reconcileIngestSource(env, ws, WS, ref, `see ${ASSET_URL}`, "octocat", {
+      fetchImpl,
+      putImpl,
+    });
+
+    expect(calls).toHaveLength(1);
+    expect(summary.skipped).toEqual([]);
+  });
+
   it("guard skip: asset 404 is a permanent skip", async () => {
     const { env } = baseEnv();
     const ref: IngestSourceRef = { repo: REPO, kind: "pull", num: 7, source: "body" };
@@ -560,9 +645,13 @@ describe("ingestForWebhook", () => {
     expect(fetchImpl).not.toHaveBeenCalled();
   });
 
-  it("link + workspace but knob false: resolves without fetching the issue body or asset", async () => {
+  it("link + workspace but knob explicitly false: resolves without fetching the issue body or asset", async () => {
     const { env: base } = baseEnv();
-    const env = withRegistry(base, { provider: "r2", bucket: "b" } as WorkspaceRecord);
+    const env = withRegistry(base, {
+      provider: "r2",
+      bucket: "b",
+      githubIngestAttachments: false,
+    } as WorkspaceRecord);
     await recordRepoLink(env.DB, REPO, WS, "test");
     const calls: string[] = [];
     const impl = ((url: string, init: RequestInit = {}) => {
@@ -576,13 +665,9 @@ describe("ingestForWebhook", () => {
     expect(calls.some((u) => u.includes("/issues/7") || u.includes(ASSET_ID))).toBe(false);
   });
 
-  it("knob true: fetches the issue body, puts, ledgers", async () => {
+  it("knob unset: ingestion is on by default — fetches the issue body, puts, ledgers", async () => {
     const { env: base } = baseEnv();
-    const env = withRegistry(base, {
-      provider: "r2",
-      bucket: "b",
-      githubIngestAttachments: true,
-    } as WorkspaceRecord);
+    const env = withRegistry(base, { provider: "r2", bucket: "b" } as WorkspaceRecord);
     await recordRepoLink(env.DB, REPO, WS, "test");
     const impl = fakeFetch({
       "/contents/": () => new Response("nf", { status: 404 }),
@@ -603,6 +688,47 @@ describe("ingestForWebhook", () => {
     expect(row).not.toBeNull();
   });
 
+  it("passes the resolved ingestBotAttachments knob through: a repo config enabling it re-admits bot authors", async () => {
+    const { env: base } = baseEnv();
+    const env = withRegistry(base, { provider: "r2", bucket: "b" } as WorkspaceRecord);
+    await recordRepoLink(env.DB, REPO, WS, "test");
+    const impl = fakeFetch({
+      "/contents/": () => new Response("comment:\n  ingestBotAttachments: true\n", { status: 200 }),
+      "/repos/acme/app/issues/7": () =>
+        new Response(JSON.stringify({ body: `see ${ASSET_URL}`, user: { login: "claude[bot]" } }), {
+          status: 200,
+        }),
+      [ASSET_ID]: pngRoute(PNG),
+    });
+    const { putImpl, calls } = spyPut();
+    const ref: IngestSourceRef = { repo: REPO, kind: "pull", num: 7, source: "body" };
+
+    await withGlobalFetch(impl, () => ingestForWebhook(env, ref, { fetchImpl: impl, putImpl }));
+
+    expect(calls).toHaveLength(1);
+  });
+
+  it("filters a bot-authored body by default (no repo config)", async () => {
+    const { env: base } = baseEnv();
+    const env = withRegistry(base, { provider: "r2", bucket: "b" } as WorkspaceRecord);
+    await recordRepoLink(env.DB, REPO, WS, "test");
+    const impl = fakeFetch({
+      "/contents/": () => new Response("nf", { status: 404 }),
+      "/repos/acme/app/issues/7": () =>
+        new Response(JSON.stringify({ body: `see ${ASSET_URL}`, user: { login: "claude[bot]" } }), {
+          status: 200,
+        }),
+      [ASSET_ID]: pngRoute(PNG),
+    });
+    const { putImpl, calls } = spyPut();
+    const ref: IngestSourceRef = { repo: REPO, kind: "pull", num: 7, source: "body" };
+
+    await withGlobalFetch(impl, () => ingestForWebhook(env, ref, { fetchImpl: impl, putImpl }));
+
+    expect(calls).toHaveLength(0);
+    expect(await ledgerRow(env.DB, REPO, ASSET_ID)).toBeNull();
+  });
+
   it("comment source 404: reconciles with text null (detach-all), not a throw", async () => {
     const { env: base } = baseEnv();
     const env = withRegistry(base, {

apps/api/src/github-ingest.ts

@@ -43,7 +43,12 @@ import {
   setLedgerSource,
 } from "./github-ingest-ledger";
 import { updateFileMetadataValue } from "./file-metadata";
-import { detectContentType, maxBytesForContentType, resolveUploadPolicy } from "./guards";
+import {
+  detectContentType,
+  detectImageDimensions,
+  maxBytesForContentType,
+  resolveUploadPolicy,
+} from "./guards";
 import { putObject } from "./files-core";
 import { findRepoLinkStrict } from "./github-repo-links";
 import { resolveRepoCommentOptions } from "./repo-comment-config";
@@ -88,8 +93,24 @@ export interface IngestDeps {
    * today's behavior.
    */
   mode?: GhKeyMode;
+  /**
+   * Re-admit attachments authored by `*[bot]` logins (issue #690 junk
+   * filter). Absent/false — the default — skips them permanently
+   * (`bot_author`); webhook ingestion threads the resolved
+   * `ingestBotAttachments` knob through here.
+   */
+  ingestBotAuthors?: boolean;
 }
 
+/**
+ * Floor on either pixel dimension for ingested images (issue #690): the
+ * emoji, badge, and tracking-pixel junk bots embed is far below it, real
+ * screenshots are far above. Applies only when the header's dimensions are
+ * decodable — an undecodable header fails open, since this is an index tier
+ * whose originals stay on GitHub either way.
+ */
+const MIN_INGEST_IMAGE_DIMENSION = 200;
+
 function emptySummary(): IngestSummary {
   return { ingested: [], reattached: [], detached: [], skipped: [] };
 }
@@ -235,6 +256,14 @@ async function fetchAndStore(
     return { kind: "skip", reason: "too_large" };
   }
 
+  const dims = detectImageDimensions(bytes, sniffed);
+  if (
+    dims &&
+    (dims.width < MIN_INGEST_IMAGE_DIMENSION || dims.height < MIN_INGEST_IMAGE_DIMENSION)
+  ) {
+    return { kind: "skip", reason: "too_small" };
+  }
+
   const mode: GhKeyMode = deps.mode ?? { mode: "plain" };
   const key = ingestKeyForMode(mode, ref, attachment.id, extensionForContentType(sniffed));
   try {
@@ -303,6 +332,12 @@ export async function reconcileIngestSource(
     await Promise.all(found.map(async (a) => [a.id, await ledgerRow(db, ref.repo, a.id)] as const)),
   );
 
+  // Bot-author gate applies only to NEW assets: an already-ledgered asset was
+  // admitted under whatever policy held when it was fetched, so reattach/
+  // source-move bookkeeping below still runs for it — junk never enters the
+  // ledger in the first place, and this loop never re-fetches ledgered rows.
+  const skipBotAuthor = author !== null && author.endsWith("[bot]") && !deps.ingestBotAuthors;
+
   for (const attachment of found) {
     const row = rows.get(attachment.id) ?? null;
     if (row) {
@@ -330,6 +365,11 @@ export async function reconcileIngestSource(
       continue;
     }
 
+    if (skipBotAuthor) {
+      summary.skipped.push({ url: attachment.url, reason: "bot_author" });
+      continue;
+    }
+
     const result = await fetchAndStore(env, ws, workspaceName, ref, attachment, author, deps);
     if (result.kind === "skip") {
       summary.skipped.push({ url: attachment.url, reason: result.reason });
@@ -416,6 +456,7 @@ export async function ingestForWebhook(
     ...deps,
     token,
     mode,
+    ingestBotAuthors: options.ingestBotAttachments,
   });
 }
 

apps/api/src/guards.ts

@@ -115,6 +115,82 @@ export function detectContentType(bytes: Uint8Array): string | null {
   return null;
 }
 
+export interface ImageDimensions {
+  width: number;
+  height: number;
+}
+
+/**
+ * Best-effort pixel dimensions read straight from an image's header —
+ * PNG (IHDR), GIF (logical screen descriptor), JPEG (first SOF marker),
+ * WebP (VP8X/VP8/VP8L chunk). Returns `undefined` for any other content
+ * type or a header it can't decode; callers that gate on dimensions must
+ * fail open on `undefined` rather than treating it as zero.
+ */
+export function detectImageDimensions(
+  bytes: Uint8Array,
+  contentType: string,
+): ImageDimensions | undefined {
+  const view = new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength);
+  const valid = (width: number, height: number) =>
+    width > 0 && height > 0 ? { width, height } : undefined;
+
+  if (contentType === "image/png") {
+    // 8-byte signature, 4-byte chunk length, "IHDR", then BE uint32 w/h.
+    if (bytes.length < 24 || !matches(bytes, [0x49, 0x48, 0x44, 0x52], 12)) return undefined;
+    return valid(view.getUint32(16, false), view.getUint32(20, false));
+  }
+  if (contentType === "image/gif") {
+    // "GIF8xa" then LE uint16 logical screen width/height.
+    if (bytes.length < 10) return undefined;
+    return valid(view.getUint16(6, true), view.getUint16(8, true));
+  }
+  if (contentType === "image/jpeg") {
+    // Walk marker segments to the first start-of-frame (SOF0–SOF15, minus
+    // the non-frame DHT/DAC/RST markers in that range).
+    let i = 2;
+    while (i + 9 < bytes.length) {
+      if (bytes[i] !== 0xff) return undefined;
+      const marker = bytes[i + 1]!;
+      if (marker === 0xd8 || (marker >= 0xd0 && marker <= 0xd9)) {
+        i += 2;
+        continue;
+      }
+      const isSof =
+        marker >= 0xc0 && marker <= 0xcf && marker !== 0xc4 && marker !== 0xc8 && marker !== 0xcc;
+      if (isSof) {
+        return valid(view.getUint16(i + 7, false), view.getUint16(i + 5, false));
+      }
+      i += 2 + view.getUint16(i + 2, false);
+    }
+    return undefined;
+  }
+  if (contentType === "image/webp") {
+    if (bytes.length < 30) return undefined;
+    const chunk = asciiAt(bytes, 12, 4);
+    if (chunk === "VP8X") {
+      // 24-bit LE canvas width-1 / height-1 at payload offsets 4 and 7.
+      const w = bytes[24]! | (bytes[25]! << 8) | (bytes[26]! << 16);
+      const h = bytes[27]! | (bytes[28]! << 8) | (bytes[29]! << 16);
+      return valid(w + 1, h + 1);
+    }
+    if (chunk === "VP8 ") {
+      // Lossy bitstream: 14-bit LE dimensions after the 3-byte frame tag +
+      // 3-byte start code (9D 01 2A).
+      if (!matches(bytes, [0x9d, 0x01, 0x2a], 23)) return undefined;
+      return valid(view.getUint16(26, true) & 0x3fff, view.getUint16(28, true) & 0x3fff);
+    }
+    if (chunk === "VP8L") {
+      // Lossless bitstream: signature 0x2f then 14-bit width-1/height-1.
+      if (bytes[20] !== 0x2f) return undefined;
+      const bits = view.getUint32(21, true);
+      return valid((bits & 0x3fff) + 1, ((bits >> 14) & 0x3fff) + 1);
+    }
+    return undefined;
+  }
+  return undefined;
+}
+
 export type UploadRejection = {
   ok: false;
   status: 413 | 415;

apps/api/src/routes/me.test.ts

@@ -2313,7 +2313,8 @@ describe("GET /me/workspaces/:name/comment-preview", () => {
       metaState: true,
       linkToFilePage: true,
       note: null,
-      ingestGithubAttachments: false,
+      ingestGithubAttachments: true,
+      ingestBotAttachments: false,
     });
     expect(body.source).toMatchObject({ imageWidth: "auto" });
     expect(typeof body.body).toBe("string");