Files
multica/server/internal/handler/attachment_capability_test.go
Bohan Jiang 2187b58292 feat(attachments): let callers opt out of pre-signed URLs in bulk responses (MUL-5372) (#6119)
* feat(attachments): let callers opt out of pre-signed URLs in bulk responses

A CloudFront-signed download_url is ~800 chars, ~630 of which are a
Policy+Signature pair re-minted on every request (the policy embeds
now+TTL at second granularity). Emitting one per attachment on every
list response costs three ways: raw payload, a fresh RSA sign per
attachment per request, and — because the bytes differ on every read —
it defeats any cache keyed on response content.

Agents pay all three and use none of it. `multica attachment download
<id>` fetches a fresh signature from the single-attachment endpoint, so
the id is the only part the CLI needs; measured on a 15-attachment
issue, the URL fields were 28.3% of the payload and ~10k chars of that
rotated on every read.

Callers now advertise `stable_attachment_urls` in X-Client-Capabilities
to receive the stable /api/attachments/{id}/download path instead. That
endpoint re-signs and 302s on every hit, so the value stays correct
forever at ~95 chars. The CLI advertises it unconditionally — it is a
protocol detail, not a user-visible flag.

The single-attachment endpoint keeps signing regardless of the
capability: it is the one source of fresh, natively-loadable URLs, and
stable-mode callers exchange a stable path for a signature there. That
carve-out is what makes the rest safe.

The server default never moves. A caller that advertises nothing — every
installed mobile build, every third-party script — gets byte-identical
responses, so this ships ahead of any client and each surface migrates
on its own release cadence. Web/desktop (Phase 2) and mobile (Phase 3)
are deliberately not touched here.

MUL-5372

Co-authored-by: multica-agent <github@multica.ai>

* test(attachments): pin the CLI capability header and the rotation premise

Two review nits from #6119.

The CLI's setHeaders call is the only place Phase 1 is switched on, and
nothing observed it: the server tests prove a request carrying
`X-Client-Capabilities: stable_attachment_urls` gets stable paths, but a
typo in the CLI token — or dropping the header — would have left every
test green while the CLI silently went back to ~800-char signed URLs.
Assert the header verbatim across GET, GET-with-headers, POST, and the
no-token client. Verified the assertion is load-bearing by mutating the
constant to `stable_attachment_url`: the test fails.

TestAttachmentToResponse_SignedModeRotatesButStableDoesNot only checked
the "stable does not" half, so its name overstated coverage. Add the
rotation half. It drives the signer with two explicit expiries rather
than calling attachmentToResponse twice: that function mints its expiry
from time.Now() at second granularity, so consecutive calls usually land
in the same second and asserting on them directly would be a flaky test
of a real property. Also pins that only the query rotates, and that a
fixed expiry re-signs deterministically — without which the rotation
assertion would prove nothing about the clock.

MUL-5372

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: Bohan-J <bohan@devv.ai>
Co-authored-by: multica-agent <github@multica.ai>
2026-07-29 18:25:14 +08:00

328 lines
12 KiB
Go

package handler
import (
"bytes"
"context"
"crypto/sha256"
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"strconv"
"strings"
"testing"
"time"
"github.com/go-chi/chi/v5"
"github.com/multica-ai/multica/server/internal/auth"
)
const capabilityTestAttachmentID = "11111111-2222-3333-4444-555555555555"
// capabilityQuery returns just the query string of a freshly minted
// capability, so tests can vary one field at a time.
func capabilityQuery(t *testing.T, attachmentID string, now time.Time) url.Values {
t.Helper()
path := attachmentCapabilityPath(attachmentID, now)
idx := strings.Index(path, "?")
if idx < 0 {
t.Fatalf("capability path has no query: %q", path)
}
values, err := url.ParseQuery(path[idx+1:])
if err != nil {
t.Fatalf("parse capability query: %v", err)
}
return values
}
// newCapabilityRequest deliberately sets NO authentication headers. The whole
// point of the capability route is that it works for a native download that
// carries neither a Bearer token nor a session cookie.
func newCapabilityRequest(attachmentID string, query url.Values) (*http.Request, *httptest.ResponseRecorder) {
req := httptest.NewRequest("GET", "/api/attachments/"+attachmentID+"/signed-download?"+query.Encode(), nil)
rctx := chi.NewRouteContext()
rctx.URLParams.Add("id", attachmentID)
req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
return req, httptest.NewRecorder()
}
// ---------------------------------------------------------------------------
// Signature unit tests (no DB)
// ---------------------------------------------------------------------------
func TestAttachmentCapability_RoundTrips(t *testing.T) {
now := time.Unix(1_800_000_000, 0)
query := capabilityQuery(t, capabilityTestAttachmentID, now)
if !verifyAttachmentCapability(capabilityTestAttachmentID, query.Get("exp"), query.Get("sig"), now) {
t.Fatal("freshly minted capability did not verify")
}
// Still valid one second before the TTL elapses.
if !verifyAttachmentCapability(
capabilityTestAttachmentID, query.Get("exp"), query.Get("sig"),
now.Add(attachmentCapabilityTTL-time.Second),
) {
t.Fatal("capability expired before its TTL elapsed")
}
}
func TestAttachmentCapability_FailsClosed(t *testing.T) {
now := time.Unix(1_800_000_000, 0)
valid := capabilityQuery(t, capabilityTestAttachmentID, now)
// A capability minted for a DIFFERENT attachment must not verify against
// this one — the id is inside the signed message, not just the URL.
other := capabilityQuery(t, "99999999-8888-7777-6666-555555555555", now)
tampered := []byte(valid.Get("sig"))
if tampered[0] == 'a' {
tampered[0] = 'b'
} else {
tampered[0] = 'a'
}
cases := []struct {
name string
id string
exp string
sig string
now time.Time
expected bool
}{
{"valid", capabilityTestAttachmentID, valid.Get("exp"), valid.Get("sig"), now, true},
{"expired", capabilityTestAttachmentID, valid.Get("exp"), valid.Get("sig"), now.Add(attachmentCapabilityTTL + time.Second), false},
{"tampered signature", capabilityTestAttachmentID, valid.Get("exp"), string(tampered), now, false},
{"extended expiry", capabilityTestAttachmentID, strconv.FormatInt(now.Add(24*time.Hour).Unix(), 10), valid.Get("sig"), now, false},
{"signature for another attachment", capabilityTestAttachmentID, other.Get("exp"), other.Get("sig"), now, false},
{"missing signature", capabilityTestAttachmentID, valid.Get("exp"), "", now, false},
{"missing expiry", capabilityTestAttachmentID, "", valid.Get("sig"), now, false},
{"missing id", "", valid.Get("exp"), valid.Get("sig"), now, false},
{"non-numeric expiry", capabilityTestAttachmentID, "not-a-number", valid.Get("sig"), now, false},
{"non-hex signature", capabilityTestAttachmentID, valid.Get("exp"), "zzzz", now, false},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := verifyAttachmentCapability(tc.id, tc.exp, tc.sig, tc.now); got != tc.expected {
t.Fatalf("verify = %v, want %v", got, tc.expected)
}
})
}
}
// The capability key must not be the JWT secret, nor a bare hash of it. If the
// two signing domains ever shared a key, a capability signature and a session
// signature would be interchangeable.
func TestAttachmentCapability_KeyIsDomainSeparatedFromJWTSecret(t *testing.T) {
key := attachmentCapabilitySigningKey()
if bytes.Equal(key, auth.JWTSecret()) {
t.Fatal("capability key equals the raw JWT secret")
}
bare := sha256.Sum256(auth.JWTSecret())
if bytes.Equal(key, bare[:]) {
t.Fatal("capability key is an undomained hash of the JWT secret")
}
}
// ---------------------------------------------------------------------------
// Route tests
// ---------------------------------------------------------------------------
// installProxyModeStorage puts the handler into proxy mode with an in-memory
// backend, which is the deployment shape (local disk / private object host)
// where capabilities are minted.
func installProxyModeStorage(t *testing.T) *mockStorage {
t.Helper()
store := &mockStorage{}
origStorage := testHandler.Storage
origCfg := testHandler.cfg
origSigner := testHandler.CFSigner
testHandler.Storage = store
testHandler.cfg.AttachmentDownloadMode = "proxy"
testHandler.CFSigner = nil
t.Cleanup(func() {
testHandler.Storage = origStorage
testHandler.cfg = origCfg
testHandler.CFSigner = origSigner
})
return store
}
func TestDownloadAttachmentWithCapability_ServesWithoutAuthentication(t *testing.T) {
store := installProxyModeStorage(t)
body := []byte("quarterly numbers, do not leak")
id := seedPreviewAttachment(t, store, "downloads/report.pdf", "Q3 report.pdf", "application/pdf", body)
req, w := newCapabilityRequest(id, capabilityQuery(t, id, time.Now()))
testHandler.DownloadAttachmentWithCapability(w, req)
if w.Code != http.StatusOK {
t.Fatalf("status = %d, want 200; body=%s", w.Code, w.Body.String())
}
if !bytes.Equal(w.Body.Bytes(), body) {
t.Fatalf("body = %q, want %q", w.Body.String(), body)
}
// The original filename must survive to the save dialog — that is half
// of what the bug report asked for.
if disposition := w.Header().Get("Content-Disposition"); !strings.Contains(disposition, "Q3 report.pdf") {
t.Fatalf("Content-Disposition = %q, want the original filename", disposition)
}
if got := w.Header().Get("Referrer-Policy"); got != "no-referrer" {
t.Fatalf("Referrer-Policy = %q, want no-referrer", got)
}
if got := w.Header().Get("Cache-Control"); got != "no-store" {
t.Fatalf("Cache-Control = %q, want no-store", got)
}
}
func TestDownloadAttachmentWithCapability_RejectsInvalidCapabilities(t *testing.T) {
store := installProxyModeStorage(t)
body := []byte("secret bytes")
id := seedPreviewAttachment(t, store, "downloads/secret.txt", "secret.txt", "text/plain", body)
expired := capabilityQuery(t, id, time.Now().Add(-2*attachmentCapabilityTTL))
forged := capabilityQuery(t, id, time.Now())
forged.Set("sig", strings.Repeat("0", len(forged.Get("sig"))))
// A capability legitimately minted for a different attachment must not
// unlock this one.
otherID := seedPreviewAttachment(t, store, "downloads/other.txt", "other.txt", "text/plain", []byte("other"))
crossed := capabilityQuery(t, otherID, time.Now())
cases := []struct {
name string
query url.Values
}{
{"expired", expired},
{"forged signature", forged},
{"capability for another attachment", crossed},
{"no capability at all", url.Values{}},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
req, w := newCapabilityRequest(id, tc.query)
testHandler.DownloadAttachmentWithCapability(w, req)
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body=%s", w.Code, w.Body.String())
}
if bytes.Contains(w.Body.Bytes(), body) {
t.Fatal("rejected response leaked the attachment body")
}
})
}
}
// Range support is what makes an interrupted download resumable (RAS-29). The
// capability route reuses proxyAttachmentDownload, so it must not regress.
func TestDownloadAttachmentWithCapability_PreservesRangeAndHeaders(t *testing.T) {
store := installProxyModeStorage(t)
body := bytes.Repeat([]byte("abcdefgh"), 512) // 4 KiB
id := seedPreviewAttachment(t, store, "downloads/big.bin", "big.bin", "application/octet-stream", body)
req, w := newCapabilityRequest(id, capabilityQuery(t, id, time.Now()))
req.Header.Set("Range", "bytes=100-199")
testHandler.DownloadAttachmentWithCapability(w, req)
if w.Code != http.StatusPartialContent {
t.Fatalf("status = %d, want 206; body len=%d", w.Code, w.Body.Len())
}
if got := w.Header().Get("Content-Range"); got != "bytes 100-199/4096" {
t.Fatalf("Content-Range = %q", got)
}
if !bytes.Equal(w.Body.Bytes(), body[100:200]) {
t.Fatalf("partial body mismatch: len=%d", w.Body.Len())
}
if disposition := w.Header().Get("Content-Disposition"); !strings.Contains(disposition, "big.bin") {
t.Fatalf("Content-Disposition = %q, want the original filename", disposition)
}
}
// ---------------------------------------------------------------------------
// Minting
// ---------------------------------------------------------------------------
func TestGetAttachmentByID_ProxyModeReturnsRedeemableCapability(t *testing.T) {
store := installProxyModeStorage(t)
id := seedPreviewAttachment(t, store, "downloads/inline.png", "inline.png", "image/png", []byte("png-bytes"))
req := httptest.NewRequest("GET", "/api/attachments/"+id, nil)
req.Header.Set("X-User-ID", testUserID)
req.Header.Set("X-Workspace-ID", testWorkspaceID)
rctx := chi.NewRouteContext()
rctx.URLParams.Add("id", id)
req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
w := httptest.NewRecorder()
testHandler.GetAttachmentByID(w, req)
if w.Code != http.StatusOK {
t.Fatalf("status = %d, want 200; body=%s", w.Code, w.Body.String())
}
var resp AttachmentResponse
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("decode response: %v; body=%s", err, w.Body.String())
}
// Site-relative on purpose: an absolute URL here would be picked up by
// the inline-media re-sign path in packages/views/editor/attachment.tsx
// and pinned into an <img> for far longer than the 60s TTL.
if !strings.HasPrefix(resp.DownloadURL, "/api/attachments/"+id+"/signed-download?") {
t.Fatalf("download_url = %q, want a site-relative capability path", resp.DownloadURL)
}
parsed, err := url.Parse(resp.DownloadURL)
if err != nil {
t.Fatalf("parse download_url: %v", err)
}
if !verifyAttachmentCapability(id, parsed.Query().Get("exp"), parsed.Query().Get("sig"), time.Now()) {
t.Fatalf("minted capability does not verify: %q", resp.DownloadURL)
}
// markdown_url is persisted into comment bodies and must outlive the
// session, so a 60-second capability must never reach it — that is the
// exact class of bug MUL-3130 fixed.
if strings.Contains(resp.MarkdownURL, "signed-download") {
t.Fatalf("markdown_url = %q, must not embed a short-lived capability", resp.MarkdownURL)
}
}
// List-shaped responses are held far longer than the capability TTL, so
// attachmentToResponse must keep emitting the stable endpoint.
func TestAttachmentToResponse_ProxyModeDoesNotMintCapability(t *testing.T) {
store := installProxyModeStorage(t)
id := seedPreviewAttachment(t, store, "downloads/listed.txt", "listed.txt", "text/plain", []byte("listed"))
att, err := testHandler.Queries.GetAttachmentByIDOnly(context.Background(), parseUUID(id))
if err != nil {
t.Fatalf("GetAttachmentByIDOnly: %v", err)
}
resp := testHandler.attachmentToResponse(att, attachmentURLModeSigned)
if want := "/api/attachments/" + id + "/download"; resp.DownloadURL != want {
t.Fatalf("download_url = %q, want stable %q (no capability in list responses)", resp.DownloadURL, want)
}
}
// The pre-existing authenticated route must stay authenticated. Adding a
// public capability entry point must not turn the original path into an open
// one for clients that predate this change.
func TestDownloadAttachment_StillRequiresAuthentication(t *testing.T) {
store := installProxyModeStorage(t)
id := seedPreviewAttachment(t, store, "downloads/guarded.txt", "guarded.txt", "text/plain", []byte("guarded"))
req := httptest.NewRequest("GET", "/api/attachments/"+id+"/download", nil)
rctx := chi.NewRouteContext()
rctx.URLParams.Add("id", id)
req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
w := httptest.NewRecorder()
testHandler.DownloadAttachment(w, req)
if w.Code == http.StatusOK {
t.Fatalf("unauthenticated request to the legacy download path succeeded: %s", w.Body.String())
}
}