Skip to content

Commit c15d801

Browse files
committed
fix(sqs): gate v2 receipt handles at the public API boundary
CodeRabbit major on PR #724 round 2: the v2 codec is in the binary AND reachable on the public API. A client can re-encode a legitimately-issued v1 handle's (queue_gen, message_id, receipt_token) under the v2 layout, and DeleteMessage / ChangeMessageVisibility will accept it (downstream validation only checks queue_gen + receipt_token). The behaviour is technically correct (the v1 keyspace lookup still finds the message) but it leaks the new wire format before PR 5b lands — breaking the "no behavior change yet" guarantee. Fix decodeClientReceiptHandle wraps decodeReceiptHandle with the dormancy gate: any v2 handle on the public API surfaces as ReceiptHandleIsInvalid until PR 5b's queue-aware version replaces the gate (v1 required on non-partitioned queues, v2 required on partitioned ones). Changed call sites Three public-API decode points switch from decodeReceiptHandle to decodeClientReceiptHandle: - parseQueueAndReceipt (sqs_messages.go) — DeleteMessage and ChangeMessageVisibility entry point. - DeleteMessageBatch entry decode (sqs_messages_batch.go). - ChangeMessageVisibilityBatch entry decode (sqs_messages_batch.go). The low-level decodeReceiptHandle keeps working as a pure codec so the existing v1 + v2 round-trip tests keep passing. Tests - TestDecodeClientReceiptHandle_AcceptsV1: v1 handles flow through unchanged. - TestDecodeClientReceiptHandle_RejectsV2: a v2 handle on the public API surfaces as the dormancy-gate error. The low-level decoder still accepts it (the gate is at the API boundary, not in the codec). - TestDecodeClientReceiptHandle_PassesThroughDecodeErrors: a decode-step error (e.g. base64 garbage) is NOT masked by the dormancy-gate message — operators see the underlying error. Audit per the lessons-learned discipline The semantic change here is the rejection contract for v2 handles on the public API. grep -rn "decodeReceiptHandle" across adapter/ showed 3 production call sites (parseQueueAndReceipt, DeleteMessageBatch, ChangeMessageVisibilityBatch). All 3 updated. The v2 codec round-trip tests intentionally keep using decodeReceiptHandle directly so they exercise the codec, not the public API. Note on Gemini's "appendU32 is undefined" finding (round 2) False positive. appendU32 is defined in adapter/sqs_keys.go:444 and has been since PR #703 merged. Gemini did not search across files in the package; CI build is green and the round-1 v2 round-trip tests exercise the function. No code change needed.
1 parent 42c9894 commit c15d801

3 files changed

Lines changed: 109 additions & 3 deletions

File tree

adapter/sqs_messages.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,48 @@ func decodeReceiptHandleV1(b []byte) (*decodedReceiptHandle, error) {
368368
}, nil
369369
}
370370

