fix(webapp): badge the credential the share gate already found (BEA-147) (#183)

* fix(webapp): badge the credential the share gate already found (BEA-147) The hub could identify an AWS access key on line 3 well enough to refuse to publish the file, and rendered that same key to every member as ordinary body text. scanSecrets had exactly one caller — share minting — so the strongest protection in the product sat on the rarest path and was absent from the path every file takes. The render response now carries the same finding, omitted when the file is clean, and the markdown file view shows an advisory strip above the content. Advisory only: nothing is blocked and nothing is redacted, because a member who can open the file could already read the key. The label vocabulary moves out of Browser.tsx into lib/secrets.ts, shared by the badge and the share dialog, so the two surfaces cannot drift apart on the wording of the same finding. The ?sha= history render is scanned too — two lines, and it stops the badge vanishing the moment you click into history on the file it was warning about. Rule ids and line numbers only. The matched text reaches no response body and no log line, pinned by a test on the new caller the way shares_test.go pins the old one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(architecture): the credential scan gains a render-path caller (BEA-147) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 08:08:08 +02:00 · 2026-08-19 12:11:00 -07:00
parent 039e350d79
commit 20f59352ca
15 changed files with 404 additions and 69 deletions
@@ -265,6 +265,29 @@ test("share mints a public link that serves the file, revoke kills it", async ({
  expect(gone.status()).toBe(404);
 });

+// BEA-147: the same finding the share dialog names, on the path every file
+// takes — the viewer used to render the key as ordinary prose.
+test("a file holding a key carries a badge in the file view", async ({ page }) => {
+  await login(page);
+  const pid = await wikiId(page);
+  await page.goto(`/${pid}/deploy.md`);
+
+  const badge = page.locator(".sbadge");
+  await expect(badge).toBeVisible();
+  // Same rule, same line, same words as the share dialog's modal below.
+  await expect(badge).toContainText("an AWS access key (line 3)");
+  // Advisory: the file still renders in full, nothing is redacted.
+  await expect(page.locator("#content h1")).toHaveText("Deploy");
+  await expect(page.locator("#content")).toContainText("AWS_ACCESS_KEY_ID");
+  // The badge itself must never echo the thing it found.
+  await expect(badge).not.toContainText("AKIA");
+
+  // A clean file gets no badge at all.
+  await page.goto(`/${pid}/index.md`);
+  await expect(page.locator("#content h1")).toHaveText("Wiki");
+  await expect(page.locator(".sbadge")).toHaveCount(0);
+});
+
 // BEA-111: sharing a file that looks like it holds credentials asks first.
 // Cancel mints nothing; Share anyway mints the link it would have.
 test("share on a file holding a key asks before it mints, and Cancel mints nothing", async ({
@@ -3,6 +3,8 @@
 // The API is deliberately storage-blind: nothing here ever names a bucket,
 // remote URL, or credential, and heat responses carry no actor identities.

+import type { SecretFinding } from "../lib/secrets";
+
 // GET /api/config (handleConfig, server.go)
 export interface ServerConfig {
  mode: "volume" | "hub";
@@ -154,6 +156,10 @@ export interface RenderDoc {
  user_name?: string;
  author?: string;
  device?: string;
+  // The share gate's credential scan, run on the render path too (BEA-147).
+  // Omitted by the server when the file is clean, so a truthiness test is
+  // the whole check. Rule ids and line numbers only — never the matched text.
+  findings?: SecretFinding[];
 }

 // GET .../heat (handleHeat, reads.go) — counts only, never who.
@@ -32,36 +32,10 @@ import { Insights, useInsightsDevices } from "../components/Insights";
 import { HistoryView, historyTitle } from "../components/HistoryView";
 import type { Run } from "../lib/runs";
 import { VersionBanner } from "../components/VersionBanner";
+import { secretsMessage } from "../lib/secrets";
 import { ConflictBanner } from "../components/ConflictBanner";
 import { parseConflict } from "../lib/conflict";

-// The hub's six share-time credential rules, in words. Only one caller
-// (shareNow), so it lives here rather than in its own file.
-const SECRET_LABELS: Record<string, string> = {
-  aws_access_key_id: "an AWS access key",
-  openai_api_key: "an OpenAI API key",
-  github_pat: "a GitHub token",
-  slack_token: "a Slack token",
-  private_key: "a private key",
-  gitlab_pat: "a GitLab token",
-};
-
-// secretsMessage phrases the 409 for the confirm dialog. The second sentence
-// is not decoration: a link always serves the file's LATEST content, so the
-// copy may only ever claim what was true at the moment of sharing — never
-// that the file is clean.
-function secretsMessage(findings: { rule: string; line: number }[] = []): string {
-  const parts = findings.map((f) => `${SECRET_LABELS[f.rule] ?? f.rule} (line ${f.line})`);
-  const what =
-    parts.length > 1
-      ? parts.slice(0, -1).join(", ") + " and " + parts[parts.length - 1]
-      : parts[0] || "something credential-shaped";
-  return (
-    `BearDrive found ${what} in this file. The check covers the file at the moment you share it — ` +
-    `a link always serves the file's latest content, so later changes are never checked. Share anyway?`
-  );
-}
-
 // The browsing surface shared by hub projects and single-volume mode: the
 // file tree, folder listings, file views, and every topbar action. Sidebar
 // chrome (vault header, project nav, org bar) is injected by the caller;
@@ -20,6 +20,8 @@ import {
 import { urlForPath } from "../router";
 import { CSV_ROWS, parseDelimited, type Csv } from "../lib/csv";
 import { hasMermaid, renderMermaid } from "../lib/mermaid";
+import { secretsBadge, type SecretFinding } from "../lib/secrets";
+import { Icon } from "./shell";

 export function FileView(props: {
  apiBase: string;
@@ -235,10 +237,36 @@ function MarkdownView(props: Parameters<typeof FileView>[0]) {
  // Server-rendered, server-sanitized markdown — same trust model as the
  // classic app assigning innerHTML.
  return (
-    <div
-      dangerouslySetInnerHTML={{ __html: diagrams ?? html }}
-      onClick={(e) => handleLinkClick(e, path, onOpenFile)}
-    />
+    <>
+      <SecretBadge findings={doc.findings} />
+      <div
+        dangerouslySetInnerHTML={{ __html: diagrams ?? html }}
+        onClick={(e) => handleLinkClick(e, path, onOpenFile)}
+      />
+    </>
+  );
+}
+
+/* The share gate could already name the rule and the line well enough to
+   refuse to publish this file, while the file view rendered the same key as
+   ordinary prose (BEA-147). Advisory only, in VersionBanner's shape: a strip
+   above the content, role="status", no actions. Nothing is blocked and
+   nothing is redacted — a reader who can open the file could already read
+   the key, and the point is that they now know it is in there. */
+function SecretBadge({ findings }: { findings?: SecretFinding[] }) {
+  if (!findings?.length) return null;
+  return (
+    <div className="sbadge" role="status">
+      <span className="sb-icon">
+        <Icon name="shield" />
+      </span>
+      <div className="sb-text">
+        <b>{secretsBadge(findings)}</b>
+        <span>
+          Checked when this page loaded. Sharing the file asks you to confirm first.
+        </span>
+      </div>
+    </div>
  );
 }

@@ -0,0 +1,43 @@
+// Run with `npm test` (node's built-in runner; node ≥ 23 strips the types).
+// Excluded from tsconfig's include — it imports node: builtins, which the
+// app's DOM-only lib set does not know about.
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { secretsBadge, secretsMessage } from "./secrets.ts";
+
+test("one finding names the rule in words and the line", () => {
+  const s = secretsBadge([{ rule: "aws_access_key_id", line: 3 }]);
+  assert.equal(s, "This file contains an AWS access key (line 3).");
+});
+
+test("several findings read as a list", () => {
+  const s = secretsBadge([
+    { rule: "aws_access_key_id", line: 3 },
+    { rule: "github_pat", line: 9 },
+    { rule: "private_key", line: 40 },
+  ]);
+  assert.equal(
+    s,
+    "This file contains an AWS access key (line 3), a GitHub token (line 9) and a private key (line 40).",
+  );
+});
+
+test("an unknown rule id falls back to the raw id rather than reporting nothing", () => {
+  assert.match(secretsBadge([{ rule: "nomad_token", line: 1 }]), /nomad_token \(line 1\)/);
+});
+
+test("the badge and the share dialog name the same finding identically", () => {
+  const f = [{ rule: "openai_api_key", line: 12 }];
+  assert.match(secretsBadge(f), /an OpenAI API key \(line 12\)/);
+  assert.match(secretsMessage(f), /an OpenAI API key \(line 12\)/);
+});
+
+test("the share dialog never claims the file stays clean", () => {
+  const s = secretsMessage([{ rule: "aws_access_key_id", line: 3 }]);
+  assert.match(s, /latest content/);
+  assert.match(s, /Share anyway\?$/);
+});
+
+test("no findings still phrases something rather than an empty sentence", () => {
+  assert.match(secretsMessage(), /something credential-shaped/);
+});
@@ -0,0 +1,53 @@
+/* The hub's six credential rules, in words — shared by the two surfaces that
+   report the same finding: the share dialog's confirm (it refuses to mint)
+   and the file view's badge (it only warns). One map, so the two wordings
+   cannot drift; that shared vocabulary is the whole reason this lives in
+   lib/ rather than beside either caller.
+
+   Mirrors `labels` in internal/secrets/secrets.go, which is what `bdrive
+   share` and `bdrive status` print. */
+
+export interface SecretFinding {
+  rule: string;
+  line: number;
+}
+
+const SECRET_LABELS: Record<string, string> = {
+  aws_access_key_id: "an AWS access key",
+  openai_api_key: "an OpenAI API key",
+  github_pat: "a GitHub token",
+  slack_token: "a Slack token",
+  private_key: "a private key",
+  gitlab_pat: "a GitLab token",
+};
+
+// A rule with no label renders as its bare id: ugly, but a seventh rule that
+// reported nothing would be worse.
+function phrase(f: SecretFinding): string {
+  return `${SECRET_LABELS[f.rule] ?? f.rule} (line ${f.line})`;
+}
+
+function list(findings: SecretFinding[]): string {
+  const parts = findings.map(phrase);
+  if (parts.length > 1) return parts.slice(0, -1).join(", ") + " and " + parts[parts.length - 1];
+  return parts[0] || "something credential-shaped";
+}
+
+// secretsMessage phrases the 409 for the confirm dialog. The second sentence
+// is not decoration: a link always serves the file's LATEST content, so the
+// copy may only ever claim what was true at the moment of sharing — never
+// that the file is clean.
+export function secretsMessage(findings: SecretFinding[] = []): string {
+  return (
+    `BearDrive found ${list(findings)} in this file. The check covers the file at the moment you share it — ` +
+    `a link always serves the file's latest content, so later changes are never checked. Share anyway?`
+  );
+}
+
+// secretsBadge phrases the same finding for the file view, where nothing is
+// blocked and nothing is redacted — a reader who can open the file could
+// already read the key. Same map, so the badge and the dialog name the rule
+// and the line identically.
+export function secretsBadge(findings: SecretFinding[] = []): string {
+  return `This file contains ${list(findings)}.`;
+}
@@ -1177,6 +1177,18 @@ input[type="checkbox"] { accent-color: var(--accent); }
 .vbanner .vb-actions { flex: none; display: flex; gap: 8px; }
 .vbanner .vb-actions .ai-btn { display: inline-flex; align-items: center; text-decoration: none; }

+/* Credential badge on the file view (BEA-147) — the share gate's finding on
+   the path every file takes. Same strip geometry as .vbanner above; the red
+   family rather than the accent, because accent+glow already means "you are
+   looking at an old version" and the two strips can appear together. The hue
+   is .meta-stale's (#e07070, style.css:385), which is the warning colour this
+   same view already speaks. */
+.sbadge { display: flex; align-items: center; gap: 12px; flex-wrap: wrap; margin: 0 0 22px; padding: 11px 14px; border: 1px solid rgba(242,109,109,.32); border-radius: var(--r-card); background: rgba(242,109,109,.08); }
+.sbadge .sb-icon { flex: none; display: flex; color: #e07070; }
+.sbadge .sb-text { flex: 1 1 220px; min-width: 0; display: flex; flex-direction: column; gap: 1px; font-size: 12.5px; line-height: 1.45; }
+.sbadge .sb-text b { color: #e07070; font-weight: 600; }
+.sbadge .sb-text span { color: var(--text-dim); }
+
 /* plain file / binary views */
 pre.plain { background: var(--code-bg); border: 1px solid var(--border); border-radius: var(--r-card); padding: 14px 16px; overflow-x: auto; font: 12.5px/1.6 var(--mono); color: #c6cbd3; white-space: pre-wrap; overflow-wrap: anywhere; }
 /* long unbreakable lines wrap instead of blowing out the column: .page has min-width: 0 */
@@ -0,0 +1,151 @@
+package webapp
+
+import (
+	"bytes"
+	"encoding/json"
+	"log"
+	"net/http"
+	"net/http/httptest"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/runbear-io/beardrive/internal/secrets"
+)
+
+// getAs fetches as a signed-in member: the render route is membership-gated,
+// and the badge is for the people who can already read the file.
+func getAs(t *testing.T, srv *Server, h http.Handler, url string) *httptest.ResponseRecorder {
+	t.Helper()
+	req := httptest.NewRequest("GET", url, nil)
+	authAs(t, srv, req)
+	return doHTTP(h, req)
+}
+
+func renderDoc(t *testing.T, srv *Server, h http.Handler, url string) (struct {
+	HTML     string            `json:"html"`
+	Findings []secrets.Finding `json:"findings"`
+}, *httptest.ResponseRecorder) {
+	t.Helper()
+	var doc struct {
+		HTML     string            `json:"html"`
+		Findings []secrets.Finding `json:"findings"`
+	}
+	rec := getAs(t, srv, h, url)
+	if rec.Code != 200 {
+		t.Fatalf("render %s: %d %s", url, rec.Code, rec.Body)
+	}
+	if err := json.Unmarshal(rec.Body.Bytes(), &doc); err != nil {
+		t.Fatalf("decode render body %q: %v", rec.Body, err)
+	}
+	return doc, rec
+}
+
+// TestRenderSecretFindings: the scan that refuses to publish the file also
+// runs on the path every file takes, so the viewer can say what the share
+// dialog would say (BEA-147).
+func TestRenderSecretFindings(t *testing.T) {
+	srv, p, _, f, h := shareHub(t)
+	f.put("dev1", "deploy.md", "# Deploy\n\nexport AWS_ACCESS_KEY_ID="+planted+"\n")
+	f.put("dev1", "clean.md", "# Clean\n\nnothing to see\n")
+	base := "/api/p/" + p.ID + "/"
+
+	doc, _ := renderDoc(t, srv, h, base+"render?path=deploy.md")
+	want := []secrets.Finding{{Rule: "aws_access_key_id", Line: 3}}
+	if !reflect.DeepEqual(doc.Findings, want) {
+		t.Fatalf("findings = %+v, want %+v", doc.Findings, want)
+	}
+	// Advisory, not a redaction: the file still renders in full.
+	if !strings.Contains(doc.HTML, "Deploy") {
+		t.Fatalf("the render was suppressed: %q", doc.HTML)
+	}
+
+	// A clean file carries no field at all — omitted, not empty, so the
+	// frontend's check is a plain truthiness test.
+	doc, rec := renderDoc(t, srv, h, base+"render?path=clean.md")
+	if len(doc.Findings) != 0 {
+		t.Fatalf("clean file has findings: %+v", doc.Findings)
+	}
+	if strings.Contains(rec.Body.String(), "findings") {
+		t.Fatalf("clean render sent an empty findings field: %s", rec.Body)
+	}
+}
+
+// TestRenderSecretNeverEchoed mirrors TestShareSecretNeverEchoed for the new
+// caller: rule ids and line numbers only, never the matched text.
+func TestRenderSecretNeverEchoed(t *testing.T) {
+	srv, p, _, f, h := shareHub(t)
+	f.put("dev1", "creds.md", "key = "+planted+"\n")
+
+	var logs bytes.Buffer
+	prev := log.Writer()
+	log.SetOutput(&logs)
+	t.Cleanup(func() { log.SetOutput(prev) })
+
+	rec := getAs(t, srv, h, "/api/p/"+p.ID+"/render?path=creds.md")
+	if rec.Code != 200 {
+		t.Fatalf("render: %d %s", rec.Code, rec.Body)
+	}
+	// The rendered HTML is the file, so the key is in the body by design —
+	// what must never appear is a SECOND copy carried by the finding.
+	var doc struct {
+		HTML     string          `json:"html"`
+		Findings json.RawMessage `json:"findings"`
+	}
+	if err := json.Unmarshal(rec.Body.Bytes(), &doc); err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(string(doc.Findings), planted) {
+		t.Errorf("the finding echoed the secret: %s", doc.Findings)
+	}
+	if strings.Contains(logs.String(), planted) {
+		t.Errorf("the secret reached the log: %s", logs.String())
+	}
+}
+
+// TestRenderSecretScanLimit: the 1 MiB boundary is a decision, pinned for
+// minting in shares_test.go and pinned here so the two surfaces cannot
+// disagree about the same file.
+func TestRenderSecretScanLimit(t *testing.T) {
+	srv, p, _, f, h := shareHub(t)
+	f.put("dev1", "big.md", strings.Repeat("filler\n", secrets.ScanLimit/7+1)+planted+"\n")
+	base := "/api/p/" + p.ID + "/"
+
+	doc, _ := renderDoc(t, srv, h, base+"render?path=big.md")
+	if len(doc.Findings) != 0 {
+		t.Fatalf("a key past the scan limit was badged: %+v", doc.Findings)
+	}
+}
+
+// TestRenderVersionSecretFindings: clicking into history on the file the
+// badge was warning about must not make the warning disappear.
+func TestRenderVersionSecretFindings(t *testing.T) {
+	srv, p, _, f, h := shareHub(t)
+	f.put("dev1", "deploy.md", "# Deploy\n\nexport AWS_ACCESS_KEY_ID="+planted+"\n")
+	f.put("dev1", "deploy.md", "# Deploy\n\ncleaned up\n")
+	base := "/api/p/" + p.ID + "/"
+
+	rec := getAs(t, srv, h, base+"history?path=deploy.md")
+	var hist struct {
+		Entries []HistoryEntry `json:"entries"`
+	}
+	if err := json.Unmarshal(rec.Body.Bytes(), &hist); err != nil {
+		t.Fatal(err)
+	}
+	if len(hist.Entries) < 2 {
+		t.Fatalf("history has %d entries, want the pre-cleanup version too", len(hist.Entries))
+	}
+	old := hist.Entries[len(hist.Entries)-1].Blob
+
+	// Current version is clean...
+	doc, _ := renderDoc(t, srv, h, base+"render?path=deploy.md")
+	if len(doc.Findings) != 0 {
+		t.Fatalf("cleaned-up file has findings: %+v", doc.Findings)
+	}
+	// ...the version that held the key still says so.
+	doc, _ = renderDoc(t, srv, h, base+"render?path=deploy.md&sha="+old)
+	want := []secrets.Finding{{Rule: "aws_access_key_id", Line: 3}}
+	if !reflect.DeepEqual(doc.Findings, want) {
+		t.Fatalf("version findings = %+v, want %+v", doc.Findings, want)
+	}
+}
@@ -43,6 +43,7 @@ import (

 	"github.com/runbear-io/beardrive/internal/journal"
 	"github.com/runbear-io/beardrive/internal/remote"
+	"github.com/runbear-io/beardrive/internal/secrets"
 	"github.com/runbear-io/beardrive/internal/templates"
 )

@@ -1413,9 +1414,33 @@ func (s *Server) handleRender(v *volume, w http.ResponseWriter, r *http.Request)
 	if fi.UserName != "" {
 		doc["user_name"] = fi.UserName
 	}
+	if f := renderFindings(src); len(f) > 0 {
+		doc["findings"] = f
+	}
 	writeJSON(w, doc)
 }

+// renderFindings is the share gate's credential scan on the path every file
+// takes. The gate could name the rule and the line well enough to refuse to
+// publish a file while the viewer rendered the same key as ordinary prose
+// (BEA-147), so the render response carries the finding too — advisory only,
+// nothing is blocked and nothing is redacted, since a member who can open the
+// file could already read the key.
+//
+// Rule ids and line numbers only. The matched text must never reach a
+// response body; see the doc comment on secrets.Scan.
+//
+// The cap is a slice rather than the LimitReader the two streaming callers
+// use: the render path already holds the whole file, because that is what
+// RenderMarkdown needs. Same ScanLimit either way, so the badge and the share
+// dialog can never disagree about the same file.
+func renderFindings(src []byte) []secrets.Finding {
+	if len(src) > secrets.ScanLimit {
+		src = src[:secrets.ScanLimit]
+	}
+	return secrets.Scan(src)
+}
+
 // renderVersion renders one exact past version by content hash — the
 // markdown counterpart of /blob?sha=, so opening an old .md from history
 // shows a rendered page instead of raw source. Provenance is not returned:
@@ -1446,9 +1471,16 @@ func (s *Server) renderVersion(v *volume, w http.ResponseWriter, r *http.Request
 		http.Error(w, fmt.Sprintf("render: %v", err), http.StatusInternalServerError)
 		return
 	}
-	writeJSON(w, map[string]any{
+	doc := map[string]any{
 		"path": r.URL.Query().Get("path"), "html": html, "size": len(src),
-	})
+	}
+	// The history view goes through this same endpoint, so scanning here too
+	// is what stops the badge vanishing the moment you click into history on
+	// the very file it was warning about.
+	if f := renderFindings(src); len(f) > 0 {
+		doc["findings"] = f
+	}
+	writeJSON(w, doc)
 }

 // inlineMarkup reports whether a Content-Type names something the browser
@@ -5,10 +5,10 @@
 <meta name="viewport" content="width=device-width, initial-scale=1">
 <title>BearDrive</title>
 <link rel="icon" href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32' fill='%23f5a623'><rect x='4' y='4' width='5.6' height='24'/><rect x='11.2' y='4' width='14.4' height='11.2'/><rect x='11.2' y='16.8' width='16.8' height='11.2'/></svg>">
-  <script type="module" crossorigin src="/assets/index-BysAiMHJ.js"></script>
+  <script type="module" crossorigin src="/assets/index-C17b7d2I.js"></script>
  <link rel="modulepreload" crossorigin href="/assets/_commonjsHelpers-CqkleIqs.js">
  <link rel="modulepreload" crossorigin href="/assets/mermaid-DQuCJ8Gi.js">
-  <link rel="stylesheet" crossorigin href="/assets/index-Bg9aVdbo.css">
+  <link rel="stylesheet" crossorigin href="/assets/index-DISTZ6FW.css">
 </head>
 <body>
 <div id="root"></div>