From 5606184bfcbc60cdffdc4e8a38ca4d8c3ea5b699 Mon Sep 17 00:00:00 2001 From: Bohan-J Date: Wed, 29 Jul 2026 18:09:35 +0800 Subject: [PATCH] test(attachments): pin the CLI capability header and the rotation premise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server/internal/cli/client_test.go | 98 +++++++++++++++++++ .../handler/attachment_url_mode_test.go | 33 ++++++- 2 files changed, 128 insertions(+), 3 deletions(-) diff --git a/server/internal/cli/client_test.go b/server/internal/cli/client_test.go index ac1aaeaced..0f6f05942a 100644 --- a/server/internal/cli/client_test.go +++ b/server/internal/cli/client_test.go @@ -427,3 +427,101 @@ func TestNormalizeGOOS(t *testing.T) { } } } + +// TestSetHeaders_AdvertisesStableAttachmentURLs is the only test that proves +// Phase 1 of MUL-5372 is actually switched on. +// +// The server-side tests verify that a request carrying +// `X-Client-Capabilities: stable_attachment_urls` gets stable attachment paths, +// but nothing there observes what the CLI sends. Without this assertion a typo +// in the token, or dropping the header from setHeaders entirely, would leave +// every other test green while the CLI silently went back to receiving ~800-char +// signed URLs on every attachment of every list read. +// +// The exact string is load-bearing: the server matches the token literally +// (handler.requestHasClientCapability), so it is asserted verbatim rather than +// through the constant. +func TestSetHeaders_AdvertisesStableAttachmentURLs(t *testing.T) { + const wantCapability = "stable_attachment_urls" + + // Every verb goes through setHeaders, and an agent's read path is not only + // GET (comment add posts, attachment upload multiparts). Cover the shapes + // that actually carry attachment payloads back. + t.Run("GET", func(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("X-Client-Capabilities") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + client := NewAPIClient(srv.URL, "ws-1", "test-token") + var out map[string]any + if err := client.GetJSON(context.Background(), "/api/issues/x/comments", &out); err != nil { + t.Fatalf("GetJSON: %v", err) + } + if got != wantCapability { + t.Errorf("X-Client-Capabilities = %q, want %q — Phase 1 is off unless the CLI advertises this", got, wantCapability) + } + }) + + t.Run("GET with headers", func(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("X-Client-Capabilities") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + client := NewAPIClient(srv.URL, "ws-1", "test-token") + var out []map[string]any + if _, err := client.GetJSONWithHeaders(context.Background(), "/api/issues/x/comments", &out); err != nil { + t.Fatalf("GetJSONWithHeaders: %v", err) + } + if got != wantCapability { + t.Errorf("X-Client-Capabilities = %q, want %q", got, wantCapability) + } + }) + + t.Run("POST", func(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("X-Client-Capabilities") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + client := NewAPIClient(srv.URL, "ws-1", "test-token") + var out map[string]any + if err := client.PostJSON(context.Background(), "/api/issues/x/comments", map[string]string{"content": "hi"}, &out); err != nil { + t.Fatalf("PostJSON: %v", err) + } + if got != wantCapability { + t.Errorf("X-Client-Capabilities = %q, want %q", got, wantCapability) + } + }) + + // An unauthenticated client (no token configured yet) must still advertise: + // the capability describes what the binary can parse, not who it is. + t.Run("without a token", func(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("X-Client-Capabilities") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + client := NewAPIClient(srv.URL, "", "") + var out map[string]any + if err := client.GetJSON(context.Background(), "/api/health", &out); err != nil { + t.Fatalf("GetJSON: %v", err) + } + if got != wantCapability { + t.Errorf("X-Client-Capabilities = %q, want %q", got, wantCapability) + } + }) +} diff --git a/server/internal/handler/attachment_url_mode_test.go b/server/internal/handler/attachment_url_mode_test.go index 3bc7ecd42f..9ac9ccf5db 100644 --- a/server/internal/handler/attachment_url_mode_test.go +++ b/server/internal/handler/attachment_url_mode_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/go-chi/chi/v5" db "github.com/multica-ai/multica/server/pkg/db/generated" @@ -124,9 +125,10 @@ func TestAttachmentToResponse_StableModeDropsSignature(t *testing.T) { } } -// TestAttachmentToResponse_SignedModeStillRotates documents why this change -// exists at all: the signed value is re-minted per request, so identical -// content yields different bytes and defeats content-keyed caching. +// TestAttachmentToResponse_SignedModeRotatesButStableDoesNot documents why this +// change exists at all: the signed value is a function of an expiry the server +// re-derives per request, so identical content yields different bytes on every +// read and defeats content-keyed caching. Stable mode has no such term. func TestAttachmentToResponse_SignedModeRotatesButStableDoesNot(t *testing.T) { withCloudFrontSigner(t) @@ -141,6 +143,31 @@ func TestAttachmentToResponse_SignedModeRotatesButStableDoesNot(t *testing.T) { if strings.Contains(stableA.DownloadURL, "Policy=") || strings.Contains(stableA.DownloadURL, "Signature=") { t.Errorf("stable download_url leaked signature params: %q", stableA.DownloadURL) } + + // The rotation half. attachmentToResponse mints its expiry from time.Now() + // at second granularity, so calling it twice in a row would usually land in + // the same second and produce identical bytes — asserting on that directly + // would be a flaky test of a real property. Drive the signer with two + // explicit expiries instead: it proves the URL varies with the expiry term, + // which is exactly what makes the response unstable as the clock advances. + signed := testHandler.attachmentToResponse(att, attachmentURLModeSigned) + if !strings.Contains(signed.DownloadURL, "Policy=") { + t.Fatalf("signed mode should embed a policy, got %q", signed.DownloadURL) + } + base := time.Now() + first := testHandler.CFSigner.SignedURL(att.Url, base.Add(30*time.Minute)) + second := testHandler.CFSigner.SignedURL(att.Url, base.Add(30*time.Minute+time.Second)) + if first == second { + t.Errorf("signed URL did not change with the expiry; rotation is the premise of this whole change") + } + if strings.Split(first, "?")[0] != strings.Split(second, "?")[0] { + t.Errorf("only the query should rotate, the resource path must be stable: %q vs %q", first, second) + } + // And re-signing with the SAME expiry must reproduce the same bytes — + // otherwise the rotation above would prove nothing about the clock. + if again := testHandler.CFSigner.SignedURL(att.Url, base.Add(30*time.Minute)); again != first { + t.Errorf("signing is not deterministic for a fixed expiry: %q vs %q", again, first) + } } // TestAttachmentToResponse_StableModeIsNoOpWithoutSigner pins the presign /