371+
// decodeClientReceiptHandle is the public-API entry point for
372+
// decoding a client-supplied receipt handle. It wraps
373+
// decodeReceiptHandle with the dormancy gate that keeps the v2
374+
// codec inert until Phase 3.D PR 5b wires the partitioned-FIFO
375+
// data plane.
376+
//
377+
// # Why the gate
378+
//
379+
// PR 5a adds the v2 codec to the binary but does NOT yet wire any
380+
// production path that produces v2 handles — SendMessage on a
381+
// partitioned queue is rejected by the §11 PR 2 dormancy gate
382+
// (PartitionCount > 1 → InvalidAttributeValue). Without this
383+
// helper, a client could craft a v2 handle (re-encoding a
384+
// legitimately-issued v1 handle's queue_gen / message_id /
385+
// receipt_token under the v2 layout) and DeleteMessage /
386+
// ChangeMessageVisibility would accept it, since the downstream
387+
// validation only checks queue_gen + receipt_token. The behaviour
388+
// is technically correct (the v1 keyspace lookup still finds the
389+
// message) but it leaks the new wire format before PR 5b lands —
390+
// breaking the "no behavior change yet" guarantee of this PR
391+
// (codex/coderabbit major on PR #724).
392+
//
393+
// PR 5b lifts this gate together with the rest of the data-plane
394+
// fanout: it replaces the != v1 check with a queue-aware version
395+
// (v1 required on non-partitioned queues, v2 required on
396+
// partitioned ones), so neither version leaks into the wrong
397+
// keyspace. Until then, any v2 handle on the public API surfaces
398+
// as ReceiptHandleIsInvalid.
399+
func decodeClientReceiptHandle(raw string) (*decodedReceiptHandle, error) {
400+
handle, err := decodeReceiptHandle(raw)
401+
if err != nil {
402+
return nil, err
403+
}
404+
if handle.Version != sqsReceiptHandleVersion1 {
405+
// v2 codec is added but dormant until PR 5b. Reject any
406+
// non-v1 handle on the public API so the wire format
407+
// does not leak.
408+
return nil, errors.New("receipt handle version is not yet enabled on the public API")
409+
}
410+
return handle, nil
411+
}
412+
371413
func decodeReceiptHandleV2(b []byte) (*decodedReceiptHandle, error) {
372414
if len(b) != sqsReceiptHandleV2Size {
373415
return nil, errors.New("receipt handle length or version mismatch")
@@ -1494,7 +1536,7 @@ func (s *SQSServer) parseQueueAndReceipt(queueUrl, receiptHandle string) (string
14941536
if err != nil {
14951537
return "", nil, err
14961538
}
1497-
handle, err := decodeReceiptHandle(receiptHandle)
1539+
handle, err := decodeClientReceiptHandle(receiptHandle)
14981540
if err != nil {
14991541
return "", nil, newSQSAPIError(http.StatusBadRequest, sqsErrReceiptHandleInvalid, "receipt handle is not parseable")
15001542
}

adapter/sqs_messages_batch.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ func (s *SQSServer) deleteMessageBatch(w http.ResponseWriter, r *http.Request) {
467467
// retry-bound stale-is-success delete that single DeleteMessage
468468
// uses. Per-entry isolation matches AWS, where a malformed
469469
// handle in slot 3 must not poison slot 4.
470-
handle, decodeErr := decodeReceiptHandle(entry.ReceiptHandle)
470+
handle, decodeErr := decodeClientReceiptHandle(entry.ReceiptHandle)
471471
if decodeErr != nil {
472472
failed = append(failed, sqsBatchResultErrorEntry{
473473
Id: entry.Id,
@@ -567,7 +567,7 @@ func (s *SQSServer) applyChangeVisibilityBatchEntry(ctx context.Context, queueNa
567567
SenderFault: true,
568568
}
569569
}
570-
handle, decodeErr := decodeReceiptHandle(entry.ReceiptHandle)
570+
handle, decodeErr := decodeClientReceiptHandle(entry.ReceiptHandle)
571571
if decodeErr != nil {
572572
return false, sqsBatchResultErrorEntry{
573573
Id: entry.Id,

adapter/sqs_receipt_handle_v2_test.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,3 +264,67 @@ func TestDecodeReceiptHandle_RejectsBase64Garbage(t *testing.T) {
264264
require.Error(t, err,
265265
"non-base64 input must fail at the base64 decode step")
266266
}
267+
268+
// TestDecodeClientReceiptHandle_AcceptsV1 pins the public-API
269+
// wrapper's happy path — v1 handles flow through unchanged.
270+
func TestDecodeClientReceiptHandle_AcceptsV1(t *testing.T) {
271+
t.Parallel()
272+
token := make([]byte, sqsReceiptTokenBytes)
273+
h, err := encodeReceiptHandle(7, "deadbeefdeadbeefdeadbeefdeadbeef", token)
274+
require.NoError(t, err)
275+
back, err := decodeClientReceiptHandle(h)
276+
require.NoError(t, err)
277+
require.Equal(t, sqsReceiptHandleVersion1, back.Version)
278+
require.Equal(t, uint64(7), back.QueueGeneration)
279+
}
280+
281+
// TestDecodeClientReceiptHandle_RejectsV2 pins the codex/coderabbit
282+
// major fix on PR #724 round 1: the v2 codec is added but
283+
// dormant. A client-supplied v2 handle MUST be rejected at the
284+
// public API boundary so the wire format does not leak before
285+
// PR 5b wires the partitioned-FIFO data plane.
286+
//
287+
// Without this gate, a malicious / curious client could re-encode
288+
// a legitimately-issued v1 handle's (queue_gen, message_id,
289+
// receipt_token) under the v2 layout, and DeleteMessage /
290+
// ChangeMessageVisibility would accept it (since downstream
291+
// validation only checks queue_gen + receipt_token). PR 5b
292+
// replaces this gate with a queue-aware version (v1 required on
293+
// non-partitioned queues, v2 required on partitioned ones), so
294+
// the gate-removal lands together with the partitioned data plane.
295+
func TestDecodeClientReceiptHandle_RejectsV2(t *testing.T) {
296+
t.Parallel()
297+
token := make([]byte, sqsReceiptTokenBytes)
298+
h, err := encodeReceiptHandleV2(3, 7, "deadbeefdeadbeefdeadbeefdeadbeef", token)
299+
require.NoError(t, err,
300+
"v2 encoder must succeed even though the public API "+
301+
"wrapper rejects the result — the codec is dormant, "+
302+
"not absent")
303+
304+
// Low-level decoder still accepts v2 (it's pure codec).
305+
back, err := decodeReceiptHandle(h)
306+
require.NoError(t, err)
307+
require.Equal(t, sqsReceiptHandleVersion2, back.Version,
308+
"low-level decodeReceiptHandle must keep working — the "+
309+
"gate is at the public API boundary, not in the codec")
310+
311+
// Public API wrapper rejects v2.
312+
_, err = decodeClientReceiptHandle(h)
313+
require.Error(t, err,
314+
"v2 handle on the public API must fail until PR 5b lifts "+
315+
"the dormancy gate")
316+
require.Contains(t, err.Error(), "not yet enabled")
317+
}
318+
319+
// TestDecodeClientReceiptHandle_PassesThroughDecodeErrors pins
320+
// that decode-error propagation is unchanged — a malformed blob
321+
// still surfaces the underlying base64 / length error rather than
322+
// being masked by the dormancy-gate message.
323+
func TestDecodeClientReceiptHandle_PassesThroughDecodeErrors(t *testing.T) {
324+
t.Parallel()
325+
_, err := decodeClientReceiptHandle("!!!" + strings.Repeat("?", 50))
326+
require.Error(t, err)
327+
require.NotContains(t, err.Error(), "not yet enabled",
328+
"a decode-step error must NOT be reported as the "+
329+
"dormancy-gate message")
330+
}

0 commit comments

Comments
 (0)