Repository navigation
Conversation
…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 LeanerCloud#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 LeanerCloud#1116, which makes the same change. Refs LeanerCloud#310 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
triaged
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #310 groups three low-severity findings from the 2026-09-02 audit. I re-checked each one against current main (a14dc8f):
requiresCSRFValidationnow exempts only GET/HEAD/OPTIONS, and there's a router-table guard). CORS part still open:buildResponseHeaderssendsGET, POST, PUT, DELETE, OPTIONS, and the Lambda Function URL CORS config in bothterraform/modules/compute/aws/lambda/main.tfandcloudformation/stacks/CUDly/template.yamlallows onlyGET, POST, PUT, DELETE.reference_tokenconfig.AccountRegistration.ReferenceTokenis taggedjson:"reference_token", soGET /api/registrationsandGET /api/registrations/{id}return each registrant's status credential.json:"-".MatchesAccount/CanAccessAccount. Changing the same code here would conflict with it.Refs #310
This PR plus #1116 together cover the issue. Once both merge, #310 can be closed.
Root cause and fix
A02-015: CORS method list
The router serves
PATCH /api/plans/{id}andPATCH /api/ladder/tranches/{id}, but none of the three CORS method lists included PATCH:internal/api/handler.gobuildResponseHeaders: this is what the handler (and the container/HTTP server that passes its headers through) sends in reply to a preflight.terraform/modules/compute/aws/lambda/main.tfaws_lambda_function_url.maincors.allow_methods.cloudformation/stacks/CUDly/template.yamlAWS::Lambda::UrlCors.AllowMethods.When a Function URL has a CORS config, AWS answers the preflight from that config. Fixing only the Go header would therefore leave Lambda deployments refusing cross-origin PATCH. All three sites now list PATCH. The CloudFormation comment that says its lists match the Terraform module is still accurate.
Same-origin deployments, where the dashboard is served through CloudFront and the frontend module already forwards PATCH, did not hit this. It only affected deployments that call the API cross-origin.
A02-020:
reference_tokenin admin responsesReferenceTokenis now taggedjson:"-"onconfig.AccountRegistration.submitRegistrationalready returns the token through its own explicitmap[string]string, so registrants and the federation IaC scripts (jq -r .reference_token) still get it. The admin list and detail endpoints, and anything else that serializes the struct later, no longer include it.rejectRegistration's filtered map is unchanged. No code JSON-decodes anAccountRegistration, and the DB layer scans the column directly, so storage and theGET /api/register/{token}lookup are unaffected.openapi.yamldoesn't document the field.Verification
All runs were local on Linux with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. No cloud APIs were called. The evidence is unit tests throughHandler.HandleRequestwith mocked auth and config stores, plus static checks that read the Terraform and CloudFormation source. There is no deployed or real-browser cross-origin run.New tests:
internal/api/cors_methods_test.goTestHandleRequest_PreflightAllowsPATCH: an OPTIONS preflight for/api/plans/{id}must list PATCH.TestHandleRequest_PreflightAllowsEveryRoutedMethod: reads the real router table and requires every verb a route uses in the preflight'sAccess-Control-Allow-Methods. HEAD is excluded because it is CORS-safelisted and never preflighted. A floor assertion fails if the table read finds no GET/POST/PUT/PATCH/DELETE route.internal/api/handler_registrations_token_test.goTestListRegistrations_DoesNotExposeReferenceTokenandTestGetRegistration_DoesNotExposeReferenceToken: admin session throughHandleRequest. The body contains neither thereference_tokenkey nor the token value, and it still contains the registration, so a fix that blanks the response would also fail.TestAccountRegistration_JSONOmitsReferenceToken: pins the type itself.handler_registrations_submit_test.goalready asserts thatsubmitRegistrationstill returnsreference_token.terraform/modules/compute/aws/function_url_cors_test.go:TestFunctionURLCORS_TerraformAllowsRoutedMethodsandTestFunctionURLCORS_CloudFormationAllowsRoutedMethodsparse the Function URL CORS method list at each site and require GET/POST/PUT/PATCH/DELETE.Fail before, pass after: I reverse-applied the four non-test changes, kept the new tests, and ran
go test -count=1 -v -run 'Preflight|ReferenceToken|FunctionURLCORS' ./internal/api/ ./terraform/modules/compute/aws/. All seven new tests failed:With the fix re-applied, all seven pass. The existing
TestHandler_HandleRequest_CORS_Preflightpasses both before and after.Other checks on the branch head:
go build ./...,go vet ./...,go vet -tags integration ./...: pass.go test -count=1run one package at a time on./internal/api/,./internal/config/,./terraform/modules/compute/aws/,./internal/mocks/and./internal/server/: all ok.run --allow-parallel-runners ./internal/api/... ./internal/config/... ./terraform/modules/compute/aws/...: 0 issues.gocyclo -over 10 -ignore "_test\.go" internal/api/handler.go internal/config/types.go: clean.terraform fmt -checkandterraform validate(init -backend=falsewith a temporaryTF_DATA_DIR) onterraform/modules/compute/aws/lambda: pass. No plan or apply.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
internal/server/health.gosets its ownAccess-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONSon the health endpoint. That endpoint is GET-only, so it was left as is.allow_headerslists differ from the handler'sAccess-Control-Allow-Headers(for example,X-API-KeyandX-Authorizationare not in the Function URL list). That is not part of this finding and was not changed.Frontend follow-up for the UI owner
frontend/src/api/registrations.tsdeclaresreference_token: string;on theAccountRegistrationinterface. Nothing infrontend/srcreads it, so nothing breaks at runtime. The field is now always absent fromGET /api/registrationsandGET /api/registrations/{id}, so it should be removed from the interface (or made optional) to keep the type accurate.Labels
Could a maintainer mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s,type/security?🤖 Generated with Claude Code