Skip to content

fix(security): replace real AWS account ID with placeholder#1212

Open
cristim wants to merge 1 commit into
mainfrom
fix/hyg-02-redact-account-id
Open

fix(security): replace real AWS account ID with placeholder#1212
cristim wants to merge 1 commit into
mainfrom
fix/hyg-02-redact-account-id

Conversation

@cristim

@cristim cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member

Problem

Closes #1167 (review finding HYG-02, P2).

The real production AWS account ID 540659244915 was committed in four tracked files on main:

  • internal/email/sender.go (doc comment on AccountLabel)
  • internal/email/template_renderers_test.go (fixture + assertions, 3 occurrences)
  • frontend/src/__tests__/settings-accounts.test.ts (fixtures + aria-label selectors, 5 occurrences)
  • known_issues/23_ux_review_2026_04_22.md (UX evidence quote)

This violates the repo's own .gitallowed policy, which states that only deliberate placeholders may appear in tracked files and that a real account ID landing in the repo must be treated as a leak, not allowlisted.

Fix

Replace every occurrence with the already-allowlisted placeholder 123456789012. Test data shape and assertions are preserved in lockstep (the label still flows through rendering and is asserted in the output), so coverage is unchanged.

No literal-absence regression test is added: such a test would have to re-embed the very ID it guards against. The git-secrets pre-commit/CI gate remains the enforcement point; the gate hole and follow-ups are documented on issue #1167:

  • git-secrets' --register-aws account-ID pattern only matches key/assignment shapes (aws_account_id = ...); bare 12-digit IDs in string labels, comments, and external_id: '...' fixtures escape it (confirmed by probing the registered patterns: grep -w word-boundary semantics reject matches starting mid-identifier, and label/comment shapes lack the required :/= immediately before the digits).
  • History scrubbing and treat-as-leak handling per the .gitallowed note are owner decisions, left on the issue.
  • A second non-placeholder account ID (909626172446, "CUDly host") exists in 3 tracked files; filed as a separate follow-up issue.

Test evidence

  • go build ./... succeeds
  • go test ./internal/email/... passes (335 tests)
  • cd frontend && npx jest src/__tests__/settings-accounts.test.ts passes (80 tests)
  • git grep 540659244915 returns no matches (exit 1)

Summary by CodeRabbit

  • Tests

    • Updated account override UI tests and email template renderer tests to use corrected example account external IDs, along with matching modal and rendered email assertions.
  • Documentation

    • Refreshed the example account identifier in the known issues UX review notes for the Settings → Accounts evidence line.

@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/s Hours type/security Security finding labels Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 16 minutes and 39 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fec1c27e-d33d-44e3-aa93-ea4dad23ec6a

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and 805de41.

📒 Files selected for processing (4)
  • frontend/src/__tests__/settings-accounts.test.ts
  • internal/email/sender.go
  • internal/email/template_renderers_test.go
  • known_issues/23_ux_review_2026_04_22.md
📝 Walkthrough

Walkthrough

Replaces the real AWS account ID 540659244915 with 123456789012 in frontend tests, backend email examples and tests, and one known-issues note. No functional code paths change.

Changes

Account ID example replacements

Layer / File(s) Summary
Frontend account overrides modal test data
frontend/src/__tests__/settings-accounts.test.ts
Mocked account IDs, overrides-button selectors, and modal title assertions now use 123456789012.
Backend email examples and note
internal/email/sender.go, internal/email/template_renderers_test.go, known_issues/23_ux_review_2026_04_22.md
The RecommendationSummary comment example, approval email test inputs/assertions, and the known-issues evidence line now use 123456789012.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

  • #1167: The PR replaces the tracked real AWS account ID called out in the issue across the affected files.
  • #1213: The frontend settings-accounts test updates touch the same hardcoded account-ID fixtures/selectors area mentioned by the issue.

Suggested labels

bug

Poem

🐰 A secret number took a hop,
From real to placeholder, clean and стоп?
Tests and notes now match the tune,
123456789012 shines like the moon. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Most occurrences were redacted, but #1167's affected file internal/email/executed_notification_test.go is not shown as updated. Update the remaining tracked occurrence, especially internal/email/executed_notification_test.go, and confirm git grep finds no 540659244915.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the security-focused replacement of a real AWS account ID with a placeholder.
Out of Scope Changes check ✅ Passed The diff stays within the leak-removal scope: only test fixtures, a comment, and a markdown note were updated.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hyg-02-redact-account-id

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

CI status note: the failing checks on this PR are pre-existing breakage on main, not caused by this diff. The same workflows fail on main's head commit cfead3bc5 (the exact base of this branch):

  • Lint / Integration: internal/config/store_postgres_recommendations_test.go:177 still uses the removed MinSavings field after the MinSavingsUSD/MinSavingsPct rename (integration build tag, so plain go test missed it).
  • Security Scanning: govulncheck findings in dependency modules, unrelated to this change.

PR #1220 tracks the main CI repair. Once it merges, a rebase/re-run here should go green.

Independent re-verification on this branch (fresh worktree at 9ec1242):

@cristim cristim force-pushed the fix/hyg-02-redact-account-id branch from 9ec1242 to ce21e93 Compare June 19, 2026 14:47
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The production AWS account ID was committed in four tracked files (a
sender.go doc comment, email template test fixtures, frontend
settings-accounts test fixtures, and a known_issues UX review doc),
violating the .gitallowed policy that only deliberate placeholders may
appear in the repo. Replace every occurrence with the allowlisted
placeholder 123456789012.

No literal-absence regression test is added because such a test would
re-embed the very ID it guards against; the git-secrets pre-commit/CI
gate remains the enforcement point. The gate hole (bare 12-digit IDs in
string labels and comments escape the registered key=value pattern) and
the history-scrubbing decision are documented on the tracking issue.

Verified: go build ./..., go test ./internal/email/... (335 passed),
npx jest settings-accounts.test.ts (80 passed), git grep confirms zero
remaining occurrences.

Closes #1167
@cristim cristim force-pushed the fix/hyg-02-redact-account-id branch from ce21e93 to 805de41 Compare June 26, 2026 17:20
@cristim

cristim commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Rebased on current main to clear the migration 074 collision with #1261 (renumbered to 077 on main). Diff unchanged (4 files, +10/-10 string replacements). Local re-verification: go test ./internal/email/... 335 passed, npx jest src/__tests__/settings-accounts.test.ts 80 passed, git grep 540659244915 empty.

Pre-existing main CI failures (Lint errorlint/errcheck, Integration Tests, Security Scanning) are repo-wide on main HEAD and not caused by this PR (verified against latest main run #27833571269 — same failing jobs).

CodeRabbit hit a per-developer rate limit on the initial review pass and never produced findings. Re-pinging for a full review now (per project memory rule: always full review after a rate-limit skip, never the incremental form).

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@cristim Understood — kicking off a full review now, with your note that the rebased diff is unchanged and the currently failing CI jobs are pre-existing on main.


✏️ Learnings added
Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1212

Timestamp: 2026-06-26T17:21:01.353Z
Learning: For the LeanerCloud/CUDly repository review workflow, when a previous CodeRabbit review pass was skipped or failed to produce findings due to a rate-limit event, use a full review request on the pull request rather than the incremental review form.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/security Security finding urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HYG-02: Real production AWS account ID committed in tracked files, violating the repo's .gitallowed policy

1 participant