Merge remote-tracking branch 'origin/feat/backup-phase0a-keymap-manifest' into feat/backup-phase0a-redis-simple

Author: bootjp

internal/backup/filename.go

@@ -81,33 +81,64 @@ var ErrShaFallbackNeedsKeymap = errors.New("backup: filename uses SHA fallback;
 // inputs.
 //
 // The encoding is deterministic and idempotent given the same input.
+//
+// Two short-circuits ensure the encoder never trips its own invariants:
+//
+//   - If raw is so large that percent-encoding it would always overflow
+//     maxSegmentBytes (3*len(raw) > maxSegmentBytes), we go straight to
+//     shaFallback without allocating the full expansion. Without this an
+//     adversarial caller could force a very large transient allocation
+//     just to discard it.
+//   - If the percent-encoded form happens to match the SHA-fallback shape
+//     (32 hex chars followed by "__"), we promote it to a real
+//     SHA-fallback so DecodeSegment's structural detection cannot
+//     misclassify a legitimate key. Both isShaFallback and shaFallback
+//     are true on the resulting output, so KEYMAP.jsonl carries the
+//     original bytes for exact-byte recovery.
 func EncodeSegment(raw []byte) string {
+	if len(raw)*percentEncodeMaxExpansion > maxSegmentBytes {
+		return shaFallback(raw)
+	}
 	encoded := percentEncode(raw)
-	if len(encoded) <= maxSegmentBytes {
-		return encoded
+	if len(encoded) > maxSegmentBytes || isShaFallback(encoded) {
+		return shaFallback(raw)
 	}
-	return shaFallback(raw)
+	return encoded
 }
 
 // EncodeBinarySegment encodes a DynamoDB B-attribute (binary) segment as
 // "b64.<base64url-no-padding>" so that binary keys never collide with string
 // keys whose hex-encoding happens to look like base64.
 //
-// b64-encoded segments take the SHA fallback if they exceed maxSegmentBytes
-// after the base64 expansion (~4/3 of the raw length).
+// Short-circuits the SHA-fallback for inputs whose base64 expansion (~4/3 of
+// the raw length, plus the 4-byte "b64." prefix) would always overflow
+// maxSegmentBytes. As with EncodeSegment, this avoids an unnecessary large
+// allocation when the result would have been discarded anyway.
 func EncodeBinarySegment(raw []byte) string {
+	if base64.RawURLEncoding.EncodedLen(len(raw))+len(binaryPrefix) > maxSegmentBytes {
+		return shaFallback(raw)
+	}
 	enc := binaryPrefix + base64.RawURLEncoding.EncodeToString(raw)
-	if len(enc) <= maxSegmentBytes {
-		return enc
+	if len(enc) > maxSegmentBytes {
+		return shaFallback(raw)
 	}
-	return shaFallback(raw)
+	return enc
 }
 
 // DecodeSegment is the inverse of EncodeSegment for percent-encoded and
 // binary-prefixed inputs. SHA-fallback inputs return ErrShaFallbackNeedsKeymap
 // so the caller knows to consult KEYMAP.jsonl rather than treat the partial
 // suffix as the original key.
+//
+// As a defensive measure DecodeSegment refuses inputs longer than
+// maxSegmentBytes. EncodeSegment never produces such inputs, so any caller
+// passing one is either reading a corrupted dump or has a bug; either way the
+// percentDecode allocation should not run.
 func DecodeSegment(seg string) ([]byte, error) {
+	if len(seg) > maxSegmentBytes {
+		return nil, errors.Wrapf(ErrInvalidEncodedSegment,
+			"segment length %d exceeds maximum %d", len(seg), maxSegmentBytes)
+	}
 	if isShaFallback(seg) {
 		return nil, errors.WithStack(ErrShaFallbackNeedsKeymap)
 	}
@@ -121,6 +152,10 @@ func DecodeSegment(seg string) ([]byte, error) {
 	return percentDecode(seg)
 }
 
+// percentEncodeMaxExpansion is the worst-case ratio of encoded length to
+// raw length for percentEncode (every byte expands to "%HH").
+const percentEncodeMaxExpansion = 3
+
 // IsShaFallback reports whether seg uses the SHA-prefix-and-truncated-original
 // form. Such segments cannot be reversed without KEYMAP.jsonl.
 func IsShaFallback(seg string) bool {

internal/backup/filename_test.go

@@ -309,6 +309,72 @@ func TestEncodeBinarySegment_FuzzRoundTripIfNotShaFallback(t *testing.T) {
 	})
 }
 
+func TestEncodeSegment_KeyMatchingShaFallbackShapeIsPromotedToFallback(t *testing.T) {
+	t.Parallel()
+	// A user key that is itself made of 32 hex chars + "__" + suffix
+	// would, under naive encoding, return the raw bytes unchanged
+	// (everything is unreserved) — but DecodeSegment's structural
+	// detection would then misclassify it as a SHA-fallback and
+	// return ErrShaFallbackNeedsKeymap. EncodeSegment must promote
+	// such inputs to a real SHA-fallback so the encoded->decoded
+	// invariant holds (decode refuses; KEYMAP carries the original).
+	raw := []byte("0123456789abcdef0123456789abcdef__suffix")
+	enc := EncodeSegment(raw)
+	if !IsShaFallback(enc) {
+		t.Fatalf("expected SHA fallback for collision-shaped input, got %q", enc)
+	}
+	// The fallback's hex prefix must be the SHA of the raw bytes,
+	// NOT the raw bytes' first 32 chars. That way a KEYMAP entry
+	// keyed on `enc` carries the actual original — not a structural
+	// echo.
+	if _, err := DecodeSegment(enc); !errors.Is(err, ErrShaFallbackNeedsKeymap) {
+		t.Fatalf("decode of promoted fallback: err=%v want ErrShaFallbackNeedsKeymap", err)
+	}
+}
+
+func TestEncodeSegment_HugeInputDoesNotMaterialiseFullExpansion(t *testing.T) {
+	t.Parallel()
+	// A 1 MiB input would, if percent-encoded eagerly, allocate 3
+	// MiB before the length check fired. The early short-circuit
+	// must skip that allocation. We can't directly observe the
+	// allocation here without a profile, but we can assert the
+	// output is correct (SHA fallback, length under the ceiling)
+	// and that the call returns promptly enough to be a no-op
+	// guard in profile-runs.
+	raw := make([]byte, 1<<20) // 1 MiB
+	for i := range raw {
+		raw[i] = byte(i)
+	}
+	enc := EncodeSegment(raw)
+	if !IsShaFallback(enc) {
+		t.Fatalf("expected SHA fallback for huge input")
+	}
+	if len(enc) > maxSegmentBytes {
+		t.Fatalf("encoded len %d > max %d", len(enc), maxSegmentBytes)
+	}
+}
+
+func TestDecodeSegment_RejectsOversizedInput(t *testing.T) {
+	t.Parallel()
+	too := strings.Repeat("a", maxSegmentBytes+1)
+	_, err := DecodeSegment(too)
+	if !errors.Is(err, ErrInvalidEncodedSegment) {
+		t.Fatalf("err=%v want ErrInvalidEncodedSegment for oversized input", err)
+	}
+}
+
+func TestEncodeBinarySegment_HugeInputTakesShaFallbackWithoutEncoding(t *testing.T) {
+	t.Parallel()
+	raw := make([]byte, 1<<20) // 1 MiB
+	enc := EncodeBinarySegment(raw)
+	if !IsShaFallback(enc) {
+		t.Fatalf("expected SHA fallback for huge binary input, got %q", enc[:min(40, len(enc))])
+	}
+	if len(enc) > maxSegmentBytes {
+		t.Fatalf("encoded len %d > max %d", len(enc), maxSegmentBytes)
+	}
+}
+
 func TestEncodeSegment_ShaFallbackEmbedsRecognisableSuffix(t *testing.T) {
 	t.Parallel()
 	// The truncated suffix in the SHA-fallback rendering must be derivable

internal/backup/keymap.go

@@ -155,6 +155,11 @@ func NewKeymapReader(r io.Reader) *KeymapReader {
 // (zero, false, nil) at end of stream, and (zero, false, err) on parse
 // failure or I/O error. Once an error is returned the reader is sticky:
 // subsequent calls return the same error.
+//
+// The base64-encoded `original` field is validated at parse time rather
+// than lazily: a malformed dump must surface on the first read of the
+// affected line, not propagate silently until a much later
+// rec.Original() call. Same error class either way.
 func (r *KeymapReader) Next() (KeymapRecord, bool, error) {
 	if r.err != nil {
 		return KeymapRecord{}, false, r.err
@@ -176,6 +181,10 @@ func (r *KeymapReader) Next() (KeymapRecord, bool, error) {
 		r.err = errors.Wrap(ErrInvalidKeymapRecord, "missing encoded or kind")
 		return KeymapRecord{}, false, r.err
 	}
+	if _, err := base64.RawURLEncoding.DecodeString(rec.OriginalB64); err != nil {
+		r.err = errors.Wrap(ErrInvalidKeymapRecord, err.Error())
+		return KeymapRecord{}, false, r.err
+	}
 	return rec, true, nil
 }
 

internal/backup/keymap_test.go

@@ -157,13 +157,13 @@ func TestKeymapReader_RejectsRecordWithoutEncodedOrKind(t *testing.T) {
 	}
 }
 
-func TestKeymapReader_AcceptsBlankLinesByPolicy(t *testing.T) {
+func TestKeymapReader_RejectsBlankLines(t *testing.T) {
 	t.Parallel()
 	// bufio.Scanner skips trailing newline but emits an empty line when one
 	// is in the middle of the stream. We require strict JSONL — every
-	// non-empty line must be a record. An empty line in the middle should
-	// surface as ErrInvalidKeymapRecord rather than silently skipped, so
-	// truncated dumps are recognised.
+	// non-empty line must be a record. An empty line in the middle must
+	// surface as ErrInvalidKeymapRecord rather than be silently skipped,
+	// so truncated dumps are recognised.
 	input := `{"encoded":"x","original":"AA","kind":"sha-fallback"}` + "\n\n" +
 		`{"encoded":"y","original":"AA","kind":"sha-fallback"}` + "\n"
 	r := NewKeymapReader(strings.NewReader(input))
@@ -203,3 +203,17 @@ func TestKeymapRecord_OriginalRejectsBadBase64(t *testing.T) {
 		t.Fatalf("err = %v, want ErrInvalidKeymapRecord", err)
 	}
 }
+
+func TestKeymapReader_RejectsMalformedBase64AtParseTime(t *testing.T) {
+	t.Parallel()
+	// JSON parses fine; the structural fields are present; only the
+	// `original` base64 is malformed. The reader must catch this on
+	// the first Next() rather than defer it to a later Original()
+	// call — Codex P1 #179.
+	input := `{"encoded":"x","original":"!!!","kind":"sha-fallback"}` + "\n"
+	r := NewKeymapReader(strings.NewReader(input))
+	_, _, err := r.Next()
+	if !errors.Is(err, ErrInvalidKeymapRecord) {
+		t.Fatalf("err=%v want ErrInvalidKeymapRecord on parse-time base64 validation", err)
+	}
+}

internal/backup/manifest.go

@@ -87,14 +87,27 @@ type Live struct {
 	PinTokenSHA256 string `json:"pin_token_sha256,omitempty"`
 }
 
-// Adapters lists which scopes were dumped per adapter. An empty slice
-// means "no scopes for this adapter were dumped"; a nil slice means
-// "this adapter was not in the dump's scope filter."
+// Adapters lists which scopes were dumped per adapter. The pointer
+// values express two distinguishable on-disk states:
+//
+//   - nil   -> the adapter was excluded from this dump (e.g.
+//     `--adapter dynamodb,s3` filtered it out). The corresponding
+//     JSON key is absent.
+//   - non-nil pointer to Adapter{}  -> the adapter was in scope but
+//     no scopes for it were emitted (no tables, no buckets, etc.).
+//     The JSON key is present with an empty object.
+//   - non-nil pointer to a populated Adapter -> the listed scopes
+//     were emitted.
+//
+// Storing pointers (rather than zero-value Adapter structs) is what
+// keeps "excluded by filter" distinguishable from "included but
+// empty" through json.Marshal — non-pointer fields would collapse
+// both states into the same on-disk shape.
 type Adapters struct {
-	DynamoDB Adapter `json:"dynamodb"`
-	S3       Adapter `json:"s3"`
-	Redis    Adapter `json:"redis"`
-	SQS      Adapter `json:"sqs"`
+	DynamoDB *Adapter `json:"dynamodb,omitempty"`
+	S3       *Adapter `json:"s3,omitempty"`
+	Redis    *Adapter `json:"redis,omitempty"`
+	SQS      *Adapter `json:"sqs,omitempty"`
 }
 
 // Adapter holds the scope identifiers for one adapter. Field names are
@@ -193,6 +206,17 @@ func ReadManifest(r io.Reader) (Manifest, error) {
 	if err := dec.Decode(&m); err != nil {
 		return Manifest{}, errors.Wrap(ErrInvalidManifest, err.Error())
 	}
+	// MANIFEST.json is exactly one JSON object. Trailing bytes
+	// (a second object, junk, even whitespace-only padding) point at
+	// concatenation bugs or partial-write corruption — both of which
+	// must surface here rather than be silently discarded. We use
+	// io.Discard rather than parsing because we only care that
+	// nothing-decodable is present; structural validation lives in
+	// validate().
+	if dec.More() {
+		return Manifest{}, errors.Wrap(ErrInvalidManifest,
+			"trailing bytes after manifest JSON object")
+	}
 	if m.FormatVersion == 0 {
 		return Manifest{}, errors.Wrapf(ErrUnsupportedFormatVersion,
 			"format_version is zero")

internal/backup/manifest_test.go

@@ -20,10 +20,10 @@ func TestManifest_Phase0RoundTrip(t *testing.T) {
 	m.LastCommitTS = 4517352099840000
 	m.Source = &Source{FSMPath: "/data/fsm-snap/0000000000000064.fsm", FSMCRC32C: "deadbeef"}
 	m.Adapters = Adapters{
-		DynamoDB: Adapter{Tables: []string{"orders", "users"}},
-		S3:       Adapter{Buckets: []string{"photos"}},
-		Redis:    Adapter{Databases: []uint32{0}},
-		SQS:      Adapter{Queues: []string{"orders-fifo.fifo"}},
+		DynamoDB: &Adapter{Tables: []string{"orders", "users"}},
+		S3:       &Adapter{Buckets: []string{"photos"}},
+		Redis:    &Adapter{Databases: []uint32{0}},
+		SQS:      &Adapter{Queues: []string{"orders-fifo.fifo"}},
 	}
 	m.Exclusions = Exclusions{} // all defaults
 
@@ -205,6 +205,61 @@ func TestNewPhase0SnapshotManifest_DefaultsArePopulated(t *testing.T) {
 	}
 }
 
+func TestReadManifest_RejectsTrailingBytes(t *testing.T) {
+	t.Parallel()
+	// Two manifests concatenated; the second must surface as a
+	// trailing-bytes error rather than be silently discarded — Codex
+	// P2 #194.
+	m := NewPhase0SnapshotManifest(time.Now())
+	body, err := json.Marshal(m)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	bad := append([]byte{}, body...)
+	bad = append(bad, body...)
+	_, err = ReadManifest(bytes.NewReader(bad))
+	if !errors.Is(err, ErrInvalidManifest) {
+		t.Fatalf("err=%v want ErrInvalidManifest on trailing bytes", err)
+	}
+}
+
+func TestReadManifest_RejectsTrailingNonWhitespace(t *testing.T) {
+	t.Parallel()
+	m := NewPhase0SnapshotManifest(time.Now())
+	body, err := json.Marshal(m)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	bad := append([]byte{}, body...)
+	bad = append(bad, []byte("garbage")...)
+	_, err = ReadManifest(bytes.NewReader(bad))
+	if !errors.Is(err, ErrInvalidManifest) {
+		t.Fatalf("err=%v want ErrInvalidManifest on trailing garbage", err)
+	}
+}
+
+func TestAdaptersStruct_NilVsEmptyDistinguishedOnDisk(t *testing.T) {
+	t.Parallel()
+	// Gemini #98: an excluded adapter (nil pointer) must serialize
+	// differently from an included-but-empty adapter (non-nil pointer
+	// to Adapter{}).
+	excluded := Adapters{
+		DynamoDB: &Adapter{}, // present, no scopes
+		// S3 / Redis / SQS left nil — out of scope
+	}
+	body, err := json.Marshal(excluded)
+	if err != nil {
+		t.Fatal(err)
+	}
+	out := string(body)
+	if !strings.Contains(out, `"dynamodb":{}`) {
+		t.Fatalf("included-empty must serialise as `dynamodb:{}`, got %s", out)
+	}
+	if strings.Contains(out, `"s3"`) || strings.Contains(out, `"redis"`) || strings.Contains(out, `"sqs"`) {
+		t.Fatalf("excluded adapters must be omitted, got %s", out)
+	}
+}
+
 func TestWriteManifest_ProducesPrettyJSON(t *testing.T) {
 	t.Parallel()
 	m := NewPhase0SnapshotManifest(time.Now())