From 5e11c21f179fd0848e686b72d9ce23bc52fc17f0 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:50:13 +0000 Subject: [PATCH] sec(api): allow PATCH in CORS and stop returning registration tokens to admins Two of the three low-severity findings grouped in the 2026-09-02 audit issue. A02-015: the CSRF half was fixed by #1104. The CORS half was not: the handler's Access-Control-Allow-Methods and the Lambda Function URL CORS config (Terraform lambda module and the CloudFormation stack) listed GET, POST, PUT and DELETE only, while the router serves PATCH /api/plans/{id} and PATCH /api/ladder/tranches/{id}. All three now list PATCH. A test reads the router table and requires every routed verb (HEAD excepted, it is CORS-safelisted) in the preflight's list. A02-020: config.AccountRegistration.ReferenceToken was tagged json:"reference_token", so GET /api/registrations and GET /api/registrations/{id} returned each registrant's private status token to admins. It is now json:"-"; submitRegistration still returns it from its own explicit map. A03-015 (account scope matching display names) is left to the open group account-scope binding PR #1116, which makes the same change. Refs #310 Co-Authored-By: Claude Opus 5.5 --- cloudformation/stacks/CUDly/template.yaml | 1 + internal/api/cors_methods_test.go | 77 +++++++++++++ internal/api/handler.go | 2 +- .../api/handler_registrations_token_test.go | 107 ++++++++++++++++++ internal/config/types.go | 2 +- .../compute/aws/function_url_cors_test.go | 86 ++++++++++++++ terraform/modules/compute/aws/lambda/main.tf | 2 +- 7 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 internal/api/cors_methods_test.go create mode 100644 internal/api/handler_registrations_token_test.go create mode 100644 terraform/modules/compute/aws/function_url_cors_test.go diff --git a/cloudformation/stacks/CUDly/template.yaml b/cloudformation/stacks/CUDly/template.yaml index b801ef6aa..3e5b45206 100644 --- a/cloudformation/stacks/CUDly/template.yaml +++ b/cloudformation/stacks/CUDly/template.yaml @@ -669,6 +669,7 @@ Resources: - GET - POST - PUT + - PATCH - DELETE AllowHeaders: - Content-Type diff --git a/internal/api/cors_methods_test.go b/internal/api/cors_methods_test.go new file mode 100644 index 000000000..179b364f6 --- /dev/null +++ b/internal/api/cors_methods_test.go @@ -0,0 +1,77 @@ +package api + +import ( + "context" + "strings" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// CORS method list vs the router (issue #310, finding A02-015). +// +// buildResponseHeaders advertised GET, POST, PUT, DELETE and OPTIONS, but the +// router also serves PATCH (PATCH /api/plans/{id}, PATCH +// /api/ladder/tranches/{id}). A cross-origin deployment's preflight for those +// routes was answered without PATCH, so the browser refused the real request. + +// preflightAllowedMethods sends an OPTIONS preflight for path to a handler +// with a configured CORS origin and returns the advertised methods. +func preflightAllowedMethods(t *testing.T, h *Handler, path string) map[string]bool { + t.Helper() + resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "origin": h.corsAllowedOrigin, + "access-control-request-method": "PATCH", + }, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "OPTIONS", Path: path}, + }, + }) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode, resp.Body) + header := resp.Headers["Access-Control-Allow-Methods"] + require.NotEmpty(t, header, "a handler with a CORS origin must advertise its methods") + allowed := map[string]bool{} + for _, m := range strings.Split(header, ",") { + allowed[strings.TrimSpace(m)] = true + } + return allowed +} + +// TestHandleRequest_PreflightAllowsPATCH is the issue's scenario: the +// preflight for PATCH /api/plans/{id} must list PATCH. +func TestHandleRequest_PreflightAllowsPATCH(t *testing.T) { + h := &Handler{corsAllowedOrigin: "https://dashboard.example.com"} + allowed := preflightAllowedMethods(t, h, "/api/plans/"+patchCSRFPlan) + assert.True(t, allowed["PATCH"], "preflight for PATCH /api/plans/{id} does not allow PATCH: %v", allowed) +} + +// TestHandleRequest_PreflightAllowsEveryRoutedMethod reads the real router +// table: every verb a route is registered with must be in the advertised +// list, so a verb a route adds later cannot silently fail cross-origin. +func TestHandleRequest_PreflightAllowsEveryRoutedMethod(t *testing.T) { + h := &Handler{corsAllowedOrigin: "https://dashboard.example.com"} + allowed := preflightAllowedMethods(t, h, "/api/plans/"+patchCSRFPlan) + + routed := map[string]bool{} + for _, route := range NewRouter(h).routes { + // A route with no Method matches any verb and is covered by the + // concrete verbs below. HEAD is a CORS-safelisted method: browsers + // never preflight it, so it need not be advertised. + if route.Method == "" || route.Method == "HEAD" { + continue + } + routed[route.Method] = true + assert.True(t, allowed[route.Method], + "%s is routed with %s but the CORS preflight does not allow it", csrfRouteKey(route), route.Method) + } + // Floor: if the table read finds no PATCH route, the guard is reading the + // wrong table and would pass without checking anything. + for _, m := range []string{"GET", "POST", "PUT", "PATCH", "DELETE"} { + assert.True(t, routed[m], "no %s route found in the router table", m) + } + assert.True(t, allowed["OPTIONS"], "preflight must keep allowing OPTIONS") +} diff --git a/internal/api/handler.go b/internal/api/handler.go index 1bfcb9d99..a26734e9e 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -774,7 +774,7 @@ func (h *Handler) buildResponseHeaders() map[string]string { if h.corsAllowedOrigin != "" { corsHeaders["Access-Control-Allow-Origin"] = h.corsAllowedOrigin - corsHeaders["Access-Control-Allow-Methods"] = "GET, POST, PUT, DELETE, OPTIONS" + corsHeaders["Access-Control-Allow-Methods"] = "GET, POST, PUT, PATCH, DELETE, OPTIONS" corsHeaders["Access-Control-Allow-Headers"] = "Content-Type, X-API-Key, Authorization, X-Authorization, X-CSRF-Token" corsHeaders["Access-Control-Allow-Credentials"] = "true" } diff --git a/internal/api/handler_registrations_token_test.go b/internal/api/handler_registrations_token_test.go new file mode 100644 index 000000000..c7978cdda --- /dev/null +++ b/internal/api/handler_registrations_token_test.go @@ -0,0 +1,107 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// reference_token on the admin registration endpoints (issue #310, finding +// A02-020). +// +// The reference token is the registrant's only credential for +// GET /api/register/{token}. rejectRegistration already built a filtered map +// so the token was "not exposed to admin", but listRegistrations and +// getRegistration returned config.AccountRegistration as-is, and its +// ReferenceToken field was tagged json:"reference_token". + +const ( + regTokenSession = "reg-token-admin" + regTokenID = "22222222-2222-2222-2222-222222222222" + regTokenValue = "reg-token-value-under-test" +) + +func regTokenHandler(t *testing.T) (*Handler, *MockConfigStore) { + t.Helper() + store := new(MockConfigStore) + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, regTokenSession). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + m.grantAdmin() + return &Handler{config: store, auth: m}, store +} + +func regTokenFixture() config.AccountRegistration { + return config.AccountRegistration{ + ID: regTokenID, ReferenceToken: regTokenValue, Status: "pending", Provider: "aws", + AccountName: "Registrant", ContactEmail: "owner@example.com", + CreatedAt: time.Now(), UpdatedAt: time.Now(), + } +} + +func regTokenGet(t *testing.T, h *Handler, path string) *events.LambdaFunctionURLResponse { + t.Helper() + resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer " + regTokenSession}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET", Path: path}, + }, + }) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode, resp.Body) + return resp +} + +// assertNoReferenceToken checks the raw body for both the key and the value, +// and that the registration itself is still there (a fix that returns nothing +// would also hide the token). +func assertNoReferenceToken(t *testing.T, body string) { + t.Helper() + assert.NotContains(t, body, "reference_token", "admin response exposes the reference_token key") + assert.NotContains(t, body, regTokenValue, "admin response exposes the registrant's token value") + assert.Contains(t, body, regTokenID, "admin response must still carry the registration") +} + +func TestListRegistrations_DoesNotExposeReferenceToken(t *testing.T) { + h, store := regTokenHandler(t) + store.On("ListAccountRegistrations", mock.Anything, mock.Anything). + Return([]config.AccountRegistration{regTokenFixture()}, nil) + + resp := regTokenGet(t, h, "/api/registrations") + assertNoReferenceToken(t, resp.Body) + + var regs []map[string]any + require.NoError(t, json.Unmarshal([]byte(resp.Body), ®s)) + require.Len(t, regs, 1) + assert.Equal(t, "pending", regs[0]["status"]) +} + +func TestGetRegistration_DoesNotExposeReferenceToken(t *testing.T) { + h, store := regTokenHandler(t) + reg := regTokenFixture() + store.On("GetAccountRegistration", mock.Anything, regTokenID).Return(®, nil) + + resp := regTokenGet(t, h, "/api/registrations/"+regTokenID) + assertNoReferenceToken(t, resp.Body) + + var got map[string]any + require.NoError(t, json.Unmarshal([]byte(resp.Body), &got)) + assert.Equal(t, "Registrant", got["account_name"]) +} + +// TestAccountRegistration_JSONOmitsReferenceToken pins the type itself, so +// any future endpoint that serializes a registration inherits the redaction. +func TestAccountRegistration_JSONOmitsReferenceToken(t *testing.T) { + b, err := json.Marshal(regTokenFixture()) + require.NoError(t, err) + assert.NotContains(t, string(b), "reference_token") + assert.NotContains(t, string(b), regTokenValue) +} diff --git a/internal/config/types.go b/internal/config/types.go index 4b43d6a0f..a8df04a6b 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -1294,7 +1294,7 @@ type AccountServiceOverride struct { // of the federation IaC, then approved or rejected by a CUDly admin. type AccountRegistration struct { ID string `json:"id"` - ReferenceToken string `json:"reference_token"` + ReferenceToken string `json:"-"` // registrant's status credential; only submitRegistration returns it (#310) Status string `json:"status"` // pending, approved, rejected Provider string `json:"provider"` ExternalID string `json:"external_id"` diff --git a/terraform/modules/compute/aws/function_url_cors_test.go b/terraform/modules/compute/aws/function_url_cors_test.go new file mode 100644 index 000000000..842453115 --- /dev/null +++ b/terraform/modules/compute/aws/function_url_cors_test.go @@ -0,0 +1,86 @@ +// Regression test for issue #310 (finding A02-015): the API serves PATCH +// (PATCH /api/plans/{id}, PATCH /api/ladder/tranches/{id}), but the Lambda +// Function URL CORS config in both the Terraform lambda module and the +// CloudFormation stack listed only GET, POST, PUT and DELETE. When a Function +// URL has a CORS config, AWS answers the preflight itself from that config, +// so a cross-origin PATCH was refused before it reached the handler, whatever +// the handler's own Access-Control-Allow-Methods said. +package aws_test + +import ( + "os" + "path/filepath" + "regexp" + "testing" +) + +// apiRoutedMethods are the verbs internal/api/router.go registers routes +// with. internal/api's TestHandleRequest_PreflightAllowsEveryRoutedMethod +// checks the handler's header against the router table itself. +var apiRoutedMethods = []string{"GET", "POST", "PUT", "PATCH", "DELETE"} + +var ( + tfFunctionURLBlock = regexp.MustCompile(`(?s)resource\s+"aws_lambda_function_url"\s+"main"\s*\{.*?\n\}`) + tfCORSAllowMethods = regexp.MustCompile(`(?m)^\s*allow_methods\s*=\s*\[([^\]]*)\]`) + tfQuotedValue = regexp.MustCompile(`"([^"]*)"`) + cfnFunctionURLBlock = regexp.MustCompile(`(?s)Type:\s*AWS::Lambda::Url\n.*?\n \S`) + cfnAllowMethodsList = regexp.MustCompile(`(?m)^[ \t]*AllowMethods:[ \t]*\n((?:[ \t]*-[ \t].*\n)+)`) + cfnAllowMethodsEntry = regexp.MustCompile(`(?m)^\s*-\s*(\S+)\s*$`) +) + +func assertAllowsRoutedMethods(t *testing.T, site string, methods []string) { + t.Helper() + got := map[string]bool{} + for _, m := range methods { + got[m] = true + } + for _, m := range apiRoutedMethods { + if !got[m] && !got["*"] { + t.Errorf("%s: Function URL CORS allow-methods %v is missing %s, which the API routes; a cross-origin preflight for it is refused at the Function URL", site, methods, m) + } + } +} + +func TestFunctionURLCORS_TerraformAllowsRoutedMethods(t *testing.T) { + path := filepath.Join(repoRoot(t), "terraform", "modules", "compute", "aws", "lambda", "main.tf") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + block := tfFunctionURLBlock.FindString(string(data)) + if block == "" { + t.Fatal(`lambda/main.tf has no resource "aws_lambda_function_url" "main" block`) + } + m := tfCORSAllowMethods.FindStringSubmatch(block) + if m == nil { + t.Fatal("aws_lambda_function_url.main has no cors allow_methods list") + } + values := tfQuotedValue.FindAllStringSubmatch(m[1], -1) + methods := make([]string, 0, len(values)) + for _, v := range values { + methods = append(methods, v[1]) + } + assertAllowsRoutedMethods(t, "terraform lambda module", methods) +} + +func TestFunctionURLCORS_CloudFormationAllowsRoutedMethods(t *testing.T) { + path := filepath.Join(repoRoot(t), "cloudformation", "stacks", "CUDly", "template.yaml") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + block := cfnFunctionURLBlock.FindString(string(data)) + if block == "" { + t.Fatal("CUDly template has no AWS::Lambda::Url resource") + } + m := cfnAllowMethodsList.FindStringSubmatch(block) + if m == nil { + t.Fatal("the AWS::Lambda::Url resource has no Cors AllowMethods list") + } + entries := cfnAllowMethodsEntry.FindAllStringSubmatch(m[1], -1) + methods := make([]string, 0, len(entries)) + for _, v := range entries { + methods = append(methods, v[1]) + } + assertAllowsRoutedMethods(t, "CloudFormation CUDly stack", methods) +} diff --git a/terraform/modules/compute/aws/lambda/main.tf b/terraform/modules/compute/aws/lambda/main.tf index 5b7d0573f..f5b09ea12 100644 --- a/terraform/modules/compute/aws/lambda/main.tf +++ b/terraform/modules/compute/aws/lambda/main.tf @@ -143,7 +143,7 @@ resource "aws_lambda_function_url" "main" { cors { allow_credentials = true allow_origins = var.allowed_origins - allow_methods = ["GET", "POST", "PUT", "DELETE"] + allow_methods = ["GET", "POST", "PUT", "PATCH", "DELETE"] allow_headers = ["Content-Type", "Authorization", "X-CSRF-Token", "X-Session-Token"] max_age = 3600 }