ux(onboarding): add AWS IAM console deep link in account modal - #39
Merged
Merged
Conversation
Issue #21 — following #19's trust-policy snippet, the natural next step is landing the operator on the IAM console's role-creation wizard so they can paste the policy directly. Previously they had to navigate there manually. - New `<a id="account-aws-iam-console-link" target="_blank">` below the trust-policy block, pointing at the role-creation wizard (`https://console.aws.amazon.com/iam/home#/roles$new`). - `rel="noopener noreferrer"` is mandatory for target=_blank to avoid reverse-tabnabbing; the regression test locks that in. - Small `.trust-policy-section #account-aws-iam-console-link` rule for inline-block display + top spacing so the link sits just below the hint text. Closes #21.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
… over non-TLS (07-H1, 07-H2) Plain-text email renderers were using html/template which HTML-encodes special characters in data fields -- "O'Brien" rendered as "O'Brien" and "a&b@x.com" as "a&b@x.com" in recipients' inboxes (07-H1). Add renderTextTemplate backed by text/template and route all non-HTML renderers through it. HTML renderers retain html/template for XSS safety. Regression tests assert that &, ', and < survive verbatim in plain-text bodies and that html/template still escapes <script> in HTML bodies. Also guard dispatchSMTP against cleartext credential transmission (07-H2): if SMTP auth is configured but UseTLS is false the call is refused unless AllowInsecure is explicitly set. AllowInsecure is test-only (local SMTP stubs); factory constructors all use UseTLS=true and never set it. Update smtp_server_test.go tests that used plaintext auth stubs to set AllowInsecure, and add TestSMTPSender_DispatchSMTP_RefusesAuthOverCleartext regression. Also: per-message random MIME boundary via crypto/rand (07-N2); doc note on SMTP SendNotification no-op for GCP/Azure broadcast (07-N4). Closes #1077 (07-H1, 07-H2, 07-N2, 07-N4)
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…1036) * sec(email): route scheduled-purchase and RI-exchange approval tokens through targeted SES, not SNS broadcast (closes #1015) SendScheduledPurchaseNotification and SendRIExchangePendingApproval on the SES/SNS Sender previously rendered bodies that embed live approve/pause/cancel tokens and shipped them via SendNotification (SNS broadcast). Every topic subscriber received working action links, matching the exact threat the purchase-approval path was hardened against in sender.go:386-391 and templates.go:698-704. Fix: - Both methods now require data.RecipientEmail and route through SendToEmailWithCC (targeted SES), mirroring SendPurchaseApprovalRequest. Return ErrNoRecipient when the field is empty rather than silently broadcasting. - Add RecipientEmail/CCEmails fields to RIExchangeNotificationData. - Update call sites (purchase/notifications.go, handler_ri_exchange.go) to resolve GlobalConfig.NotificationEmail and pass it as RecipientEmail. When the global email is unconfigured, log a warning and skip the notification rather than broadcast. - Add structural guard in SendNotification: reject any message body that contains "token=" with ErrTokenInBroadcast, preventing future regressions regardless of how a token-bearing body reaches the broadcast path. - Add 8 regression tests: guard rejects token bodies, guard allows clean bodies, both methods use SES not SNS when RecipientEmail is set, both methods return ErrNoRecipient when RecipientEmail is absent. * fix(email): harden token-broadcast guard and tighten RI-exchange recipient contract Address CodeRabbit pass-1 nitpicks on PR #1036: - sender.go: make the SNS token-broadcast guard case-insensitive so a future template that capitalizes the query param (Token=, TOKEN=) cannot slip a token-bearing body past the guard. Add mixed-case regression cases to TestSendNotification_RejectsTokenBearingBody (fails pre-fix, passes post-fix). - handler_ri_exchange.go: only populate RIExchangeNotificationData.RecipientEmail for the pending-approval branch. The completed/failed path is transport-driven (SNS broadcast / SMTP notifyEmail) and ignores RecipientEmail, so setting it there misrepresented the struct contract. Update the buildExchangeNotificationData doc comment accordingly. - handler_ri_exchange_test.go: add coverage for a configured NotificationEmail, asserting the pending branch receives RecipientEmail populated and the completed branch leaves it empty. - templates_test.go: drop the leftover no-op t.Cleanup(func() {}). * fix(email): use text/template for plain-text bodies, refuse SMTP auth over non-TLS (07-H1, 07-H2) Plain-text email renderers were using html/template which HTML-encodes special characters in data fields -- "O'Brien" rendered as "O'Brien" and "a&b@x.com" as "a&b@x.com" in recipients' inboxes (07-H1). Add renderTextTemplate backed by text/template and route all non-HTML renderers through it. HTML renderers retain html/template for XSS safety. Regression tests assert that &, ', and < survive verbatim in plain-text bodies and that html/template still escapes <script> in HTML bodies. Also guard dispatchSMTP against cleartext credential transmission (07-H2): if SMTP auth is configured but UseTLS is false the call is refused unless AllowInsecure is explicitly set. AllowInsecure is test-only (local SMTP stubs); factory constructors all use UseTLS=true and never set it. Update smtp_server_test.go tests that used plaintext auth stubs to set AllowInsecure, and add TestSMTPSender_DispatchSMTP_RefusesAuthOverCleartext regression. Also: per-message random MIME boundary via crypto/rand (07-N2); doc note on SMTP SendNotification no-op for GCP/Azure broadcast (07-N4). Closes #1077 (07-H1, 07-H2, 07-N2, 07-N4) * fix(email): SNS subject sanitize+truncate, sandbox PII, warnIfPlaintext heuristic (07-M1--M3, 07-L2--L3) 07-M3: SendNotification now passes the subject through sanitizeHeader (strips CR/LF to prevent SNS InvalidParameter) and truncates to 100 bytes (SNS hard limit) before Publish. A PlanName containing a newline would previously cause a runtime InvalidParameter error. Regression tests cover newline stripping and byte truncation. 07-M2: ensureSandboxRecipientVerified user-facing error retains the raw recipient address (needed for the recipient to know which inbox to verify) but the second fmt.Errorf no longer duplicates the address in a gratuitous "sent to %s" suffix. Doc comments clarify the PII-vs-usability tradeoff. 07-M1: doc comments on buildSESSendEmailInput{Multipart} note that SES v2 structured API is not CRLF-injectable via Subject.Data; any future raw-MIME path must call sanitizeHeader before header composition. 07-L2: replace hand-rolled containsColon loop with strings.Contains. Delete containsColon helper. 07-L3: replace the weak length+colon/slash heuristic in warnIfPlaintext with an explicit isSecretManagerReference check (HasPrefix "arn:", "projects/", Contains ".vault.azure.net/", HasPrefix "/"). A 25-char password with a colon no longer silently skips the warning. Update coverage_extra_test.go to test isSecretManagerReference instead of the deleted containsColon. Closes #1077 (07-M1, 07-M2, 07-M3, 07-L2, 07-L3)
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…S (CR #1046) Replace the div.textContent/innerHTML impl with explicit character replacement so double and single quotes are escaped to " and '. Values interpolated into data-* attributes via innerHTML could break out of the attribute value when the string contained a raw quote, enabling XSS (plans.ts:731+, 969-970, 1009-1013). Fix also updates the two utils.test.ts cases that pinned the buggy unescaped-quote output, hardens the providerBadgeHtml mock in plans.test.ts to escape the label before injecting it into the test DOM, and strengthens the history.test.ts absent-total_upfront assertion to verify formatCurrency is called with null and the rendered card shows '--' instead of '$0'.
7 tasks
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
…rs (#1046) * fix(frontend): neutralise provider XSS and dedup modal/global listeners (#1034) H1 - Stored XSS in plans.ts: both planned-purchase row (line 180) and plan card detail (line 412) injected the API provider string into innerHTML class attributes and text without whitelist/escaping. Extract shared providerBadgeClass() and providerBadgeHtml() helpers to utils.ts (matching the existing whitelist in history.ts:341 and recommendations.ts:2417) and replace both raw interpolations. H3 - Event-listener stacking in plans.ts: setupRampScheduleHandlers added listeners to static modal elements on every modal open. Add rampHandlersInstalled install-once guard; export _resetRampHandlersForTest() for test isolation. D1/L4 - Shared provider badge helper: dashboard.ts was using a loose alphanumeric regex instead of the strict aws|azure|gcp whitelist. Align on providerBadgeClass() from utils.ts. Tests: add regression tests to utils.test.ts (providerBadgeClass/ providerBadgeHtml XSS neutralisation), plans.test.ts (H1 call-through and H3 install-once dedup), and update utils mocks in dashboard.test.ts and dashboard-ownership-950.test.ts for the new import. Note: H2 (users/userList.ts duplicate document change listener) was already fixed via AbortController in a prior PR merged to feat/multicloud-web-frontend. * fix(frontend): escape group.id and purchase/plan ids in innerHTML (11-M2, 11-L1) Wrap group.id in escapeHtml() in updateGroupFilterDropdown (finding 11-M2): the option value attribute was injecting the raw API value without escaping, violating the feedback_innerhtml_xss policy. Escape plan.id and purchase.id in all data-id attributes inside the plan-card and planned-purchase-row innerHTML templates (finding 11-L1). UUIDs are safe in practice but must be escaped for consistency and defence-in-depth. Adds regression test in users.test.ts asserting the group.id XSS payload is not emitted verbatim into the rendered option element. Closes part of #1065 * fix(frontend): validate plan and permission numeric fields strictly (11-M1, 11-M3) savePlan now validates term, target_coverage, notification_days_before, and custom ramp fields with Number.isInteger(Number(raw)) before building the request object (finding 11-M1). Fractional or NaN inputs show a toast error and abort without calling the API, matching the pattern in settings.ts validatePurchasingSettings and feedback_strict_int_parse. collectPermissions in groupModals.ts guards max_amount with Number.isFinite(parsed) && parsed >= 0 before assigning to the constraint object (finding 11-M3). Infinity, NaN, and negative values are silently skipped so the rest of the permission constraints still reach the API. Adds regression tests in plans.test.ts (11-M1) and groups.test.ts (11-M3) that verify the guards reject bad inputs and accept valid values. Closes part of #1065 * fix(frontend): replace confirm() and alert() with async toast/dialog (11-L2, 11-L3) Replace blocking window.confirm() calls in handlePlannedPurchaseAction (run and disable actions) with the async confirmDialog helper (finding 11-L2). The dialog uses the same destructive-button styling already applied to the delete-plan and purchase-cancel flows. This removes the last confirm() calls from plans.ts. Replace window.alert() calls with showToast across four files (finding 11-L3): - recommendations.ts: refreshRecommendations success and failure feedback - auth.ts: updateProfile validation error, success, and failure feedback - federation.ts: IaC download failure feedback - modules/registrations.ts: reject-registration failure feedback auth.ts adds a showToast import; federation.ts adds a showToast import. Updates regression tests in plans.test.ts (confirmDialog assertions), auth.test.ts (adds toast mock, updates profile alert assertions), and recommendations.test.ts (updates refreshRecommendations alert assertions) to match the new behaviour. Adds structuredClone polyfill for jsdom in setup.ts. Closes part of #1065 * fix(frontend): structuredClone for deepClone; distinguish absent from zero (11-N1, 11-N2) deepClone in utils.ts now delegates to structuredClone instead of a JSON round-trip (finding 11-N1). structuredClone preserves undefined values, Date objects, Set and Map instances, and handles circular-safe trees. The old JSON.parse/stringify variant is retained as an explicit jsonClone() export for callers that need JSON-serialisation semantics. formatCurrency returns '--' for null, undefined, and non-finite (NaN, Infinity) inputs instead of '$0' (finding 11-N2). This makes missing data visually distinguishable from a real zero balance, closing the silent missing-data bug described in feedback_nullable_not_zero. Adds regression tests in utils.test.ts for both changes. Closes #1065 * fix(frontend): surface absent money/coverage fields as -- instead of fabricating defaults H-2: renderHistorySummary now accepts null summary and renders '--' on all KPI cards rather than passing {} and coercing every money field to $0. H-3: dashboard KPI tiles no longer fabricate values for absent fields: target_coverage removes the hardcoded 80% default (shows '--'); absent current_coverage and active_commitments show '--' not '0%'/'0'; the savings range catch block returns '--' not $0. H-4: extractPlanInfo returns null for absent provider/term/coverage instead of defaulting to 'aws'/3yr/80%. The plan card renders '--' for missing fields; the edit modal leaves selects unset so the user must explicitly choose. H-5: edit modal no longer pre-selects 'no-upfront' when the payment field is absent from the plan -- the payment select is left blank, requiring an explicit choice before re-saving. M-1: savings summary card uses ?? null (not ?? 0) so absent savings propagate to '--' via formatCostForPeriod/formatScaledRange. M-2: numericCellValue returns NaN for absent savings/upfront_cost (consistent with monthly_cost/on_demand_monthly) so filter predicates don't match absent rows as if they were zero. M-3: approval modal header skips null savings in the aggregate (not += 0); total_upfront_cost propagates null to formatCurrency so absent shows '--'. M-5: lastRecommendationsSummary typed as RecommendationsSummary|null; absent API summary is no longer coerced to {}. M-6/M-7: savings-history private formatCurrency gains a null/non-finite guard returning '--'; chart data mapping uses ?? 0 (not || 0) preserving genuine 0. L-2: payback fallback changed from '0 months' to '—' to distinguish inapplicable from instantaneous payback. Regression tests added for H-2, H-3, H-4, H-5 that fail pre-fix. * fix(frontend): escape quotes in escapeHtml to close data-attribute XSS (CR #1046) Replace the div.textContent/innerHTML impl with explicit character replacement so double and single quotes are escaped to " and '. Values interpolated into data-* attributes via innerHTML could break out of the attribute value when the string contained a raw quote, enabling XSS (plans.ts:731+, 969-970, 1009-1013). Fix also updates the two utils.test.ts cases that pinned the buggy unescaped-quote output, hardens the providerBadgeHtml mock in plans.test.ts to escape the label before injecting it into the test DOM, and strengthens the history.test.ts absent-total_upfront assertion to verify formatCurrency is called with null and the rendered card shows '--' instead of '$0'. * fix(frontend): show '--' sentinel for empty savings range, align mock - dashboard.ts: replace formatCurrency(0) with literal '--' when range.cellCount === 0 (CR finding: empty recs fabricated a real $0 instead of the absent-data sentinel) - dashboard.test.ts: align formatCurrency mock with null/non-finite sentinel contract (val==null or !isFinite returns '--'); update three stale assertions that expected '$0' when recs are absent or failed Addresses CR review actionable findings on PR #1046 (outside-diff comments dashboard.ts:252-267 and dashboard.test.ts:104-111). Finding plans.ts:731-735 skipped as CR misread: escapeHtml in utils.ts already escapes both '"' and "'" (lines 162-163). * fix(frontend): use attribute-safe escaping for data-* injections (XSS) Introduces escapeHtmlAttr as a semantic alias for escapeHtml that makes clear the caller is escaping an attribute value. The escapeHtml implementation was already updated in the prior commit to encode " and ' (closing the root vulnerability); escapeHtmlAttr calls through to it so all attribute-context injections are covered. Replaces the remaining escapeHtml calls in data-attribute context: - history.ts: data-retry-id and both data-execution-id sites - plans.ts: data-id and data-name on all plan/purchase action buttons Adds escapeHtmlAttr unit tests to utils.test.ts (quote, ampersand, and bracket encoding; null/undefined; safe strings) and a regression test in history.test.ts that fails against the pre-fix quote-transparent path. --------- Co-authored-by: Claude <noreply@anthropic.com>
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
… renderer Add two tests missing from the #296 wave: - TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload: feeds <script>alert('xss')</script> into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule).
cristim
added a commit
that referenced
this pull request
Jul 10, 2026
… renderer Add two tests missing from the #296 wave: - TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload: feeds <script>alert('xss')</script> into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule).
cristim
added a commit
that referenced
this pull request
Jul 10, 2026
… renderer Add two tests missing from the #296 wave: - TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload: feeds <script>alert('xss')</script> into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule).
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… renderer Add two tests missing from the #296 wave: - TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload: feeds <script>alert('xss')</script> into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule).
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
closes #296) (#813) * feat(email): multipart HTML+plaintext for RI exchange pending approval (#296) Mirror the purchaseApprovalRequestHTMLTemplate pattern for the SendRIExchangePendingApproval flow: - Add riExchangePendingApprovalHTMLTemplate: inline-styled table of exchanges with per-row Approve/Reject CTA buttons, summary block, optional requested-by line, configurable cancellation-window note, and skipped-exchanges section. - Extend riExchangePendingApprovalTemplate (plaintext): labelled Approve/Reject URLs (replacing bracket notation), optional requested-by block, configurable cancellation-window note. - Extend RIExchangeNotificationData with RequestedByName, RequestedByEmail, RequestedAt, CancellationWindowNote fields alongside the RecipientEmail/CCEmails targeting fields. - Add RenderRIExchangePendingApprovalEmailHTML renderer. - Add sendRIExchangePendingApprovalVia helper (shared by Sender and SMTPSender, non-fatal HTML degrade on render error), now CC-aware. - Sender.SendRIExchangePendingApproval: requires RecipientEmail and routes through targeted SES multipart (SendToEmailWithCCMultipart), never broadcasting approval tokens via the SNS topic. - SMTPSender.SendRIExchangePendingApproval: resolves RecipientEmail (falling back to the static notify address), fails loud with ErrNoRecipient when neither is configured, then sends multipart. - 7 new test cases covering both halves, empty/custom cancellation notes, missing requested-by, and skipped-exchanges rendering. Rebased onto feat/multicloud-web-frontend: reconciled with the #1015 / multipart send now preserves that fail-loud, targeted-SES contract instead of falling back to the SNS broadcast topic. Updated the SMTP no-recipient coverage test to assert ErrNoRecipient accordingly. * test(email): add XSS + plain-text verbatim guards for RIExchange HTML renderer Add two tests missing from the #296 wave: - TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload: feeds <script>alert('xss')</script> into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule). * fix(email): correct spelling and MIME format in RI-exchange multipart Use %q for MIME boundary field (correct quoting, satisfies gocritic), replace British spellings (labelled->labeled, honoured->honored) in PR- added comments. All new lint findings introduced by the multipart feature are cleared; pre-existing issues in untouched code paths are unchanged.
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
https://console.aws.amazon.com/iam/home#/roles$new)rel="noopener noreferrer"(reverse-tabnabbing guard)Together with #19 (trust-policy snippet) and #18 (auto-generated External ID), this completes the self-service AWS onboarding flow: generate secret → copy policy → open IAM → paste → done.
Closes #21.
Test plan
npx jest— all tests pass (+1 new)npx tsc --noEmit— clean