Skip to content

Adapter audit follow-ups from #19: add round-trip tests, comments and remove dead fields #1907

Open

Description

@cristim

Context

Part of #19. The audit in docs/audits/api-adapter-audit-2026-11-09.md (audited commit 72d90d54b06769710b7c49094365892d9a4d4ab0) found no new data-loss bug beyond those already fixed (#1788, #1905, #1906) or tracked (#1776, #923). It leaves three kinds of follow-up work, listed below. No code change was in scope for the audit.

This issue is unclaimed on purpose: pick any item, one commit per item or group.

Round-trip test here means a test that asserts every field of the pair at the callee boundary (fixture store, SQL or outbound client), not only the converter's output. Pair ids (A1, B2, E5, ...) are the ones used in the audit.

1. Pairs with no round-trip test

  • A1 CloudAccountRequest -> createAccount: internal/api/handler_accounts.go:357, converter :548, store internal/config/store_postgres.go:4049. Only the converter guard exists.
  • A2 CloudAccountRequest -> updateAccount: handler_accounts.go:626, converter :655, store store_postgres.go:4200.
  • A3 CloudAccountRequest -> createSelfAccount: handler_accounts.go:284-296.
  • A4 CredentialsRequest -> saveAccountCredentials: handler_accounts.go:802, SaveCredential at :839; the test asserts success only.
  • A5 DiscoverOrgRequest -> discoverOrgAccounts: handler_accounts.go:1661.
  • R1 RegistrationRequest -> submitRegistration: internal/api/handler_registrations.go:91; credential fields are allowlisted in the guard.
  • R2 CloudAccountRequest -> approveRegistration: handler_registrations.go:352; add a case with an explicit enabled false.
  • R3 RejectRequest -> rejectRegistration: handler_registrations.go:402.
  • P1 {account_ids} -> setPlanAccounts: handler_accounts.go:1461; the test asserts success only.
  • B1 PlanRequest -> createPlan: internal/api/handler_plans.go:168; only SetPlanAccounts arguments are asserted.
  • B2 PlanRequest -> updatePlan: handler_plans.go:319, :327; the update path for description and target_accounts is unguarded (fix(api): request fields accepted and silently ignored (#19 audit follow-ups) #1776).
  • B5 config.ServiceConfig -> updateServiceConfig: internal/api/handler_config.go:499.
  • E1 ExchangeQuoteRequestBody -> getExchangeQuote: internal/api/handler_ri_exchange.go:1956; targets[].count, ri_ids and target_count have no assertion.
  • E2 ExchangeExecuteRequestBody -> executeExchange: handler_ri_exchange.go:2029; only the audit row is asserted, not the library request.
  • E3 AzureCompatibleOfferingsRequestBody -> getAzureCompatibleOfferings: handler_ri_exchange.go:981; the Azure call is mocked with mock.Anything.
  • E4 AzureExecuteExchangeRequestBody -> executeAzureExchange: handler_ri_exchange.go:1222; only the audit row is asserted.
  • E5 RIExchangeConfigUpdateRequest -> updateRIExchangeConfig: handler_ri_exchange.go:2305; no stored-value assertion for any of the six fields.
  • E9 ExecutePurchaseRequest -> executePurchase: internal/api/handler_purchases.go:3233; capacity_percent persistence and the execute_mode branch.
  • U1 LoginRequest -> login: internal/api/handler_auth.go:30, internal/server/app.go:1178-1184; TestLogin_MFATokenRoundTrip asserts mfa_token only.
  • U3 to U10 (PasswordResetRequest, PasswordResetConfirm, ProfileUpdateRequest, ChangePasswordRequest, MFA setup, enable, disable, regenerate): handler_auth.go:414, :505, :545, :649, :719, :743, :764, :789; only handler-boundary argument tests exist, and the adapter smoke tests cover error paths.
  • Consider extending requireFieldsReach (internal/api/adapter_field_drop_test.go:96-122) to the exchange, resolve and settle request types, or add one reflection guard for authServiceAdapter (internal/server/app.go:1178-1251).

2. Deliberately ignored fields with no source comment: add a comment

  • Service-config PUT provider and service in the body are overwritten by the URL path: internal/api/handler_config.go:510-514.
  • Ladder config created_at and updated_at are accepted and replaced by NOW(): internal/api/handler_ladder.go:102, internal/config/store_postgres_ladder.go:105,118.
  • acknowledge_unknown_outcome on outcome completed is accepted and not stored: internal/api/handler_ri_exchange_resolve.go:43-45, :247-249; handler_ri_exchange_direct_resolve_request.go:14-17, :73-74. The comments state the requirement for failed only.
  • PlanRequest.description and the update-path target_accounts: internal/api/types.go:890-907, handler_plans.go:431-469. Only the test allowlist (adapter_field_drop_test.go:145-146) mentions them. Skip if fix(api): request fields accepted and silently ignored (#19 audit follow-ups) #1776 is fixed first.
  • ExecutePurchaseRequest.recommendations[] fields replaced from the stored row rely on one shared comment (handler_purchases.go:3146-3150); optional per-field pointer for capacity_percent and execute_mode.

3. Dead fields: remove the field

  • CreateUserRequest, UpdateUserRequest, CreateGroupRequest, UpdateGroupRequest: internal/api/types.go:377, :384, :417, :425. Referenced by no Go code; live decode targets are auth.APICreateUserRequest, APIUpdateUserRequest, APICreateGroupRequest, APIUpdateGroupRequest (internal/auth/service_api.go:93, :124, :131, :151).
  • OpenAPI role on the CreateUserRequest and UpdateUserRequest schemas: internal/api/openapi.yaml:6723, :6741. No decode type has it (removed in fix(analytics): partition bounds depend on session TimeZone (DST shifts month boundaries) #912).

Other findings

  • PUT /api/config accepts ri_exchange_utilization_threshold 0 and stores 95: internal/config/validation.go:351-353, internal/config/store_postgres.go:333-336. The comment at validation.go:332-335 claims parity with the RI exchange route, which fix(api): reject an unknown ramp_schedule and a zero utilization_threshold #1906 changed. Proposed fix: reject 0 or store it, and make the comment true.
  • Marketplace price_schedule[].term_months and price are not traced into cloud-commitments-go: internal/api/handler_marketplace_attempt.go:83, :199-204. Proposed fix: assert the fields on the fake EC2 client input in the existing test (handler_marketplace_attempt_test.go:441) or add a test in the module.
  • acknowledge_unknown_outcome on completed (E6, E7): decide whether a true value should be rejected with 400 instead of ignored, handler_ri_exchange_resolve.go:237-249, handler_ri_exchange_direct_resolve_request.go:73-74. If it is rejected, the comment item above is not needed.
  • internal/server/handler.go:453 ScheduledEvent is classed as not an adapter, yet it copies fields into ScheduledTaskParams; source, detail-type and detail are unused after the decode (handler.go:451-484). Proposed fix: a one-line comment at the decode.
  • Ladder config provider is accepted but not forwarded to the engine, whose scope is hard-coded to AWS: internal/server/handler_ladder.go:598. Proposed fix: reject a non-AWS provider or comment it.
  • PUT /api/plans/{id} resets notification_days_before, enabled and auto_purchase when omitted (internal/api/types.go:857,864,865) and ignores a lone provider or service (types.go:954-956). Proposed fix: say so in internal/api/openapi.yaml, or return 400 for a lone provider or service.
  • Plain json.Unmarshal decoders accept unknown keys: handler_ri_exchange.go:981, :1222, :1956, :2029, :2305, handler_purchases_revoke.go:296, handler_purchases.go:3233; the resolve, direct-resolve and settle decoders reject them. Proposed fix: decide on DisallowUnknownFields for the exchange bodies (a client typo in a cap field is currently ignored).
  • Azure billing_scope_id on execute is recorded in the audit targets JSON with no test: internal/api/ri_exchange_audit.go:251. Proposed fix: add an audit-row assertion for a spelled-out scope.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions