Skip to content

Commit e045bfb

Browse files
committed
admin: harden forwarded delete (NUL + slash) (Codex P2 ×2)
Two related findings on PR #635 / forward_server.go's handleDelete: - NUL-byte smuggling: handleDelete decoded with goccy/go-json but skipped the explicit NUL scan that decodeCreateTableRequest applies. Same vector as the #634 fix — `{"name":"users"}\x00{"extra":1}` passes dec.More() because goccy treats NUL as end-of-input. Add the same pre-decode NUL rejection. - Slash-in-name divergence: the HTTP handleDelete and handleDescribe both reject `/` in the table name with 404, but the forwarded delete just passed body.Name straight through to AdminDeleteTable. A forwarded call could therefore act on slash-bearing tables that a leader-direct call would 404. Reject symmetrically before invoking the source. Tests: two new ForwardServer cases — NUL payload + slash name. Both confirm the source is never invoked when the precondition fails (defence in depth — an asymmetric stub source would still make the test green if we only checked the response code).
1 parent 016b1db commit e045bfb

2 files changed

Lines changed: 54 additions & 0 deletions

File tree

internal/admin/forward_server.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"errors"
77
"log/slog"
88
"net/http"
9+
"strings"
910

1011
pb "github.com/bootjp/elastickv/proto"
1112
"github.com/goccy/go-json"
@@ -171,6 +172,14 @@ func (s *ForwardServer) handleDelete(ctx context.Context, principal AuthPrincipa
171172
return rejectForward(http.StatusRequestEntityTooLarge, "payload_too_large",
172173
"forwarded payload exceeds the 64 KiB admin limit")
173174
}
175+
// Mirror decodeCreateTableRequest's NUL-byte guard: goccy/go-json
176+
// treats raw NUL as end-of-input so dec.More() would otherwise
177+
// miss `{"name":"users"}\x00{"extra":1}` payloads. Codex P2 on
178+
// PR #635 flagged this as the same smuggling vector that the
179+
// HTTP create path already covers.
180+
if bytes.IndexByte(payload, 0) >= 0 {
181+
return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload contains a NUL byte")
182+
}
174183
dec := json.NewDecoder(bytes.NewReader(payload))
175184
dec.DisallowUnknownFields()
176185
var body struct {
@@ -185,6 +194,14 @@ func (s *ForwardServer) handleDelete(ctx context.Context, principal AuthPrincipa
185194
if body.Name == "" {
186195
return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload missing name")
187196
}
197+
// Reject slash-bearing names symmetrically with the HTTP
198+
// handleDelete and handleDescribe paths. Without this, a
199+
// forwarded call could act on `foo/bar` while a leader-direct
200+
// call would 404 — divergent behaviour Codex P2 flagged on
201+
// PR #635.
202+
if strings.ContainsRune(body.Name, '/') {
203+
return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload name must not contain '/'")
204+
}
188205
if err := s.source.AdminDeleteTable(ctx, principal, body.Name); err != nil {
189206
return forwardErrorResponse(err), nil
190207
}

internal/admin/forward_server_test.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,43 @@ func TestForwardServer_DeleteTable_MissingReturns404(t *testing.T) {
151151
require.Equal(t, int32(http.StatusNotFound), resp.GetStatusCode())
152152
}
153153

154+
// TestForwardServer_DeleteTable_RejectsNULBytePayload exercises
155+
// the same Codex P2 vector that the create-table path covers:
156+
// goccy/go-json treats raw NUL as end-of-input, so a body like
157+
// `{"name":"users"}\x00{"extra":1}` would otherwise pass dec.More()
158+
// undetected. The handler now scans for NUL before decoding.
159+
func TestForwardServer_DeleteTable_RejectsNULBytePayload(t *testing.T) {
160+
src := &stubTablesSource{tables: map[string]*DynamoTableSummary{"users": {Name: "users"}}}
161+
srv := newForwardServerForTest(src, fullPrincipalRoleStore())
162+
resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
163+
Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
164+
Operation: pb.AdminOperation_ADMIN_OP_DELETE_TABLE,
165+
Payload: []byte("{\"name\":\"users\"}\x00{\"extra\":1}"),
166+
})
167+
require.NoError(t, err)
168+
require.Equal(t, int32(http.StatusBadRequest), resp.GetStatusCode())
169+
require.Contains(t, string(resp.GetPayload()), "NUL byte")
170+
// Source must not be reached — the table still exists in the
171+
// stub map after the rejection.
172+
require.Contains(t, src.tables, "users")
173+
}
174+
175+
// TestForwardServer_DeleteTable_RejectsSlashInName mirrors the
176+
// HTTP path's slash rejection on forwarded delete requests so the
177+
// two surfaces cannot diverge (Codex P2).
178+
func TestForwardServer_DeleteTable_RejectsSlashInName(t *testing.T) {
179+
src := &stubTablesSource{tables: map[string]*DynamoTableSummary{}}
180+
srv := newForwardServerForTest(src, fullPrincipalRoleStore())
181+
resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
182+
Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
183+
Operation: pb.AdminOperation_ADMIN_OP_DELETE_TABLE,
184+
Payload: []byte(`{"name":"foo/bar"}`),
185+
})
186+
require.NoError(t, err)
187+
require.Equal(t, int32(http.StatusBadRequest), resp.GetStatusCode())
188+
require.Contains(t, string(resp.GetPayload()), "must not contain")
189+
}
190+
154191
func TestForwardServer_UnknownOperationRejected(t *testing.T) {
155192
srv := newForwardServerForTest(&stubTablesSource{tables: map[string]*DynamoTableSummary{}}, fullPrincipalRoleStore())
156193
resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{

0 commit comments

Comments
 (0)