diff --git a/pkg/hold/pds/xrpc.go b/pkg/hold/pds/xrpc.go index af56205..9b96f8b 100644 --- a/pkg/hold/pds/xrpc.go +++ b/pkg/hold/pds/xrpc.go @@ -1228,29 +1228,52 @@ func (h *XRPCHandler) HandleGetBlob(w http.ResponseWriter, r *http.Request) { // handleGetOCIBlob handles OCI container image blob requests // Returns JSON with presigned URL for AppView integration -// Authorization: Protected by hold access control (captain.public or crew with blob:read) +// Authorization: +// - GET/HEAD: hold read access (captain.public, or crew with blob:read) +// - PUT: hold write access (captain, or crew with blob:write). A read verdict +// is not enough: on a public hold ValidateBlobReadAccess passes anonymous +// callers, and a presigned PUT is a write into the shared content-addressed +// layer space. func (h *XRPCHandler) handleGetOCIBlob(w http.ResponseWriter, r *http.Request, did, digest string) { slog.Debug("Processing OCI blob", "digest", digest) - // Validate blob read access (hold access control) - // If captain.public = true, returns nil (public access allowed) - // If captain.public = false, validates auth and checks for blob:read permission - scannerSecret := "" - if h.scanBroadcaster != nil { - scannerSecret = h.scanBroadcaster.Secret() - } - _, err := ValidateBlobReadAccess(r, h.pds, h.httpClient, scannerSecret) - if err != nil { - slog.Warn("OCI blob authorization failed", "error", err, "digest", digest) - http.Error(w, fmt.Sprintf("authorization failed: %v", err), http.StatusForbidden) + // Validate the digest before it becomes an S3 key + if err := validateOCIDigest(digest); err != nil { + slog.Warn("Rejected OCI blob request with malformed digest", "error", err, "digest", digest) + http.Error(w, fmt.Sprintf("invalid cid: %v", err), http.StatusBadRequest) return } - // Determine presigned URL operation (GET or HEAD) - // Check for ?method=HEAD query parameter first (from AppView) - operation := r.URL.Query().Get("method") - if operation == "" { - operation = "GET" + // Determine presigned URL operation (GET, HEAD or PUT) + // Check for ?method= query parameter first (from AppView) + operation, err := parseBlobPresignOperation(r) + if err != nil { + slog.Warn("Rejected OCI blob request with unsupported method", "error", err, "digest", digest) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + if operation == http.MethodPut { + // Write capability: gate on the hold's write model, same as the + // multipart upload endpoints. + if _, err := ValidateBlobWriteAccess(r, h.pds, h.httpClient); err != nil { + slog.Warn("OCI blob write authorization failed", "error", err, "digest", digest) + http.Error(w, fmt.Sprintf("authorization failed: %v", err), http.StatusForbidden) + return + } + } else { + // Validate blob read access (hold access control) + // If captain.public = true, returns nil (public access allowed) + // If captain.public = false, validates auth and checks for blob:read permission + scannerSecret := "" + if h.scanBroadcaster != nil { + scannerSecret = h.scanBroadcaster.Secret() + } + if _, err := ValidateBlobReadAccess(r, h.pds, h.httpClient, scannerSecret); err != nil { + slog.Warn("OCI blob authorization failed", "error", err, "digest", digest) + http.Error(w, fmt.Sprintf("authorization failed: %v", err), http.StatusForbidden) + return + } } // Generate presigned URL (use empty DID for content-addressed storage) @@ -1277,9 +1300,13 @@ func (h *XRPCHandler) handleGetOCIBlob(w http.ResponseWriter, r *http.Request, d // handleGetATProtoBlob handles standard ATProto blob requests // Returns 307 redirect to presigned URL (standard ATProto behavior) -// Authorization: Public per ATProto spec (no auth required) -func (h *XRPCHandler) handleGetATProtoBlob(w http.ResponseWriter, r *http.Request, did, cid string) { - slog.Debug("Processing ATProto blob", "cid", cid) +// Authorization: +// - GET/HEAD: public per ATProto spec (no auth required) +// - PUT: hold write access (captain, or crew with blob:write). The public read +// is a spec obligation and stops at reads; a presigned PUT would let any +// anonymous caller write to /repos/{did}/blobs/{cid}/data. +func (h *XRPCHandler) handleGetATProtoBlob(w http.ResponseWriter, r *http.Request, did, blobCID string) { + slog.Debug("Processing ATProto blob", "cid", blobCID) // Validate DID (ATProto blobs are stored per-DID for data sovereignty) if did != h.pds.DID() { @@ -1290,19 +1317,38 @@ func (h *XRPCHandler) handleGetATProtoBlob(w http.ResponseWriter, r *http.Reques return } - // Determine presigned URL operation (GET or HEAD) - operation := r.URL.Query().Get("method") - if operation == "" { - operation = "GET" + // Validate the CID before it becomes an S3 key + if err := validateATProtoBlobCID(blobCID); err != nil { + slog.Warn("Rejected ATProto blob request with malformed cid", "error", err, "cid", blobCID) + http.Error(w, fmt.Sprintf("invalid cid: %v", err), http.StatusBadRequest) + return + } + + // Determine presigned URL operation (GET, HEAD or PUT) + operation, err := parseBlobPresignOperation(r) + if err != nil { + slog.Warn("Rejected ATProto blob request with unsupported method", "error", err, "cid", blobCID) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + if operation == http.MethodPut { + // Write capability: gate on the hold's write model. This endpoint is + // otherwise unauthenticated. + if _, err := ValidateBlobWriteAccess(r, h.pds, h.httpClient); err != nil { + slog.Warn("ATProto blob write authorization failed", "error", err, "cid", blobCID) + http.Error(w, fmt.Sprintf("authorization failed: %v", err), http.StatusForbidden) + return + } } // Generate presigned URL (use DID for per-DID storage path) - presignedURL, err := h.GetPresignedURL(r.Context(), operation, cid, did) + presignedURL, err := h.GetPresignedURL(r.Context(), operation, blobCID, did) if err != nil { slog.Error("Failed to get presigned URL for ATProto blob", "error", err, "operation", operation, - "cid", cid, + "cid", blobCID, "did", did) http.Error(w, "failed to get presigned URL", http.StatusInternalServerError) return @@ -1595,8 +1641,84 @@ func (h *XRPCHandler) HandleRequestCrew(w http.ResponseWriter, r *http.Request) }) } +// parseBlobPresignOperation resolves the ?method= query parameter into the S3 +// operation to presign. +// +// The allowlist is the point. The parameter is caller-controlled and decides +// whether the endpoint hands back a read capability or a write one, so an +// unrecognised value is refused rather than defaulted: silently treating +// "put" or "PATCH" as a read hides a client bug, and any future operation +// added to GetPresignedURL is unreachable from here until it is named. +// +// GET and HEAD are reads and are served under the endpoint's read rules. PUT +// mints a write capability and MUST be gated by the caller on +// ValidateBlobWriteAccess before the URL is generated. +func parseBlobPresignOperation(r *http.Request) (string, error) { + switch operation := r.URL.Query().Get("method"); operation { + case "": + // No method parameter: this is a plain blob read. + return http.MethodGet, nil + case http.MethodGet, http.MethodHead, http.MethodPut: + return operation, nil + default: + return "", fmt.Errorf("unsupported method %q: expected GET, HEAD or PUT", operation) + } +} + +// ociDigestHexLen is the number of hex characters in a sha256 digest. +const ociDigestHexLen = 64 + +// validateOCIDigest checks that an OCI blob digest is exactly "sha256:" +// followed by 64 lowercase hex characters. +// +// The digest arrives as a query parameter and is turned into an S3 key by +// s3.BlobPath, which shards on its first two characters and interpolates the +// rest. Constraining it to [0-9a-f] means it cannot carry a separator, a dot +// or a NUL, so it cannot name a key outside the blob space it belongs to. +// Only sha256 is accepted: it is the only algorithm ATCR stores, and the only +// one the sha256: prefix check in HandleGetBlob routes here in the first place. +func validateOCIDigest(digest string) error { + hex, ok := strings.CutPrefix(digest, "sha256:") + if !ok { + return fmt.Errorf("digest %q is not a sha256 digest", digest) + } + if len(hex) != ociDigestHexLen { + return fmt.Errorf("digest %q has %d hex characters, want %d", digest, len(hex), ociDigestHexLen) + } + for i := 0; i < len(hex); i++ { + if c := hex[i]; (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return fmt.Errorf("digest %q is not lowercase hex", digest) + } + } + return nil +} + +// validateATProtoBlobCID checks that an ATProto blob CID is a well formed CID +// in its canonical string form. +// +// Same reasoning as validateOCIDigest: the value is caller-controlled and +// becomes a path element in atprotoBlobPath. Decoding is not sufficient on its +// own, so the parsed CID is re-encoded and compared: that rejects any trailing +// or alternative-encoding variant and guarantees the string written into the +// key is exactly the one the CID denotes. +func validateATProtoBlobCID(blobCID string) error { + parsed, err := cid.Decode(blobCID) + if err != nil { + return fmt.Errorf("%q is not a valid CID: %w", blobCID, err) + } + if parsed.String() != blobCID { + return fmt.Errorf("%q is not a CID in canonical form", blobCID) + } + return nil +} + // GetPresignedURL generates a presigned URL for GET, HEAD, or PUT operations // Distinguishes between ATProto blobs (per-DID) and OCI blobs (content-addressed) +// +// PUT returns a write capability for the hold's bucket, valid for 15 minutes. +// Callers are responsible for authorizing the write (ValidateBlobWriteAccess) +// and for validating the digest or CID (validateOCIDigest, +// validateATProtoBlobCID) before calling with an operation of PUT. func (h *XRPCHandler) GetPresignedURL(ctx context.Context, operation string, digest string, did string) (string, error) { var path string @@ -1694,8 +1816,12 @@ func getProxyURL(publicURL string, digest, holdDID string, operation string) str publicURL, atproto.SyncGetBlob, holdDID, digest) } - // For PUT operations, proxy fallback is not supported with XRPC - // Clients should use multipart upload flow via com.atproto.repo.uploadBlob + // Anything else, PUT included, gets no fallback URL. getBlob is a read + // endpoint and its proxy target would be reached without the write gate + // that guards the presign path, so there is no safe write fallback to + // return here. Clients should use the multipart upload flow, or + // com.atproto.repo.uploadBlob for small ATProto blobs; both authorize the + // write themselves. return "" } diff --git a/pkg/hold/pds/xrpc_blob_presign_test.go b/pkg/hold/pds/xrpc_blob_presign_test.go new file mode 100644 index 0000000..3074d2a --- /dev/null +++ b/pkg/hold/pds/xrpc_blob_presign_test.go @@ -0,0 +1,435 @@ +package pds + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "atcr.io/pkg/atproto" + "atcr.io/pkg/s3" +) + +// getBlob is routed on GET and HEAD and serves two branches: the ATProto blob +// branch, which is unauthenticated by spec, and the OCI branch, which lets +// anonymous callers through whenever the captain record says the hold is +// public. Both branches take the S3 operation to presign straight from the +// ?method= query parameter, and PUT presigning mints a 15 minute write +// capability for the hold's bucket. +// +// So two invariants have to hold on this endpoint: +// +// 1. A write capability is only ever handed to a caller who has passed the +// hold's write gate (captain, or crew with blob:write). A read +// authorization must not be convertible into a write. +// 2. Neither the cid nor the digest reaches an S3 key without being validated. +// +// These tests assert on the mock S3 client's recorded presign calls, so a +// regression is caught where the capability would be minted rather than where +// it would be used. + +const ( + // A well formed ATProto blob CID (CIDv1, raw codec, sha2-256). + testBlobCID = "bafyreib2rxk3rkhh5ylyxj3x3gathxt3s32qvwj2lf3qg4kmzr6b7teqke" + // A well formed OCI layer digest. + testOCIDigest = "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f" + // The owner DID that setupTestXRPCHandlerWithMockS3 bootstraps. + testHoldOwnerDID = "did:plc:testowner123" + testHoldDID = "did:web:hold.example.com" + testUserPDSURL = "https://test-pds.example.com" +) + +// assertNoWriteCapability fails if the handler minted an S3 write presign, or +// if the response body or redirect target looks like one. +func assertNoWriteCapability(t *testing.T, w *httptest.ResponseRecorder, mock *s3.MockS3Client) { + t.Helper() + + if len(mock.PutObjectCalls) != 0 { + t.Errorf("handler minted %d presigned PUT(s) for a caller without write access: %+v", + len(mock.PutObjectCalls), mock.PutObjectCalls) + } + + if loc := w.Header().Get("Location"); strings.Contains(loc, "/put/") { + t.Errorf("handler redirected to a presigned PUT URL: %s", loc) + } + + var body map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &body); err == nil { + if strings.Contains(body["url"], "/put/") { + t.Errorf("handler returned a presigned PUT URL: %s", body["url"]) + } + } +} + +// putBlobRequest builds an anonymous getBlob request asking for a write presign. +func putBlobRequest(blobID string) *http.Request { + return makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ + "did": testHoldDID, + "cid": blobID, + "method": "PUT", + }) +} + +// authorizeAs attaches DPoP credentials for the given DID, the same way the +// hold's other write-gated endpoints are exercised in auth_test.go. +func authorizeAs(t *testing.T, req *http.Request, did string) *http.Request { + t.Helper() + + if err := AddTestDPoP(req, did, testUserPDSURL); err != nil { + t.Fatalf("Failed to add DPoP to request: %v", err) + } + return req +} + +// addCrew registers a crew member with the given permissions on the test hold. +func addCrew(t *testing.T, handler *XRPCHandler, ctx context.Context, did string, permissions ...string) { + t.Helper() + + if _, err := handler.pds.AddCrewMember(ctx, did, "member", permissions, ""); err != nil { + t.Fatalf("Failed to add crew member %s: %v", did, err) + } +} + +// TestGetBlob_ATProtoPutRequiresWriteAccess covers the ATProto blob branch, +// which serves reads to anonymous callers per the ATProto spec. That public +// read must not extend to writes: a PUT presign for +// /repos/{did}/blobs/{cid}/data is a write to the hold's bucket and needs the +// hold's write gate. +func TestGetBlob_ATProtoPutRequiresWriteAccess(t *testing.T) { + t.Run("anonymous is refused", func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, putBlobRequest(testBlobCID)) + + if w.Code != http.StatusForbidden { + t.Errorf("Expected 403 for anonymous PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + + t.Run("crew without blob:write is refused", func(t *testing.T) { + handler, mockS3Client, ctx := setupTestXRPCHandlerWithMockS3(t) + readerDID := "did:plc:reader123" + addCrew(t, handler, ctx, readerDID, "blob:read") + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testBlobCID), readerDID)) + + if w.Code != http.StatusForbidden { + t.Errorf("Expected 403 for blob:read-only crew PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + + t.Run("owner is allowed", func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testBlobCID), testHoldOwnerDID)) + + if w.Code != http.StatusTemporaryRedirect { + t.Fatalf("Expected 307 for authorized PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + if len(mockS3Client.PutObjectCalls) != 1 { + t.Fatalf("Expected 1 presigned PUT for the owner, got %d", len(mockS3Client.PutObjectCalls)) + } + if !strings.Contains(w.Header().Get("Location"), "/put/") { + t.Errorf("Expected a presigned PUT redirect, got %s", w.Header().Get("Location")) + } + }) + + t.Run("crew with blob:write is allowed", func(t *testing.T) { + handler, mockS3Client, ctx := setupTestXRPCHandlerWithMockS3(t) + writerDID := "did:plc:writer123" + addCrew(t, handler, ctx, writerDID, "blob:write") + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testBlobCID), writerDID)) + + if w.Code != http.StatusTemporaryRedirect { + t.Fatalf("Expected 307 for authorized PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + if len(mockS3Client.PutObjectCalls) != 1 { + t.Errorf("Expected 1 presigned PUT for blob:write crew, got %d", len(mockS3Client.PutObjectCalls)) + } + }) +} + +// TestGetBlob_OCIPutRequiresWriteAccess covers the OCI branch on a public hold, +// where ValidateBlobReadAccess returns a nil error for an anonymous caller. +// That read verdict is the wrong gate for a write into the content-addressed +// layer space. +func TestGetBlob_OCIPutRequiresWriteAccess(t *testing.T) { + t.Run("anonymous on a public hold is refused", func(t *testing.T) { + handler, mockS3Client, ctx := setupTestXRPCHandlerWithMockS3(t) + + // The fixture bootstraps a public hold; assert it rather than assume it, + // so this stays the public-hold case if the fixture changes. + _, captain, err := handler.pds.GetCaptainRecord(ctx) + if err != nil { + t.Fatalf("Failed to read captain record: %v", err) + } + if !captain.Public { + t.Fatal("fixture hold is not public; this test must exercise the anonymous public-hold path") + } + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, putBlobRequest(testOCIDigest)) + + if w.Code != http.StatusForbidden { + t.Errorf("Expected 403 for anonymous PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + + t.Run("crew without blob:write is refused", func(t *testing.T) { + handler, mockS3Client, ctx := setupTestXRPCHandlerWithMockS3(t) + readerDID := "did:plc:reader123" + addCrew(t, handler, ctx, readerDID, "blob:read") + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testOCIDigest), readerDID)) + + if w.Code != http.StatusForbidden { + t.Errorf("Expected 403 for blob:read-only crew PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + + t.Run("owner is allowed", func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testOCIDigest), testHoldOwnerDID)) + + if w.Code != http.StatusOK { + t.Fatalf("Expected 200 for authorized PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + if len(mockS3Client.PutObjectCalls) != 1 { + t.Fatalf("Expected 1 presigned PUT for the owner, got %d", len(mockS3Client.PutObjectCalls)) + } + + var body map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("Failed to parse response: %v", err) + } + if !strings.Contains(body["url"], "/put/") { + t.Errorf("Expected a presigned PUT URL, got %s", body["url"]) + } + }) + + t.Run("crew with blob:write is allowed", func(t *testing.T) { + handler, mockS3Client, ctx := setupTestXRPCHandlerWithMockS3(t) + writerDID := "did:plc:writer123" + addCrew(t, handler, ctx, writerDID, "blob:write") + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(testOCIDigest), writerDID)) + + if w.Code != http.StatusOK { + t.Fatalf("Expected 200 for authorized PUT, got %d (body: %s)", w.Code, w.Body.String()) + } + if len(mockS3Client.PutObjectCalls) != 1 { + t.Errorf("Expected 1 presigned PUT for blob:write crew, got %d", len(mockS3Client.PutObjectCalls)) + } + }) +} + +// TestGetBlob_RejectsUnknownOperations pins the allowlist: anything that is not +// GET, HEAD or PUT is refused with a 400 rather than defaulting to a read or +// reaching S3 at all. Case matters, since the value is compared against the +// canonical http.Method constants. +func TestGetBlob_RejectsUnknownOperations(t *testing.T) { + operations := []string{"POST", "DELETE", "put", "get", "PATCH", "garbage"} + + for _, op := range operations { + for _, blobID := range []string{testBlobCID, testOCIDigest} { + t.Run(op+"/"+blobID[:12], func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ + "did": testHoldDID, + "cid": blobID, + "method": op, + }) + w := httptest.NewRecorder() + + handler.HandleGetBlob(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("Expected 400 for method=%q, got %d (body: %s)", op, w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + + if n := len(mockS3Client.GetObjectCalls) + len(mockS3Client.HeadObjectCalls); n != 0 { + t.Errorf("Expected no presign calls for rejected method=%q, got %d", op, n) + } + }) + } + } +} + +// TestGetBlob_RejectsMalformedCID pins that the cid parameter is validated +// before it becomes an S3 key. It reaches storage through atprotoBlobPath. +func TestGetBlob_RejectsMalformedCID(t *testing.T) { + cases := []struct { + name string + cid string + }{ + {"traversal", "../../../../etc/passwd"}, + {"slash", "some/nested/key"}, + {"not_a_cid", "not-a-cid"}, + {"truncated_cid", "bafkreiabcd"}, + {"trailing_junk", testBlobCID + "/../../evil"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ + "did": testHoldDID, + "cid": tc.cid, + }) + w := httptest.NewRecorder() + + handler.HandleGetBlob(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("Expected 400 for cid=%q, got %d (body: %s)", tc.cid, w.Code, w.Body.String()) + } + if n := len(mockS3Client.GetObjectCalls); n != 0 { + t.Errorf("Expected no presign for malformed cid=%q, got %d call(s): %+v", + tc.cid, n, mockS3Client.GetObjectCalls) + } + }) + } +} + +// TestGetBlob_RejectsMalformedOCIDigest pins the same for the OCI branch, where +// the only check today is the sha256: prefix that routed the request here. +func TestGetBlob_RejectsMalformedOCIDigest(t *testing.T) { + cases := []struct { + name string + digest string + }{ + {"traversal", "sha256:../../../etc/passwd"}, + {"slash", "sha256:aa/bb"}, + {"short_hex", "sha256:abc123def456"}, + {"uppercase", "sha256:E692418E4CBAF90CA69D05A66403747BAA33EE08806650B51FAB815AD7FC331F"}, + {"empty_hex", "sha256:"}, + {"non_hex", "sha256:" + strings.Repeat("z", 64)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ + "did": testHoldDID, + "cid": tc.digest, + }) + w := httptest.NewRecorder() + + handler.HandleGetBlob(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("Expected 400 for cid=%q, got %d (body: %s)", tc.digest, w.Code, w.Body.String()) + } + if n := len(mockS3Client.GetObjectCalls); n != 0 { + t.Errorf("Expected no presign for malformed digest=%q, got %d call(s): %+v", + tc.digest, n, mockS3Client.GetObjectCalls) + } + }) + } +} + +// TestGetBlob_MalformedIdentifierRejectedBeforeWriteGate pins the ordering: a +// malformed identifier is refused on its own terms, and never becomes an S3 key +// even for a caller who does hold write access. +func TestGetBlob_MalformedIdentifierRejectedBeforeWriteGate(t *testing.T) { + for _, blobID := range []string{"../../../../etc/passwd", "sha256:../../../etc/passwd"} { + t.Run(blobID, func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + w := httptest.NewRecorder() + handler.HandleGetBlob(w, authorizeAs(t, putBlobRequest(blobID), testHoldOwnerDID)) + + if w.Code != http.StatusBadRequest { + t.Errorf("Expected 400 for cid=%q, got %d (body: %s)", blobID, w.Code, w.Body.String()) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + } +} + +// TestGetBlob_ReadOperationsStillWork is the positive control: the anonymous +// GET and HEAD reads that AppView, the scanner and the web UI depend on must +// keep working unchanged, on both branches. +func TestGetBlob_ReadOperationsStillWork(t *testing.T) { + cases := []struct { + name string + cid string + method string + wantStatus int + wantGets int + wantHeads int + }{ + {"oci_default_get", testOCIDigest, "", http.StatusOK, 1, 0}, + {"oci_explicit_get", testOCIDigest, "GET", http.StatusOK, 1, 0}, + {"oci_head", testOCIDigest, "HEAD", http.StatusOK, 0, 1}, + {"atproto_default_get", testBlobCID, "", http.StatusTemporaryRedirect, 1, 0}, + {"atproto_explicit_get", testBlobCID, "GET", http.StatusTemporaryRedirect, 1, 0}, + {"atproto_head", testBlobCID, "HEAD", http.StatusTemporaryRedirect, 0, 1}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) + + params := map[string]string{ + "did": testHoldDID, + "cid": tc.cid, + } + if tc.method != "" { + params["method"] = tc.method + } + req := makeXRPCGetRequest(atproto.SyncGetBlob, params) + w := httptest.NewRecorder() + + handler.HandleGetBlob(w, req) + + if w.Code != tc.wantStatus { + t.Errorf("Expected %d, got %d (body: %s)", tc.wantStatus, w.Code, w.Body.String()) + } + if len(mockS3Client.GetObjectCalls) != tc.wantGets { + t.Errorf("Expected %d GetObject presign(s), got %d", tc.wantGets, len(mockS3Client.GetObjectCalls)) + } + if len(mockS3Client.HeadObjectCalls) != tc.wantHeads { + t.Errorf("Expected %d HeadObject presign(s), got %d", tc.wantHeads, len(mockS3Client.HeadObjectCalls)) + } + assertNoWriteCapability(t, w, mockS3Client) + }) + } +} + +// TestGetProxyURL_NoWriteFallback pins the fallback used when presigning fails +// or no S3 client is configured. It is an unauthenticated public endpoint, so +// it must never stand in for a write: writes go through the multipart upload +// flow, which has its own write gate. +func TestGetProxyURL_NoWriteFallback(t *testing.T) { + for _, op := range []string{http.MethodPut, http.MethodPost, http.MethodDelete, "garbage"} { + if got := getProxyURL("https://hold.example.com", testOCIDigest, testHoldDID, op); got != "" { + t.Errorf("getProxyURL(%q) returned %q, want empty", op, got) + } + } + + for _, op := range []string{http.MethodGet, http.MethodHead} { + if got := getProxyURL("https://hold.example.com", testOCIDigest, testHoldDID, op); got == "" { + t.Errorf("getProxyURL(%q) returned empty, want the XRPC read endpoint", op) + } + } +} diff --git a/pkg/hold/pds/xrpc_test.go b/pkg/hold/pds/xrpc_test.go index 7d1b4b3..c0230e5 100644 --- a/pkg/hold/pds/xrpc_test.go +++ b/pkg/hold/pds/xrpc_test.go @@ -2325,7 +2325,7 @@ func TestHandleGetBlob_MockS3_SHA256Digest(t *testing.T) { handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) holdDID := "did:web:hold.example.com" - digest := "sha256:abc123def456" // OCI digest format + digest := testOCIDigest // OCI digest format req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ "did": holdDID, @@ -2367,7 +2367,7 @@ func TestHandleGetBlob_MockS3_HeadMethod(t *testing.T) { handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) holdDID := "did:web:hold.example.com" - digest := "sha256:abc123def456" // OCI digest format + digest := testOCIDigest // OCI digest format // Use HEAD instead of GET with method query param req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ @@ -2406,48 +2406,11 @@ func TestHandleGetBlob_MockS3_HeadMethod(t *testing.T) { } } -// TestHandleGetBlob_MockS3_PutMethod tests PUT request support with MockS3Client -func TestHandleGetBlob_MockS3_PutMethod(t *testing.T) { - handler, mockS3Client, _ := setupTestXRPCHandlerWithMockS3(t) - - holdDID := "did:web:hold.example.com" - digest := "sha256:abc123def456" // OCI digest format - - req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ - "did": holdDID, - "cid": digest, - "method": "PUT", - }) - w := httptest.NewRecorder() - - handler.HandleGetBlob(w, req) - - // Should return 200 OK with JSON response - if w.Code != http.StatusOK { - t.Errorf("Expected status 200 OK, got %d", w.Code) - } - - // Parse JSON response - var response map[string]string - if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { - t.Fatalf("Failed to parse JSON response: %v", err) - } - - // Verify URL field points to mock S3 - if response["url"] == "" { - t.Error("Expected url field in response") - } - - // URL should be from mock S3 server - if !strings.Contains(response["url"], "mock-s3.example.com") { - t.Errorf("Expected mock S3 URL, got: %s", response["url"]) - } - - // Verify PutObjectPresignable was called - if len(mockS3Client.PutObjectCalls) != 1 { - t.Errorf("Expected 1 PutObject call, got %d", len(mockS3Client.PutObjectCalls)) - } -} +// PUT presigning used to be tested here as an unauthenticated capability, which +// is what made it an anonymous write into the hold's bucket. It now requires +// hold write access; see TestGetBlob_OCIPutRequiresWriteAccess and +// TestGetBlob_ATProtoPutRequiresWriteAccess in xrpc_blob_presign_test.go, which +// cover both the refused and the authorized cases on both branches. // Tests for HandleGetBlob (without S3 - fallback to XRPC proxy) @@ -2491,7 +2454,7 @@ func TestHandleGetBlob_SHA256Digest(t *testing.T) { handler, _, _ := setupTestXRPCHandlerWithBlobs(t) holdDID := "did:web:hold.example.com" - digest := "sha256:abc123def456" // OCI digest format + digest := testOCIDigest // OCI digest format req := makeXRPCGetRequest(atproto.SyncGetBlob, map[string]string{ "did": holdDID,