From 88a025d0e097b5e42f077f6dc38b4a9d6036306c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 19:31:00 +0100 Subject: [PATCH 1/3] refactor(api): extract registrationFromRequest Pure mapper from RegistrationRequest to AccountRegistration so it can be unit tested. No behaviour change. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_registrations.go | 49 +++++++++++++++------------ 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 1ab2b700d..6613a0d56 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -54,6 +54,33 @@ type RejectRequest struct { Reason string `json:"reason"` } +// registrationFromRequest maps a validated RegistrationRequest to a pending +// config.AccountRegistration. The credential fields are not mapped here: they +// are encrypted and attached by submitRegistration. +func registrationFromRequest(req RegistrationRequest, token string) *config.AccountRegistration { + return &config.AccountRegistration{ + ReferenceToken: token, + Status: "pending", + Provider: req.Provider, + ExternalID: req.ExternalID, + AccountName: req.AccountName, + ContactEmail: req.ContactEmail, + Description: req.Description, + SourceProvider: req.SourceProvider, + AWSRoleARN: req.AWSRoleARN, + AWSAuthMode: req.AWSAuthMode, + AWSExternalID: req.AWSExternalID, + AzureSubscriptionID: req.AzureSubscriptionID, + AzureTenantID: req.AzureTenantID, + AzureClientID: req.AzureClientID, + AzureAuthMode: req.AzureAuthMode, + GCPProjectID: req.GCPProjectID, + GCPClientEmail: req.GCPClientEmail, + GCPAuthMode: req.GCPAuthMode, + GCPWIFAudience: req.GCPWIFAudience, + } +} + // submitRegistration handles POST /api/register. func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { if err := h.checkRateLimit(ctx, req, "register"); err != nil { @@ -74,27 +101,7 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("registrations: generate token: %w", err) } - reg := &config.AccountRegistration{ - ReferenceToken: token, - Status: "pending", - Provider: body.Provider, - ExternalID: body.ExternalID, - AccountName: body.AccountName, - ContactEmail: body.ContactEmail, - Description: body.Description, - SourceProvider: body.SourceProvider, - AWSRoleARN: body.AWSRoleARN, - AWSAuthMode: body.AWSAuthMode, - AWSExternalID: body.AWSExternalID, - AzureSubscriptionID: body.AzureSubscriptionID, - AzureTenantID: body.AzureTenantID, - AzureClientID: body.AzureClientID, - AzureAuthMode: body.AzureAuthMode, - GCPProjectID: body.GCPProjectID, - GCPClientEmail: body.GCPClientEmail, - GCPAuthMode: body.GCPAuthMode, - GCPWIFAudience: body.GCPWIFAudience, - } + reg := registrationFromRequest(body, token) encrypted, encErr := h.encryptRegistrationCredential(body.CredentialPayload) if encErr != nil { From ebaa6348ceac11cd1fc51978991d311cd07dea1d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 19:31:58 +0100 Subject: [PATCH 2/3] test(api): guard request-to-store adapters against dropped fields Reflection test per adapter (CloudAccountRequest, RegistrationRequest, PlanRequest): each json field must change the converted struct unless it is allowlisted with a reason, and a stale allowlist entry fails. Part of #19 Co-Authored-By: Claude Opus 5.5 --- internal/api/adapter_field_drop_test.go | 111 ++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 internal/api/adapter_field_drop_test.go diff --git a/internal/api/adapter_field_drop_test.go b/internal/api/adapter_field_drop_test.go new file mode 100644 index 000000000..fb08a8c84 --- /dev/null +++ b/internal/api/adapter_field_drop_test.go @@ -0,0 +1,111 @@ +package api + +import ( + "encoding/json" + "reflect" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Field-drop guards (#19): a field added to an API request struct must reach +// the store struct, or be listed with a reason in the adapter's allowlist. + +var timestampRE = regexp.MustCompile(`"\d{4}-\d{2}-\d{2}T[^"]*"`) + +// jsonShape renders v with timestamps masked, so two conversions made at +// different moments compare equal unless a field value differs. +func jsonShape(t *testing.T, v any) string { + t.Helper() + b, err := json.Marshal(v) + require.NoError(t, err) + return timestampRE.ReplaceAllString(string(b), `"T"`) +} + +// setDistinct sets f to a non-zero value derived from tag, so it differs from +// a base request that leaves the field unset. +func setDistinct(t *testing.T, f reflect.Value, tag string) { + t.Helper() + switch f.Kind() { + case reflect.String: + f.SetString("v-" + tag) + case reflect.Bool: + f.SetBool(true) + case reflect.Int: + f.SetInt(11) + case reflect.Float64: + f.SetFloat(7.5) + case reflect.Slice: + f.Set(reflect.ValueOf([]string{"v-" + tag})) + case reflect.Ptr: + p := reflect.New(f.Type().Elem()) + setDistinct(t, p.Elem(), tag) + f.Set(p) + default: + t.Fatalf("field %s: unsupported kind %s, extend setDistinct", tag, f.Kind()) + } +} + +// requireFieldsReach sets each json-tagged field of the request on top of +// base, one at a time, and requires convert's output to change unless the +// field is allowlisted. An allowlisted field must NOT change the output, so a +// fixed drop leaves a stale entry that fails. +func requireFieldsReach[Req, Out any](t *testing.T, base Req, convert func(Req) Out, notMapped map[string]string) { + t.Helper() + typ := reflect.TypeOf(base) + tags := map[string]int{} + for i := 0; i < typ.NumField(); i++ { + tag, _, _ := strings.Cut(typ.Field(i).Tag.Get("json"), ",") + if tag != "" && tag != "-" { + tags[tag] = i + } + } + for tag := range notMapped { + _, ok := tags[tag] + require.Truef(t, ok, "allowlist entry %q is not a json field of %s", tag, typ.Name()) + } + baseShape := jsonShape(t, convert(base)) + for tag, idx := range tags { + t.Run(tag, func(t *testing.T) { + req := base + setDistinct(t, reflect.ValueOf(&req).Elem().Field(idx), tag) + changed := jsonShape(t, convert(req)) != baseShape + if reason, skip := notMapped[tag]; skip { + assert.Falsef(t, changed, "%s now reaches the target; remove its allowlist entry (%s)", tag, reason) + return + } + assert.Truef(t, changed, "%s is dropped by the converter: add the copy, or allowlist it with a reason", tag) + }) + } +} + +func TestCloudAccountFromRequest_NoFieldDropped(t *testing.T) { + requireFieldsReach(t, CloudAccountRequest{}, cloudAccountFromRequest, map[string]string{}) +} + +func TestRegistrationFromRequest_NoFieldDropped(t *testing.T) { + requireFieldsReach(t, RegistrationRequest{}, + func(r RegistrationRequest) *config.AccountRegistration { return registrationFromRequest(r, "tok") }, + map[string]string{ + "credential_type": "encrypted and attached by submitRegistration, not the mapper", + "credential_payload": "encrypted and attached by submitRegistration, not the mapper", + }) +} + +func TestPlanRequestToPurchasePlan_NoFieldDropped(t *testing.T) { + // provider, service, term, payment and target_coverage fold into Services; + // the custom ramp base makes custom_step_percent and custom_interval_days + // visible in RampSchedule. + base := PlanRequest{Provider: "aws", Service: "ec2", RampSchedule: "custom"} + requireFieldsReach(t, base, + func(r PlanRequest) *config.PurchasePlan { return r.toPurchasePlan() }, + map[string]string{ + "description": "accepted but dropped, tracked in #1776", + "target_accounts": "handled by createPlan/setPlanAccounts, not the converter", + }) +} From 04e2785bb63312abcebe30d4aa42db86d76f9d6d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 14:49:07 +0100 Subject: [PATCH 3/3] test(api): compare adapter outputs by reflection and key untagged fields The guard compared store structs through JSON, so json:"-" fields such as the registration credential payload were invisible to it, and it skipped untagged exported request fields that encoding/json still binds by name. It now compares with reflect.DeepEqual after zeroing timestamps and keys each field by its JSON name or Go name. Part of #19 Co-Authored-By: Claude Opus 5.5 --- internal/api/adapter_field_drop_test.go | 69 +++++++++++++++++++------ 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/internal/api/adapter_field_drop_test.go b/internal/api/adapter_field_drop_test.go index fb08a8c84..c5839a1dd 100644 --- a/internal/api/adapter_field_drop_test.go +++ b/internal/api/adapter_field_drop_test.go @@ -1,11 +1,10 @@ package api import ( - "encoding/json" "reflect" - "regexp" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,15 +15,54 @@ import ( // Field-drop guards (#19): a field added to an API request struct must reach // the store struct, or be listed with a reason in the adapter's allowlist. -var timestampRE = regexp.MustCompile(`"\d{4}-\d{2}-\d{2}T[^"]*"`) +var timeType = reflect.TypeOf(time.Time{}) -// jsonShape renders v with timestamps masked, so two conversions made at -// different moments compare equal unless a field value differs. -func jsonShape(t *testing.T, v any) string { - t.Helper() - b, err := json.Marshal(v) - require.NoError(t, err) - return timestampRE.ReplaceAllString(string(b), `"T"`) +// zeroTimes clears every settable time.Time reachable from v, so two +// conversions made at different moments compare equal unless a field value +// differs. It compares by reflection, not JSON, so json:"-" store fields +// (credentials, tokens) are seen too. +func zeroTimes(v reflect.Value) { + switch v.Kind() { + case reflect.Ptr, reflect.Interface: + if !v.IsNil() { + zeroTimes(v.Elem()) + } + case reflect.Struct: + if v.Type() == timeType { + if v.CanSet() { + v.Set(reflect.Zero(timeType)) + } + return + } + for i := 0; i < v.NumField(); i++ { + zeroTimes(v.Field(i)) + } + case reflect.Slice, reflect.Array: + for i := 0; i < v.Len(); i++ { + zeroTimes(v.Index(i)) + } + } +} + +func normalized[Out any](out Out) Out { + zeroTimes(reflect.ValueOf(&out).Elem()) + return out +} + +// requestFieldName is the JSON key encoding/json binds the field to: its tag +// name, or its Go name when untagged. Fields json never binds return "". +func requestFieldName(f reflect.StructField) string { + if !f.IsExported() { + return "" + } + name, _, _ := strings.Cut(f.Tag.Get("json"), ",") + switch name { + case "-": + return "" + case "": + return f.Name + } + return name } // setDistinct sets f to a non-zero value derived from tag, so it differs from @@ -51,7 +89,7 @@ func setDistinct(t *testing.T, f reflect.Value, tag string) { } } -// requireFieldsReach sets each json-tagged field of the request on top of +// requireFieldsReach sets each JSON-bindable field of the request on top of // base, one at a time, and requires convert's output to change unless the // field is allowlisted. An allowlisted field must NOT change the output, so a // fixed drop leaves a stale entry that fails. @@ -60,21 +98,20 @@ func requireFieldsReach[Req, Out any](t *testing.T, base Req, convert func(Req) typ := reflect.TypeOf(base) tags := map[string]int{} for i := 0; i < typ.NumField(); i++ { - tag, _, _ := strings.Cut(typ.Field(i).Tag.Get("json"), ",") - if tag != "" && tag != "-" { - tags[tag] = i + if name := requestFieldName(typ.Field(i)); name != "" { + tags[name] = i } } for tag := range notMapped { _, ok := tags[tag] require.Truef(t, ok, "allowlist entry %q is not a json field of %s", tag, typ.Name()) } - baseShape := jsonShape(t, convert(base)) + baseOut := normalized(convert(base)) for tag, idx := range tags { t.Run(tag, func(t *testing.T) { req := base setDistinct(t, reflect.ValueOf(&req).Elem().Field(idx), tag) - changed := jsonShape(t, convert(req)) != baseShape + changed := !reflect.DeepEqual(normalized(convert(req)), baseOut) if reason, skip := notMapped[tag]; skip { assert.Falsef(t, changed, "%s now reaches the target; remove its allowlist entry (%s)", tag, reason) return