fix(security/onboarding): External ID lifecycle hardening (#126/#127/#128/#130) - #146
Merged
Merged
Conversation
Closes #126, #127, #128, #130 — four cohesive fixes to the AWS account onboarding External ID generation, validation, and trust-policy snippet. All four touch the same flow (frontend/src/settings.ts + internal/api/handler{,_accounts}.go) and share regression tests, so they ship together rather than as four PRs. #126 — External ID stable across modal close/reopen The Add-AWS-Account modal previously called generateExternalID() on every open, so an operator who copied the value into AWS, closed the modal without saving, and reopened it later got a fresh UUID. Saving then stored a value that didn't match the trust policy, breaking sts:AssumeRole with a silent ExternalId mismatch. Fix: persist the generated ID in sessionStorage under `cudly:aws-account-modal:draft-external-id`. The draft is reused for every subsequent open until handleAccountFormSubmit clears it on a successful save. Edits of existing accounts always use the persisted value and never read or write the draft. sessionStorage failures fall through to in-memory generation so privacy-mode browsers still work. #127 — Replace Math.random() fallback with crypto.getRandomValues generateExternalID() previously fell back to `Math.random().toString(36)` if crypto.randomUUID was missing. The External ID is a security primitive that protects against the confused-deputy problem on cross-account sts:AssumeRole; falling back to a non-CSPRNG defeats the protection in the (rare) cases the fallback is hit. Replaced with crypto.getRandomValues over a Uint8Array(16), hex-encoded — getRandomValues has a strict superset of randomUUID's browser support, so it's a safe primary fallback. A last-resort string fallback remains for runtimes with no Web Crypto at all (jsdom without polyfill, very old webviews); it is non-secret- quality but prevents an empty submit, and the new backend validator (#128) catches obviously-weak values regardless. #128 — Backend validation for aws_external_id on role_arn auth mode internal/api/handler_accounts.go validateAuthMode now enforces, on both create and update, that aws_external_id when paired with auth_mode == "role_arn" AND a non-empty aws_role_arn: - is non-empty, - is 16..1224 chars (lower bound is CUDly-internal; upper bound is AWS's documented sts:ExternalId limit), - matches AWS's documented charset `[A-Za-z0-9_+=,.@:/-]`. Self-account onboarding (role_arn with empty role ARN, meaning "use ambient Lambda credentials, no AssumeRole") is exempt — that path never hits sts:ExternalId. Defence-in-depth: the frontend already populates this field, but a hostile or buggy client posting "" would otherwise make AssumeRole bypass the ExternalId condition entirely if the customer's trust policy lacks the StringEquals constraint. #130 — Four polish items on the trust-policy snippet - (a) Race short-circuit: renderAwsTrustPolicy snapshots the auth- mode select before awaiting getConfig and rechecks after. If the user switched away from role_arn during the await we abort the write so the policy JSON never lands in a hidden field. - (b) :root principal: the rendered policy still uses the documented AWS SaaS-baseline `arn:<partition>:iam::<acct>:root`. Updated the modal hint to make explicit that this trusts every IAM principal in CUDly's host account and that the sts:ExternalId condition is what locks the role to a specific registration — previously the hint understated the trust scope. - (c) Partition awareness: extended sourceIdentity (Go) and SourceIdentity (TS) with a Partition field, parsed from STS GetCallerIdentity's returned ARN. The frontend interpolates `arn:<partition>:iam::...` so AWS China (`aws-cn`) and GovCloud (`aws-us-gov`) deployments render a usable snippet instead of a silently-wrong `arn:aws:` ARN. Unknown partition values are rejected at the Go boundary (defence against ARN- injection into a copy-pasted JSON snippet) and default to `aws` on the frontend. - (d) getConfig caching: each modal open previously fired a fresh GET /api/config. Added a module-level promise cache (cachedConfigPromise) so the call resolves at most once per page load. Failures clear the cache so the next caller can retry. Tests - frontend/src/__tests__/settings-external-id.test.ts (new): 12 cases covering the #126 stable-draft + clear-on-save path, the #127 CSPRNG fallback (asserts crypto.getRandomValues is called and Math.random is not), the #130a race short-circuit, #130c partition rendering for aws/aws-cn/aws-us-gov, and the #130d single-fetch invariant. Includes a source-level regression guard that fails if Math.random( reappears in settings.ts. - internal/api/handler_accounts_external_id_test.go (new): table-driven tests for validateAWSExternalID + per-auth-mode handler tests + parseArnPartition coverage including the self-account exemption. Verified locally with go build ./... && go test ./... and npx jest && npx tsc --noEmit && npm run build.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request implements AWS External ID stability, partition-aware trust-policy rendering, and backend validation. Changes include sessionStorage-backed draft persistence preventing unwanted ID regeneration on modal reopen, replacement of Math.random() with WebCrypto fallbacks, configuration caching with race guards, partition extraction from AWS ARNs for cn/gov regions, and comprehensive frontend/backend validation of External ID format and length constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as Add AWS Account Modal
participant Store as sessionStorage
participant API
rect rgba(100, 150, 255, 0.5)
Note over User,Store: First Modal Open (Create Draft)
User->>Modal: Click "Add AWS Account"
Modal->>Store: Check for draft External ID
Store-->>Modal: No draft exists
Modal->>API: (Optional) Fetch config for partition
API-->>Modal: Config + partition info
Modal->>Modal: Generate new External ID (crypto.randomUUID)
Modal->>Store: Persist draft External ID
Store-->>Modal: Stored
Modal-->>User: Display modal with External ID
User->>User: Copy External ID to AWS trust policy
User->>Modal: Close without saving
end
rect rgba(150, 255, 100, 0.5)
Note over User,Store: Reopen Modal (Reuse Draft)
User->>Modal: Click "Add AWS Account" again
Modal->>Store: Check for draft External ID
Store-->>Modal: Return same External ID (stable)
Modal-->>User: Display modal with SAME External ID
User->>Modal: Fill rest of form & save
end
rect rgba(255, 150, 100, 0.5)
Note over User,Store: After Successful Save (Clear Draft)
Modal->>API: POST /accounts (with External ID)
API-->>Modal: Success
Modal->>Store: Clear draft External ID
Store-->>Modal: Cleared
end
sequenceDiagram
participant Frontend as Frontend (settings.ts)
participant Cache as Config Cache
participant API as Backend API
participant AuthMode as Auth Mode Selector
rect rgba(100, 150, 255, 0.5)
Note over Frontend,AuthMode: First Modal Open (Fetch Config)
Frontend->>Cache: getCachedConfig() — page-lifetime cache
Cache-->>Frontend: Cache empty, fetching...
Cache->>API: GET /api/config
API-->>Cache: Config (partition: aws-cn)
Cache-->>Frontend: Config data
Frontend->>Frontend: renderAwsTrustPolicy(partition=aws-cn)
Frontend-->>Frontend: Generate ARN with aws-cn prefix
end
rect rgba(150, 100, 255, 0.5)
Note over Frontend,Cache: Auth Mode Changes During Fetch
Frontend->>Frontend: User changes auth mode while getConfig pending
Frontend->>Cache: Store current auth-mode ID
Cache->>API: (Still fetching original request...)
API-->>Cache: Config arrives
Cache->>Frontend: Check race guard—auth mode changed!
Frontend-->>Frontend: Abort rendering (race detected)
Frontend->>Frontend: Do NOT write stale data
end
rect rgba(100, 255, 150, 0.5)
Note over Frontend,Cache: Subsequent Modal Opens (Cached)
Frontend->>Cache: getCachedConfig() — cache hit
Cache-->>Frontend: Return cached (aws-cn)
Frontend->>Frontend: renderAwsTrustPolicy(partition=aws-cn)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_accounts_external_id_test.go (1)
86-104: Minor: Test name doesn't match test data.The test is named
RoleArnTooShortwith a comment "rejects 15-char IDs", but the actual test value"too-short"is only 9 characters. The test still correctly validates the behavior (values under 16 chars are rejected), but the naming is slightly misleading.Consider aligning the test value with the name
// TestCreateAccount_AWSExternalID_RoleArnTooShort rejects 15-char IDs. func TestCreateAccount_AWSExternalID_RoleArnTooShort(t *testing.T) { ... body := `{"name":"Acme","provider":"aws","external_id":"123456789012",` + `"aws_auth_mode":"role_arn","aws_role_arn":"arn:aws:iam::123456789012:role/CUDly",` + - `"aws_external_id":"too-short"}` + `"aws_external_id":"0123456789abcde"}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler_accounts_external_id_test.go` around lines 86 - 104, The test TestCreateAccount_AWSExternalID_RoleArnTooShort has a mismatch between its name/comment and the test data: update the test input in the body string (the aws_external_id field in the handler.createAccount call) to use a 15-character value (e.g. "123456789012345") to match "rejects 15-char IDs" or alternatively rename the test/comment to reflect the current 9-char value; change the aws_external_id literal in the body or the test name/comment accordingly so they are consistent with each other and the intended validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/handler_accounts_external_id_test.go`:
- Around line 86-104: The test TestCreateAccount_AWSExternalID_RoleArnTooShort
has a mismatch between its name/comment and the test data: update the test input
in the body string (the aws_external_id field in the handler.createAccount call)
to use a 15-character value (e.g. "123456789012345") to match "rejects 15-char
IDs" or alternatively rename the test/comment to reflect the current 9-char
value; change the aws_external_id literal in the body or the test name/comment
accordingly so they are consistent with each other and the intended validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bab34cf1-f065-45fc-920d-3a8c41534a7d
📒 Files selected for processing (6)
frontend/src/__tests__/settings-external-id.test.tsfrontend/src/settings.tsfrontend/src/types.tsinternal/api/handler.gointernal/api/handler_accounts.gointernal/api/handler_accounts_external_id_test.go
CodeRabbit flagged the test name (`RoleArnTooShort`, comment "rejects 15-char IDs") didn't match the actual value (`"too-short"`, 9 chars). The behaviour was correct — anything below 16 was rejected — but the test no longer pinned the boundary it claimed to. Switch to a 15-char value so the test exercises exactly min-1, complementing the pure- helper boundary table that already covers 1, 15, and 1225. Addresses CodeRabbit nitpick on PR #146.
This was referenced Apr 27, 2026
Closed
Merged
cristim
added a commit
that referenced
this pull request
Apr 28, 2026
…nippet to bastion auth mode (#157) When the AWS account modal is in bastion auth mode, the frontend never collected an aws_external_id and never rendered a trust-policy snippet (only role_arn mode did). The user has confirmed this is an oversight, not a deliberate scope decision: bastion mode also calls sts:AssumeRole into the customer's role (just via the bastion's STS client instead of ambient credentials), so the same sts:ExternalId confused-deputy guard applies. Frontend (frontend/src/index.html, frontend/src/settings.ts): - Add a readonly External ID input + copy button + IAM trust-policy <pre> block + Copy button inside #account-aws-bastion-fields, mirroring the role_arn block. The two External ID inputs share the same draft value (resolved via the existing resolveAwsExternalIdForModal + AWS_DRAFT_EXTERNAL_ID_KEY from PR #146 — no parallel generator) so toggling auth modes mid-edit never surfaces diverging values. - buildAccountRequest's bastion branch now collects aws_external_id from the new bastion-mode input, undefined when blank. - New renderAwsBastionTrustPolicy() mirrors renderAwsTrustPolicy but sets Principal to the *bastion* account root (looked up via listAccounts → matched on the dropdown selection), with the same partition awareness (#130c) and race short-circuit (#130a) as the role_arn renderer. Re-renders on auth-mode change and on bastion dropdown change. Backend (internal/api/handler_accounts.go): - validateAWSAuthMode now also runs validateAWSExternalID for bastion mode when aws_role_arn is non-empty, parallel to role_arn. workload_identity_federation stays exempt (OIDC verifies identity via the token subject claim — stscreds.WebIdentityRoleOptions has no ExternalID field), and so does access_keys (no AssumeRole at all). The empty-role-ARN exemption mirrors role_arn's self-account onboarding shortcut. - Updated the "required" error message to mention both modes. - The credential resolver was already correct: resolveBastionProvider delegates to resolveRoleARNProvider, which has always read account.AWSExternalID and threaded it into stscreds.AssumeRoleOptions — so the missing piece was the frontend collection + backend validation, not the resolver. Tests: - frontend/src/__tests__/html.test.ts — assert the bastion section has the new input, copy button, and trust-policy block. - frontend/src/__tests__/settings-external-id.test.ts — new suite covering: bastion shows the same draft as role_arn; selecting a bastion renders a snippet with the bastion's account root as Principal; buildAccountRequest sends aws_external_id; toggling modes preserves the draft. - frontend/src/__tests__/settings-accounts.test.ts — DOM scaffold extended with the new bastion-mode fields. - internal/api/handler_accounts_external_id_test.go — bastion now enforced in dedicated tests (BastionRequiresExternalID, BastionHappyPath, BastionNoRoleArnExempt) and removed from the "no-validation-needed" loop. Builds on PR #146 (#126/#127/#128/#130) — reuses the secure CSPRNG generator, sessionStorage draft persistence, validateAWSExternalID charset/length validator, and partition-aware ARN rendering. Closes #129.
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
1 participant
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
Bundles four cohesive fixes to the AWS account onboarding External ID
flow into a single PR — they all touch the same modal lifecycle and
backend validation surface, share regression tests, and ship together
to harden one feature end-to-end.
breaking
sts:AssumeRolewhen an operator copied the value into AWS,closed the modal, and reopened later. Now the draft persists in
sessionStorageuntil a successful save and is reused across opens.generateExternalID()fell back toMath.random()whencrypto.randomUUIDwas missing. Replaced withcrypto.getRandomValues(a strict superset of
randomUUID's browser support); a non-CSPRNGstring fallback is kept only for runtimes with no Web Crypto at all,
and the new backend validator (fix(security): backend validation for aws_external_id non-empty + length + charset on role_arn auth mode #128) catches obviously-weak values.
aws_external_idfor therole_arnauth mode: non-empty + 16..1224 chars (AWS limit) + AWS-documented charset
[A-Za-z0-9_+=,.@:/-], on both create and update.Self-account onboarding (role_arn + empty role ARN ⇒ ambient creds,
no AssumeRole) is exempt.
getConfigis in flight — never paints into a hidden field;:rootprincipal trusts every IAM principal in CUDly's hostaccount, with
sts:ExternalIdproviding the per-registration lock;sourceIdentitynowpopulates a
Partitionfield parsed from STSGetCallerIdentity'sARN; the frontend interpolates
arn:<partition>:iam::...so AWSChina (
aws-cn) and GovCloud (aws-us-gov) deployments render ausable snippet instead of a silently-wrong
arn:aws:ARN;getConfig()is fetched at most once per page load(module-level promise cache).
Closes #126, #127, #128, #130.
Test plan
go build ./... && go test ./...— all packages pass.npx jest— 37 suites / 1308 tests pass, including 12 new casesin
frontend/src/__tests__/settings-external-id.test.tscovering reopen-stability, draft-clear-on-save,
crypto.getRandomValuesfallback, race short-circuit, allthree partition cases, and the once-per-session getConfig
invariant.
npx tsc --noEmit && npm run build— typecheck + productionbundle clean.
git-secrets, gosec, trivy, frontend tests, frontend build.
(https://33pz7pombdqwu3bdlxp4lqxyra0bsriy.lambda-url.us-east-1.on.aws/)
— post-merge to
feat/multicloud-web-frontend.Out of scope
issue).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests