From 36cf59a3c3ba5fb3106bab6d8fb878f4adba7030 Mon Sep 17 00:00:00 2001 From: Ben Firestone Date: Fri, 1 May 2026 12:44:32 -0700 Subject: [PATCH] fix(arc-paste): default-deny via allowlist + scrub stale dual-URL docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two findings from the PR #47 adversarial review: 1. arc-paste binary now genuinely mirrors the Caddy edge. The previous guards only blocked / and /api/v1/* — every other path (e.g. /labels, /dashboard, //issues) still fell through the SPA wildcard in web.RegisterSPA and served the arc app shell. Replaced with a single allowlist middleware (arcPasteAllowedPath) that mirrors the Caddyfile @assets + @share matchers exactly: /_app/*, /api/paste, /api/paste/*, /robots.txt, and /share/. Tests cover both the rejected set (including shape edges like /share/, /share/a/b, /api/pasteX, /robots.txt.bak) and the allowed set, plus a direct table-driven test of the predicate. 2. Documentation drift: review_howto.md still described the deprecated dual-URL CLI output, and paste-server.md had four lingering "Share URL" mentions that didn't match the new in-page Share-link button flow. Rewrote review_howto.md's "URLs you'll receive" section around the Author URL / reviewer URL split, and replaced the paste-server.md mentions with consistent "reviewer URL" terminology matching the rest of the runbook. The /api/v1/* JSON-404 carve-out is gone — Caddy returns text/plain at the edge, and the binary now matches. The arc SPA shell that motivated the JSON 404 doesn't load anymore (/ is 404), so nothing parses the body. --- arc-paste/main.go | 52 +++++++++++---- arc-paste/main_test.go | 120 ++++++++++++++++++++++++++-------- docs/runbooks/paste-server.md | 8 +-- docs/runbooks/review_howto.md | 12 ++-- 4 files changed, 141 insertions(+), 51 deletions(-) diff --git a/arc-paste/main.go b/arc-paste/main.go index 63b3cba..32387dd 100644 --- a/arc-paste/main.go +++ b/arc-paste/main.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" @@ -60,7 +61,7 @@ func run() error { } // newRouter wires the arc-paste HTTP surface. Extracted so tests can exercise -// route precedence (/, /api/v1/*, SPA fallback) without binding a real listener. +// the allowlist without binding a real listener. func newRouter(handlers *paste.Handlers) *echo.Echo { e := echo.New() e.HideBanner = true @@ -77,18 +78,19 @@ func newRouter(handlers *paste.Handlers) *echo.Echo { e.Use(middleware.Recover()) e.Use(middleware.CORS()) - // arc-paste only owns /api/paste/*. The arc SPA shell probes /api/v1/* on - // boot; without this guard those probes fall through to the SPA fallback - // and return HTML, which the browser then fails to JSON.parse. - e.Any("/api/v1/*", func(c echo.Context) error { - return c.JSON(http.StatusNotFound, map[string]string{"error": "not found"}) - }) - - // Paste deployments don't serve the arc app shell — the only useful - // destination is /share/. Mirrors the Caddy edge config so dev and - // prod behave the same. - e.Any("/", func(c echo.Context) error { - return c.String(http.StatusNotFound, "Not found") + // Default-deny everything outside the share surface. This mirrors the + // Caddyfile allowlist (arc-paste/Caddyfile) exactly so dev (no Caddy) + // and prod behave the same — without this, the SPA wildcard registered + // by web.RegisterSPA would happily serve the arc app shell at /labels, + // /dashboard, //issues, etc., even though arc-paste deploys + // have no use for any of those routes. + e.Use(func(next echo.HandlerFunc) echo.HandlerFunc { + return func(c echo.Context) error { + if !arcPasteAllowedPath(c.Request().URL.Path) { + return c.String(http.StatusNotFound, "Not found") + } + return next(c) + } }) handlers.Register(e.Group("/api/paste")) @@ -96,6 +98,30 @@ func newRouter(handlers *paste.Handlers) *echo.Echo { return e } +// arcPasteAllowedPath mirrors the Caddyfile allowlist: +// +// /_app/* SvelteKit hashed asset bundle +// /api/paste paste API (create) +// /api/paste/* paste API (per-share routes) +// /robots.txt robots +// /share/ share page (exactly one segment after /share/) +// +// Anything else is rejected with the same 404 Caddy returns at the edge. +// Keep this function in sync with arc-paste/Caddyfile if either changes. +func arcPasteAllowedPath(p string) bool { + switch p { + case "/api/paste", "/robots.txt": + return true + } + if strings.HasPrefix(p, "/_app/") || strings.HasPrefix(p, "/api/paste/") { + return true + } + if rest, ok := strings.CutPrefix(p, "/share/"); ok { + return rest != "" && !strings.Contains(rest, "/") + } + return false +} + // envOr returns the value of env variable key, or defaultVal if not set. func envOr(key, defaultVal string) string { if v, ok := os.LookupEnv(key); ok { diff --git a/arc-paste/main_test.go b/arc-paste/main_test.go index 6aeabc4..0130c90 100644 --- a/arc-paste/main_test.go +++ b/arc-paste/main_test.go @@ -7,7 +7,6 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "strings" "testing" "github.com/sentiolabs/arc/internal/paste" @@ -44,37 +43,102 @@ func TestArcPasteCreate(t *testing.T) { } } -// arc-paste only owns /api/paste/*. The arc SPA shell calls /api/v1/projects -// on boot; without an explicit guard those requests fall through to the SPA -// fallback and return HTML, which the browser fails to JSON.parse. Return a -// clean JSON 404 so the SPA gets a parseable error instead of a crash. -func TestArcPasteRejectsArcAPIProbes(t *testing.T) { - e := newTestRouter(t) - for _, path := range []string{"/api/v1/projects", "/api/v1/workspaces", "/api/v1/anything/nested"} { - rec := httptest.NewRecorder() - e.ServeHTTP(rec, httptest.NewRequest("GET", path, nil)) - if rec.Code != http.StatusNotFound { - t.Errorf("%s: expected 404, got %d", path, rec.Code) +// arc-paste deployments only legitimately serve the share surface. Anything +// outside the allowlist (paths the arc SPA shell normally owns — +// /api/v1/projects on boot, /labels, //issues, /dashboard, /, +// etc.) must 404. Without this, the SPA wildcard registered by +// web.RegisterSPA would happily return the arc app shell HTML for those +// paths, which is both confusing and (for /api/v1/* boot probes) actively +// breaks the SPA because it can't JSON.parse HTML. The test mirrors the +// Caddyfile allowlist exactly so the in-binary guard and the edge stay +// in lockstep. +func TestArcPasteAllowlist(t *testing.T) { + t.Run("rejected paths 404", func(t *testing.T) { + e := newTestRouter(t) + // A representative slice — not exhaustive. Includes the arc SPA boot + // probes and a sampling of the routes the arc app shell normally owns, + // plus shapes that look superficially like /share/ but aren't. + rejected := []string{ + "/", + "/labels", + "/dashboard", + "/teams", + "/api/v1/projects", + "/api/v1/workspaces", + "/api/v1/anything/nested", + "/share", // missing id + "/share/", // empty id + "/share/abc/sub", // multi-segment after /share/ + "/_app", // bare /_app (no trailing slash) is not the asset prefix + "/api/pasteX", // not /api/paste or /api/paste/ + "/robots.txt.bak", // not exactly /robots.txt } - if ct := rec.Header().Get("Content-Type"); !strings.Contains(ct, "application/json") { - t.Errorf("%s: expected JSON content-type, got %q", path, ct) + for _, path := range rejected { + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest("GET", path, nil)) + if rec.Code != http.StatusNotFound { + t.Errorf("%s: expected 404, got %d (body=%q)", path, rec.Code, rec.Body.String()) + } } - var body map[string]string - if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { - t.Errorf("%s: body is not JSON: %v (%q)", path, err, rec.Body.String()) + }) + + t.Run("allowed paths reach handlers", func(t *testing.T) { + e := newTestRouter(t) + // /api/paste create — the existing happy path. Round-trips a real + // CreatePasteRequest so we know the allowlist didn't accidentally + // shadow the handler. + body, _ := json.Marshal(map[string]any{ + "plan_blob": []byte{1, 2, 3}, + "plan_iv": []byte{4, 5, 6}, + "schema_ver": 1, + }) + req := httptest.NewRequest("POST", "/api/paste", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + if rec.Code != http.StatusCreated { + t.Errorf("/api/paste: expected 201 from handler, got %d (body=%q)", rec.Code, rec.Body.String()) } - } + + // Allowlisted GET paths must NOT 404. We don't assert 200 because the + // embedded SPA may not be present in CLI-only test builds — what + // matters is that the allowlist doesn't reject them. + for _, path := range []string{"/share/abc123", "/_app/anything", "/robots.txt"} { + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest("GET", path, nil)) + if rec.Code == http.StatusNotFound && rec.Body.String() == "Not found" { + t.Errorf("%s: rejected by allowlist (got 404 with allowlist body), want pass-through to handler", path) + } + } + }) } -// arc-paste deployments don't serve the arc app shell — the only useful -// destination is /share/. Returning 404 at the root mirrors the Caddy -// edge configuration so dev (localhost) and prod (arcpaste.company.com) -// behave identically. -func TestArcPasteRootIs404(t *testing.T) { - e := newTestRouter(t) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil)) - if rec.Code != http.StatusNotFound { - t.Fatalf("expected 404 at /, got %d (body=%q)", rec.Code, rec.Body.String()) +// Sanity-check the allowlist predicate directly so a regression in shape +// matching is obvious without spinning up the full router. +func TestArcPasteAllowedPath(t *testing.T) { + cases := []struct { + path string + want bool + }{ + {"/api/paste", true}, + {"/api/paste/abc", true}, + {"/api/paste/abc/blobs", true}, + {"/_app/foo.js", true}, + {"/_app/immutable/chunk.abc.js", true}, + {"/robots.txt", true}, + {"/share/abc123", true}, + {"/", false}, + {"/labels", false}, + {"/share", false}, + {"/share/", false}, + {"/share/abc/sub", false}, + {"/_app", false}, + {"/api/pasteX", false}, + {"/api/v1/projects", false}, + } + for _, tc := range cases { + if got := arcPasteAllowedPath(tc.path); got != tc.want { + t.Errorf("arcPasteAllowedPath(%q) = %v, want %v", tc.path, got, tc.want) + } } } diff --git a/docs/runbooks/paste-server.md b/docs/runbooks/paste-server.md index 18a3d16..6d41d8d 100644 --- a/docs/runbooks/paste-server.md +++ b/docs/runbooks/paste-server.md @@ -98,7 +98,7 @@ Open the **Author URL** (the one with `&t=`) in Chrome/Firefox. 3. **Pick a label** (`praise` / `issue` / `suggestion` / `question` / `nit`). 4. **Type a comment**, optionally toggle "Suggest replacement text". 5. **Post** — the first time you do this, a small modal asks for your name. (As the author you should already have a name from step 1; the modal won't fire.) The comment appears in the sidebar. -6. As the author, you'll see **Accept / Resolve / Reject** controls on every comment. Try Accept on one, Reject (with reply) on another, and Resolve on a third. If the controls don't appear, you opened the Share URL (no `&t=`) instead of the Author URL — close the tab and reopen via the Author URL. +6. As the author, you'll see **Accept / Resolve / Reject** controls on every comment. Try Accept on one, Reject (with reply) on another, and Resolve on a third. If the controls don't appear, you opened a reviewer URL (no `&t=`) instead of the Author URL — close the tab and reopen via the Author URL. 7. As a reviewer (a fresh browser profile or incognito window with NO `&t=` in the URL), find your own annotation in the sidebar. You should see an **✎ Edit** button on it. Click it; the body becomes a textarea pre-filled with your existing comment. Refine the wording — e.g. expand "expand this more" into a fully-formed suggestion — and **⌘/Ctrl-⏎** (or click Save). The card re-renders with `· edited Nm` next to the timestamp. Confirm `arc share comments ` prints the new body, not the original. 8. Switch back to the author window. The **✎ Edit** button should also appear on every reviewer's annotation (not just your own). Use it to sharpen a thin reviewer comment — the displayed `author_name` stays as the reviewer, only the body changes. Run `arc share comments ` again and confirm the refined body shows up. 9. **Click the chip** in the header — the name prompt opens prefilled with your current name (text selected). Edit and save → chip updates. This is the rename affordance. @@ -113,7 +113,7 @@ Open the **Author URL** (the one with `&t=`) in Chrome/Firefox. ### Simulate a second reviewer -Without spinning up another machine, open the **Share URL (no `&t=`)** in an **incognito window** (or a different browser entirely). Incognito gets a fresh `localStorage`, so: +Without spinning up another machine, click the in-page **Share link** button (in the page header on the author's tab) to copy the reviewer URL (`#k=…` only, no `&t=`), then open that URL in an **incognito window** (or a different browser entirely). Incognito gets a fresh `localStorage`, so: 1. **No chip** appears in the header. There's no "Sign in" button anywhere — identity is captured lazily. 2. Highlight a paragraph and click Comment in the toolbar. A name prompt fires; type any name (e.g. "Reviewer-2"). The comment posts; the chip now reads `Reviewer-2` (no `· author`, because the URL has no `&t=`). @@ -131,7 +131,7 @@ Walk these in order to confirm the new auth UX end-to-end: 3. **Author URL flow.** Open the Preview URL in a fresh browser profile. Header chip shows ` · author` immediately, no modal. Accept / Resolve buttons visible on existing comments. 4. **Reviewer URL flow.** From the author's browser tab, click the **Share link** button in the page header — clipboard now holds the bare reviewer URL (`#k=…` only, no `&t=`). Open that copied URL in a different fresh profile. No chip in the header, no "Sign in" button. Select text → click Comment in the toolbar → name modal opens → save a name → comment posts → chip now shows the name. 5. **Rename via chip.** Click the chip on the reviewer side. Modal opens prefilled with the saved name (text selected). Edit, save → chip updates. -6. **Author opens the bare share URL.** Open the share URL (without `&t=`) on the author's browser. They are now in reviewer mode (no Accept buttons). Reopen via the Author URL → author mode restored. +6. **Author opens the bare reviewer URL.** Click the in-page **Share link** button to copy the reviewer URL (`#k=…` only), then paste it into a new tab in the same browser. They are now in reviewer mode (no Accept buttons). Reopen via the Author URL → author mode restored. ## 3. Shared / remote review @@ -224,7 +224,7 @@ Step 7 (review loop) uses `arc share approve` and `arc share pull` instead of th | Symptom | Cause | Fix | |---|---|---| -| Accept / Resolve / Reject controls never appear | You opened the Share URL (no `&t=`) instead of the Author URL — author detection is token-based, not name-based. Or the share was created without an author name (`git config user.name` was empty and no `--author` flag passed), so `plan.author_name` is empty and the chip never shows `· author` | Close the tab and reopen via the Author URL (run `arc share show --author-url` to retrieve it). If the share has no author name, recreate it with `--author "Your Name"` | +| Accept / Resolve / Reject controls never appear | You opened a reviewer URL (no `&t=`) instead of the Author URL — author detection is token-based, not name-based. Or the share was created without an author name (`git config user.name` was empty and no `--author` flag passed), so `plan.author_name` is empty and the chip never shows `· author` | Close the tab and reopen via the Author URL (run `arc share show --author-url` to retrieve it). If the share has no author name, recreate it with `--author "Your Name"` | | `/share/` returns `{"message":"Not Found"}` (Echo's default 404 JSON) | Binary built without the `webui` build tag — `web.RegisterSPA` is the no-op stub | Rebuild with `make build` (not `make build-quick`); for arc-paste use `go build -tags webui -o ./bin/arc-paste ./arc-paste` | | `/share/` returns blank HTML / cannot find static assets | `web/build/` not present at compile time, even with the `webui` tag | Re-run `bun run build` in `web/`, then rebuild the binary | | SPA console says `missing #k= in URL` | URL was pasted without its fragment | Use the full URL printed by `arc share create` — fragments are dropped by some chat apps; copy carefully | diff --git a/docs/runbooks/review_howto.md b/docs/runbooks/review_howto.md index d6fec26..5774746 100644 --- a/docs/runbooks/review_howto.md +++ b/docs/runbooks/review_howto.md @@ -21,14 +21,14 @@ The discriminator is whether the comment should *cause an edit downstream*. Acce ## URLs you'll receive -`arc share create` prints two URLs: +`arc share create` prints exactly one URL — the **Author URL** (or **Preview URL** for `--local` shares). It contains both the decryption key and the `&t=` that grants Accept / Resolve / Reject. Treat it like a write password: don't paste it into tickets, screenshots, or shared chat threads. The first time you open it, the page detects your role from the URL itself; you don't sign in or pick a name. -| URL | What it is | Who gets it | -|---|---|---| -| **Share URL** (`#k=…`) | Read + comment | Reviewers | -| **Author URL** (`#k=…&t=…`) | Adds Accept / Resolve / Reject | Plan author only | +| URL form | What it is | Who gets it | Source | +|---|---|---|---| +| **Author URL** (`#k=…&t=…`) | Read + comment + Accept / Resolve / Reject | Plan author only | Printed by `arc share create` | +| **Reviewer URL** (`#k=…` only, no `&t=`) | Read + comment | Reviewers | Click the in-page **Share link** button on the share page header | -The author URL is a strict superset of the share URL — anyone given the author URL can also read and comment. Treat it like a write password: don't paste it into tickets, screenshots, or shared chat threads. The first time you open the author URL, the page detects your role from the URL itself; you don't sign in or pick a name. +To send a reviewer link: open the Author URL in your browser, click the **Share link** button in the page header, and paste the resulting URL into your message. The button strips `&t=` so the URL you share grants reviewer-only access — copy-pasting the bare Author URL would hand the recipient your edit token. Lost the URL? Run `arc share show --author-url` to reprint it (uses the `edit_token` saved to `~/.arc/shares.json`).