hold: gate presigned PUT behind write access, and validate blob identifiers

com.atproto.sync.getBlob took its S3 operation straight from ?method= with no allowlist and passed it to GetPresignedURL, whose switch mints a presigned PUT. The ATProto sub-handler is public per the ATProto spec, and the OCI one gates on ValidateBlobReadAccess, which passes anonymous callers on a public hold by design. So an anonymous caller could obtain a fifteen-minute presigned PUT into the hold's own bucket, over the avatar, the OG-card thumbnails, stored SBOMs and vulnerability reports, or the OCI layer space. Holds sharing a bucket widened it. The defect is that a read check was gating a write. PUT now requires captain or crew with blob:write via the existing ValidateBlobWriteAccess, rather than a new parallel gate. Reads are untouched: ATProto GET and HEAD stay public, OCI GET and HEAD keep the read check. An unknown method is a 400 rather than falling through to a 500 from the presign switch. The identifiers reached object storage keys unvalidated, which the tests showed was not theoretical: cid=../../../../etc/passwd presigned a key containing that path, and an OCI digest of sha256:aa/bb presigned across a shard boundary. A digest is now sha256 plus exactly 64 lowercase hex, shaped after the scanner's ParseDigest, and a CID must decode and round-trip to its canonical encoding. Validation runs before the auth gate, so a malformed identifier is a 400 even for an authorised caller. Verified before changing anything that no legitimate caller reaches PUT presigning. Pushes upload through the multipart endpoints, which live inside requireBlobWriteAccess and use PresignUploadPart; every getBlob caller in the appview, the scanner and the handlers sends GET, HEAD or nothing, and the distribution route can only produce GET or HEAD. One existing test asserted that an unauthenticated PUT returns a presigned URL. It was pinning the vulnerability as intended behaviour and is replaced by coverage of both the refused and the authorised cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
2026-09-20 01:04:15 +00:00 · 2026-09-05 16:50:01 -05:00
parent 22058cc5f4
commit 96b48f4b3d
3 changed files with 597 additions and 73 deletions
@@ -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 ""
 }

@@ -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)
+		}
+	}
+}
@@ -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,