sec(api): allow PATCH in CORS and stop returning registration tokens to admins

cloudformation/stacks/CUDly/template.yaml

@@ -669,6 +669,7 @@ Resources:
           - GET
           - POST
           - PUT
+          - PATCH
           - DELETE
         AllowHeaders:
           - Content-Type

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")
+}

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"
 	}

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), &regs))
+	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(&reg, 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)
+}

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"`

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)
+}

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
   }