Repository navigation
Conversation
…ters
Three of the four low findings in the 2026-09-02 audit issue, re-verified
on current main:
- A02-012: GET /api/users/{id}, GET /api/groups/{id} and DELETE / revoke
of an API key answered 500 for a missing row (raw pgx.ErrNoRows or an
untyped error) where openapi.yaml documents 404. internal/auth now
returns ErrUserNotFound, ErrGroupNotFound (both still match
pgx.ErrNoRows), ErrAPIKeyNotFound and ErrAPIKeyNotOwned, and the
handlers map them to 404. A non-admin acting on another user's key gets
the same 404 as a missing key.
- A02-021: the dashboard summary, both inventory reads and both account
lists accepted any provider (and any "enabled") and answered 200 with
an empty or unfiltered result. They now validate with validateProvider
(400, as on recommendations/history) and treat provider "all" as no
filter; enabled must be true or false.
- A01-021: the UpcomingPurchase doc said canceling one upcoming purchase
leaves its plan active, but since LeanerCloud#774 the cancel also disables the
plan. The Go contract comment now says so; the dashboard confirm copy
is left to the UI owner.
A02-007 (approveRegistration ordering) is left to the in-flight fix on
fix/issue-1102 (LeanerCloud#1141), which rewrites the same lines.
Refs LeanerCloud#312
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.
Works through the four low-severity
internal/apifindings bundled in #312 (2026-09-02 audit). I re-checked each one against currentmain(a14dc8f). Before touching a function, I diffed it against every in-flight branch: open PRs #1111, #1116, #1141, #1212, #1237, #1241, #1242, #1243, #1244, and the unopenedfix/issue-532,fix/issue-54,fix/issue-169,fix/issue-310,fix/issue-337on the IAmJSD fork.Refs #312
This is a partial fix (hence
Refs). A02-007 is deferred to #1141, and the A01-021 dashboard copy needs the UI owner.Per-finding status
plan_disabled: true. TheUpcomingPurchaseGo doc and the dashboard confirm dialog still say the plan stays in place.UpcomingPurchasecontract comment ininternal/api/types.gonow describes the real behavior. This only changes a comment, so it has no fail-before test; the existingdelete_planned_purchase_scope_test.gotests already pin the disable behavior. The UI copy is deferred to the UI owner (see "Frontend follow-up"). I kept the #774 behavior rather than dropping the disable, because #774/#520/#623 chose it on purpose. Reversing it is an owner decision.approveRegistrationcommitsapprovedbefore the account insertfix/issue-1102). That branch rewrites exactly these lines ofapproveRegistration: it moves the transition, the account insert and the link into a singleApproveAccountRegistrationtransaction. Editing them here would conflict.GET /api/users/{id}gets the rawpgx.ErrNoRows;GET /api/groups/{id}gets rawpgx.ErrNoRowsfromscanGroupor a plain "group not found"; API key delete/revoke get plain-string errors. All of them become 500 plus an error-level log line, whileopenapi.yamlalready documents 404.provider=AWSorenabled=1→ 200 with an empty or unfiltered result).No in-flight branch edits the lines changed here. The nearest overlaps:
fix/issue-1034(fix(auth): bind group account scope entries to account IDs #1116) editscreateGroup/updateGroup, notgetGroup.fix/issue-169edits other hunks ofauth/service_api.goandserver/app.go, notGetGroupAPIor the adapter'sGetUser.updateAccount, notbuildAccountFilter.I put the new tests in new files so they can't conflict with branches that edit the existing test files.
Root cause and fix
A02-012.
internal/authhad no not-found sentinels, andIsNotFoundErroronly matches the router's own type, sohandleRequestErrortreated every not-found as a server failure.internal/auth/errors.goadds four sentinels:ErrUserNotFoundandErrGroupNotFound. These wrappgx.ErrNoRowswhen the store returned it, so callers that checkerrors.Is(err, pgx.ErrNoRows)keep working.ErrAPIKeyNotFound.ErrAPIKeyNotOwned. Its message is unchanged:unauthorized: cannot <action> another user's API key.Service.GetUser,Service.GetGroupAPI,authorizeAPIKeyAccess(behind delete and revoke), and the server adapter's nil-user branch.mapAuthError(used bygetUser) mapsErrUserNotFoundto 404 "user not found".getGroupmapsErrGroupNotFoundto 404 "group not found".mapAPIKeyAccessErrormaps bothErrAPIKeyNotFoundandErrAPIKeyNotOwnedto the same 404 "API key not found", so a non-admin can't tell another user's key ID from a nonexistent one.A02-021. These handlers forwarded
params["provider"]without validation.buildAccountFiltersilently ignored anyenabledvalue other thantrue/false.providerFilterParamrunsvalidateProviderand mapsallto "no filter".validateProvideralready acceptsallas "every provider"; before this change these reads filtered on the literal string"all"and returned nothing.getDashboardSummary,fetchCommitmentRecords(behind/api/inventory/commitmentsand/api/inventory/coverage) andgetCoverageBreakdownnow use it before any store read.buildAccountFilter(behindGET /api/accountsand/api/accounts/list) now returns(filter, error). It rejects unknown providers and anyenabledother thantrue/false/empty with 400.openapi.yamladds the400response to/api/dashboard/summary. The inventory and accounts list endpoints aren't in the spec.The frontend only sends
aws|azure|gcp|''andenabled=true|false(frontend/src/api/{dashboard,inventory,accounts}.ts), so the UI is unaffected.No DB or schema change and no new concurrency surface, so no race test applies. The store already returns
pgx.ErrNoRowsfor a missing user, group or key (scanUser/scanGroup/scanAPIKey;store_postgres_test.gopins the group case), so the not-found paths are covered with mocks at the service and handler layers.Verification
All of this is unit/mock evidence with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9. No real cloud or HTTP traffic: theinternal/apino-network guard was active.Fail-before (run in TDD order: tests written first, then the fix):
go test -count=1 -run 'TestService_GetUser_MissingUserIsErrUserNotFound|TestService_GetGroupAPI_MissingGroupIsErrGroupNotFound|TestService_APIKeyAccess_NotFoundAndNotOwnedSentinels' ./internal/auth/. This ran with only the sentinel declarations added. Result: FAIL,Target error should be in err chain: expected: "user not found"(and the same for group, API key andunauthorized, 8 assertions).go test -count=1 -run '^TestAuthServiceAdapter_GetUser_MissingUserIsErrUserNotFound$' ./internal/server/. This ran withapp.goandservice_user.goreverted to main. Result: FAIL in both subtests (store_returns_ErrNoRows,store_returns_nil_user).go test -count=1 -run 'TestHandler_getUser_MissingUserIs404|TestHandler_getGroup_MissingGroupIs404|TestHandler_APIKeyDeleteRevoke_' ./internal/api/. Result: FAIL with:must be a client error, not a 500: user not found: no rows in result set... group not found... failed to delete API key: API key not found... failed to revoke API key: unauthorized: cannot delete another user's API key-run '^<name>$' ./internal/api/) on the pre-fix handlers:TestHandler_getDashboardSummary_RejectsUnknownProvider: FAIL.provider=AWSreachedListRecommendations(mock: I don't know what to return because the method call was unexpected).TestHandler_getDashboardSummary_AllProviderMeansNoFilter: FAIL.ListRecommendationswas called withProvider:"all".TestHandler_inventory_RejectsUnknownProvider: FAIL. The store was read withprovider=AWS.TestHandler_listActiveCommitments_AllProviderMeansNoFilter: FAIL."[]" should have 2 item(s), but has 0.TestHandler_getCoverageBreakdown_AllProviderMeansNoFilter: FAIL.provider=all must read the same as no provider.TestListAccounts_RejectsUnknownEnumParams: FAIL.An error is expected but got nilforenabled=1|yes|TRUE,provider=AWS|ibm|"aws ", on both/api/accountsand/api/accounts/list.TestListAccounts_AcceptedFilterValues: FAIL.provider=allproduced a non-nil provider filter.Pass-after on the branch head:
TestHandler_getUser_StoreFailureStays500,TestHandler_getGroup_StoreFailureStays500,TestHandler_APIKeyDeleteRevoke_StoreFailureStays500).go test -count=1 ./internal/api/→ okgo test -count=1 ./internal/auth/→ okgo test -count=1 ./internal/server/→ okgo test -count=1 ./internal/mocks/→ ok. This includes the vacuous-mock-assertion guard, which caught two matcher-lessAssertNotCalledcalls in my first draft; both are fixed.go build ./...,go vet ./...andgo vet -tags integration ./...→ clean.golangci-lint run --allow-parallel-runners ./internal/api/ ./internal/auth/ ./internal/server/(v2.10.1) → 0 issues.gocyclo -over 10 -ignore "_test\.go"on every changed non-test file → nothing over 10 (mapAuthErroris 5,buildAccountFilteris 6).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 / notes
internal/auth/service_apikeys_api.go(around lines 425 and 474), on paths the audit didn't flag. I left them alone.updateUser/deleteUserfor a missing user weren't in the finding and keep their current behavior.mapAuthErrornow also mapsErrUserNotFound, but onlygetUserproduces it today.aws|azure|gcp|all, orenabledother thantrue|false, to these reads now get 400 instead of a silently empty or unfiltered 200. I could not check those repos from here.TestListAccounts_Success(an existing test) runs alone, theinternal/apino-network guard reports blocked IMDS calls frommarkSelfAccount. The same happens on main. It passes in the full package run. Unrelated to this change; my new account tests pre-resolve the source identity so they never make that call.Frontend follow-up for the UI owner
A01-021:
frontend/src/dashboard.tscancelScheduledPurchasestill tells the user: "Cancelling removes this upcoming step from the plan. The plan itself stays in place; the next scheduled step (if any) is unaffected." The comment above it says the same. Since #774 the backend also disables the plan, so no further purchases happen until someone re-enables it on the Plans page. The response carriesplan_disabled: true. Suggested change:plan_disabledin the success toast, e.g. "Purchase cancelled; plan disabled".If the owner would rather the cancel not disable the plan, that reverses #774/#520/#623 and should be a separate issue.
Issue #312 can close once #1141 (A02-007) and the UI copy change above have both landed.
Maintainers: please mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/m,type/bug.🤖 Generated with Claude Code