feature(aws): add org discovery and account blacklist support#735
Merged
ShubhamRasal merged 10 commits intodevfrom Mar 10, 2026
Merged
feature(aws): add org discovery and account blacklist support#735ShubhamRasal merged 10 commits intodevfrom
ShubhamRasal merged 10 commits intodevfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds AWS Organizations-based account discovery to the AWS provider: when Changes
Sequence DiagramsequenceDiagram
participant Provider as AWS Provider
participant Session as AWS Session
participant STS as STS
participant OrgAPI as AWS Organizations API
Provider->>Provider: Detect OrgDiscoveryRoleArn configured
Provider->>Session: Build session/config for org discovery
Session->>STS: AssumeRole(OrgDiscoveryRoleArn)
STS-->>Session: Return temporary credentials
Provider->>OrgAPI: ListAccounts (using temporary creds)
OrgAPI-->>Provider: Return accounts (filtered by status)
Provider->>Provider: Filter out ExcludeAccountIds
Provider->>Provider: Append discovered accounts to AccountIds
Provider->>Provider: Finalize AccountIds for downstream use
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/schema/schema.go (1)
243-257:⚠️ Potential issue | 🟠 MajorScalar
exclude_account_idsvalues are silently dropped.After adding
exclude_account_idsto this special-case branch, scalar YAML values (for example,exclude_account_ids: "111,222") no longer get stored because only[]interface{}is handled here.💡 Proposed fix
case "account_ids", "exclude_account_ids", "urls", "services": if valueArr, ok := value.([]interface{}); ok { var strArr []string for _, v := range valueArr { switch v := v.(type) { case string: strArr = append(strArr, v) case int: strArr = append(strArr, fmt.Sprint(v)) default: return fmt.Errorf("unsupported type %T in %s", v, key) } } (*ob)[key] = strings.Join(strArr, ",") + } else { + (*ob)[key] = fmt.Sprint(value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/schema.go` around lines 243 - 257, The branch that handles keys "account_ids", "exclude_account_ids", "urls", "services" only processes value when it's a []interface{} (valueArr) and silently drops scalar inputs; update the handling in the same switch case so that if value is a scalar (string/int or other simple types) you convert it to a string (using fmt.Sprint) and assign it to (*ob)[key] directly (same comma-joined format used for arrays), while retaining the existing loop for []interface{} to build strArr; reference the variables and symbols key, value, valueArr, strArr and the map pointer ob to locate and implement the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/providers/aws/aws.go`:
- Around line 107-111: The CSV parsing for account IDs in the assignments to
p.AccountIds and p.ExcludeAccountIds can produce empty/invalid entries (e.g.,
trailing commas) because it only strips literal spaces; update the parsing to
split, trim whitespace for each element, filter out empty strings (and
optionally validate the ID format), then dedupe for p.AccountIds and assign the
cleaned list to p.ExcludeAccountIds; locate the code around p.AccountIds =
sliceutil.Dedupe(...) and the block.GetMetadata(excludeAccountIds) branch to
implement the per-item Trim + filter + (optional) validation step before
dedup/assignment.
- Around line 212-218: Change discoverOrgAccounts to accept a context.Context
and propagate the caller's context when invoking it from the block that checks
options.OrgDiscoveryRoleArn; update the call site to pass the ctx (e.g.
provider.discoverOrgAccounts(ctx, sess, config)). Inside discoverOrgAccounts,
replace blocking SDK calls with their context-aware variants (use
sts.AssumeRoleWithContext instead of AssumeRole, and
organizations.ListAccountsPagesWithContext instead of ListAccountsPages) and
ensure any other AWS requests are invoked with the provided ctx so the operation
can be cancelled. Ensure function signature and all internal calls are updated
accordingly.
---
Outside diff comments:
In `@pkg/schema/schema.go`:
- Around line 243-257: The branch that handles keys "account_ids",
"exclude_account_ids", "urls", "services" only processes value when it's a
[]interface{} (valueArr) and silently drops scalar inputs; update the handling
in the same switch case so that if value is a scalar (string/int or other simple
types) you convert it to a string (using fmt.Sprint) and assign it to (*ob)[key]
directly (same comma-joined format used for arrays), while retaining the
existing loop for []interface{} to build strArr; reference the variables and
symbols key, value, valueArr, strArr and the map pointer ob to locate and
implement the fix.
Incremental Security Review: Commit 8426962Incremental review of commit 8426962 (since a3f680d): No security vulnerabilities were fixed. All 4 previously reported exploitable vulnerabilities remain unaddressed. This commit only added YAML formatting improvements for numeric account IDs. Changes in This CommitFiles Modified: pkg/schema/schema.go, pkg/schema/schema_test.go, pkg/providers/aws/aws.go What Changed:
Purpose: Ensures numeric account IDs in YAML config (e.g., Security Status - All 4 Vulnerabilities UNFIXED (4 items)Verification of the 4 previously reported vulnerabilities shows none have been addressed in this commit: 🟠 ❌ ARN Injection via Account IDs (Line 278) `pkg/providers/aws/aws.go:278`Account IDs from options.AccountIds[0] are still used in fmt.Sprintf ARN construction without validation. No regex check (^[0-9]{12}$) was added. Attacker can inject malicious ARN components. 🟠 ❌ ARN Injection via AssumeRoleName (Line 278) `pkg/providers/aws/aws.go:278`AssumeRoleName is still used in fmt.Sprintf ARN construction without validation. No regex check (^[\w+=,.@-]+$) was added. Attacker can inject malicious role names. 🟡 ❌ Missing Whitespace Trimming (Lines 108, 111) `pkg/providers/aws/aws.go:108,111`CSV parsing with strings.Split still lacks strings.TrimSpace(). Whitespace in 'account_ids: "111, 222"' allows bypass of exclude_account_ids filter. Open inline comment #2862641646 tracks this issue. 🟡 ❌ Missing ARN Format Validation (Line 320) `pkg/providers/aws/aws.go:320`OrgDiscoveryRoleArn is still passed directly to AWS STS AssumeRole without validating it matches ARN format (^arn:aws:iam::[0-9]{12}:role/.+). Open inline comment #2862641647 tracks this issue. Verification SummaryReview Method: Incremental diff analysis between a3f680d → 8426962 Code Inspection Results:
Scanners: TruffleHog (0 secrets), Semgrep (0 findings), ast-grep (0 findings) 📌 Key Takeaways
|
![neo-by-projectdiscovery-dev[bot]](https://avatars.githubusercontent.com/in/2878468?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/providers/aws/aws.go (2)
217-218:⚠️ Potential issue | 🟠 MajorUse context-aware AWS SDK calls in org discovery.
Line 217 invokes org discovery without context, and Line 321/Line 341 use blocking SDK methods (
AssumeRole,ListAccountsPages). This can stall initialization and prevents cancellation/timeout control.💡 Proposed fix
- discovered, err := provider.discoverOrgAccounts(sess, config) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + discovered, err := provider.discoverOrgAccounts(ctx, sess, config)-func (p *Provider) discoverOrgAccounts(sess *session.Session, config *aws.Config) ([]string, error) { +func (p *Provider) discoverOrgAccounts(ctx context.Context, sess *session.Session, config *aws.Config) ([]string, error) { stsClient := sts.New(sess) @@ - assumeOut, err := stsClient.AssumeRole(roleInput) + assumeOut, err := stsClient.AssumeRoleWithContext(ctx, roleInput) @@ - err = orgClient.ListAccountsPages(&organizations.ListAccountsInput{}, func(page *organizations.ListAccountsOutput, lastPage bool) bool { + err = orgClient.ListAccountsPagesWithContext(ctx, &organizations.ListAccountsInput{}, func(page *organizations.ListAccountsOutput, lastPage bool) bool { ... })#!/bin/bash # Verify org discovery uses context-aware AWS SDK methods and context in signature. rg -n 'func \(p \*Provider\) discoverOrgAccounts\(|AssumeRoleWithContext\(|AssumeRole\(|ListAccountsPagesWithContext\(|ListAccountsPages\(' pkg/providers/aws/aws.goAs per coding guidelines, "Always pass and honor context.Context in Resources() and long-running API calls to support cancellation".
Also applies to: 310-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` around lines 217 - 218, The discoverOrgAccounts flow is using blocking AWS SDK calls without context; update Provider.discoverOrgAccounts to accept a context.Context (ensure Resources() passes it through) and replace synchronous calls: use sts.AssumeRoleWithContext instead of AssumeRole and organizations.ListAccountsPagesWithContext instead of ListAccountsPages (or their v2 equivalents), passing the provided ctx to each AWS call and honoring ctx cancellation in loops and retries; ensure any session or client creation used by discoverOrgAccounts can accept/propagate the context as needed and update all call sites (e.g., where discoverOrgAccounts is invoked) to supply the context.
106-111:⚠️ Potential issue | 🟠 MajorTrim and filter CSV account IDs before dedupe.
strings.Split(..., ",")on raw metadata still keeps whitespace and empty tokens (for example trailing commas), which can produce invalid account IDs and exclusion mismatches.💡 Proposed fix
+ parseCSV := func(raw string) []string { + parts := strings.Split(raw, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + p = strings.TrimSpace(p) + if p != "" { + out = append(out, p) + } + } + return out + } + if accountIds, ok := block.GetMetadata(accountIds); ok { - p.AccountIds = sliceutil.Dedupe(strings.Split(accountIds, ",")) + p.AccountIds = sliceutil.Dedupe(parseCSV(accountIds)) } if eids, ok := block.GetMetadata(excludeAccountIds); ok { - p.ExcludeAccountIds = sliceutil.Dedupe(strings.Split(eids, ",")) + p.ExcludeAccountIds = sliceutil.Dedupe(parseCSV(eids)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` around lines 106 - 111, The metadata parsing currently uses strings.Split(..., ",") then sliceutil.Dedupe which leaves whitespace and empty tokens; update the blocks that set p.AccountIds and p.ExcludeAccountIds (the branches using block.GetMetadata) to split the CSV, map strings.TrimSpace over each token, filter out empty strings, then call sliceutil.Dedupe on the cleaned slice so only valid, trimmed account IDs are stored; target the code around block.GetMetadata, p.AccountIds/p.ExcludeAccountIds, strings.Split and sliceutil.Dedupe when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/providers/aws/aws.go`:
- Around line 217-218: The discoverOrgAccounts flow is using blocking AWS SDK
calls without context; update Provider.discoverOrgAccounts to accept a
context.Context (ensure Resources() passes it through) and replace synchronous
calls: use sts.AssumeRoleWithContext instead of AssumeRole and
organizations.ListAccountsPagesWithContext instead of ListAccountsPages (or
their v2 equivalents), passing the provided ctx to each AWS call and honoring
ctx cancellation in loops and retries; ensure any session or client creation
used by discoverOrgAccounts can accept/propagate the context as needed and
update all call sites (e.g., where discoverOrgAccounts is invoked) to supply the
context.
- Around line 106-111: The metadata parsing currently uses strings.Split(...,
",") then sliceutil.Dedupe which leaves whitespace and empty tokens; update the
blocks that set p.AccountIds and p.ExcludeAccountIds (the branches using
block.GetMetadata) to split the CSV, map strings.TrimSpace over each token,
filter out empty strings, then call sliceutil.Dedupe on the cleaned slice so
only valid, trimmed account IDs are stored; target the code around
block.GetMetadata, p.AccountIds/p.ExcludeAccountIds, strings.Split and
sliceutil.Dedupe when implementing this change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/providers/aws/aws.go (2)
107-112:⚠️ Potential issue | 🟠 MajorNormalize and sanitize CSV account IDs before dedupe.
strings.Splitdirectly on raw CSV still leaves whitespace and empty entries (e.g., trailing commas), which breaks exclusion matching and can produce invalid role ARNs downstream.💡 Suggested fix
if accountIds, ok := block.GetMetadata(accountIds); ok { - p.AccountIds = sliceutil.Dedupe(strings.Split(accountIds, ",")) + parts := strings.Split(accountIds, ",") + parsed := make([]string, 0, len(parts)) + for _, id := range parts { + id = strings.TrimSpace(id) + if id != "" { + parsed = append(parsed, id) + } + } + p.AccountIds = sliceutil.Dedupe(parsed) } if eids, ok := block.GetMetadata(excludeAccountIds); ok { - p.ExcludeAccountIds = sliceutil.Dedupe(strings.Split(eids, ",")) + parts := strings.Split(eids, ",") + parsed := make([]string, 0, len(parts)) + for _, id := range parts { + id = strings.TrimSpace(id) + if id != "" { + parsed = append(parsed, id) + } + } + p.ExcludeAccountIds = sliceutil.Dedupe(parsed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` around lines 107 - 112, The CSV parsing of account IDs currently uses strings.Split and then sliceutil.Dedupe, which leaves whitespace and empty entries; update the parsing for both accountIds and excludeAccountIds (the branches that set p.AccountIds and p.ExcludeAccountIds after block.GetMetadata) to: split the raw string, trim whitespace from each token, discard any empty tokens (including those from trailing commas), then pass the cleaned slice into sliceutil.Dedupe so stored IDs are normalized and free of blanks before further ARN construction or matching.
218-218:⚠️ Potential issue | 🟠 MajorPropagate context through org discovery AWS calls.
Org discovery currently uses blocking non-context SDK calls, so initialization cannot be canceled promptly under AWS/API stalls.
💡 Suggested fix
+import "time" ... - discovered, err := provider.discoverOrgAccounts(sess, config) + discoveryCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + discovered, err := provider.discoverOrgAccounts(discoveryCtx, sess, config)-func (p *Provider) discoverOrgAccounts(sess *session.Session, config *aws.Config) ([]string, error) { +func (p *Provider) discoverOrgAccounts(ctx context.Context, sess *session.Session, config *aws.Config) ([]string, error) ([]string, error) { ... - assumeOut, err := stsClient.AssumeRole(roleInput) + assumeOut, err := stsClient.AssumeRoleWithContext(ctx, roleInput) ... - err = orgClient.ListAccountsPages(&organizations.ListAccountsInput{}, func(page *organizations.ListAccountsOutput, lastPage bool) bool { + err = orgClient.ListAccountsPagesWithContext(ctx, &organizations.ListAccountsInput{}, func(page *organizations.ListAccountsOutput, lastPage bool) bool {#!/bin/bash set -euo pipefail # Verify non-context org discovery calls still exist (should return no matches after fix) rg -nP 'discoverOrgAccounts\(|\.AssumeRole\(|\.ListAccountsPages\(' pkg/providers/aws/aws.go # Verify context-aware variants are used (should return matches after fix) rg -n 'AssumeRoleWithContext|ListAccountsPagesWithContext' pkg/providers/aws/aws.goAs per coding guidelines, "Always pass and honor context.Context in Resources() and long-running API calls to support cancellation".
Also applies to: 316-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` at line 218, The org discovery code calls blocking AWS SDK methods without a context; update discoverOrgAccounts (and its callers such as the Resources() path that triggers it) to accept and propagate a context.Context parameter, and replace blocking SDK calls (e.g., AssumeRole and ListAccountsPages) with their context-aware variants (AssumeRoleWithContext, ListAccountsPagesWithContext), passing the same ctx through AssumeRoleWithContext, the STS/client calls, and any paginator/page callbacks so cancellation is honored; ensure discoverOrgAccounts returns promptly on ctx cancellation and update any call sites that invoke provider.discoverOrgAccounts to supply the context (also apply the same changes to the other discovery block around the 316-354 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/providers/aws/aws.go`:
- Around line 241-243: The current gologger.Info().Msgf call in the AWS provider
(referencing options.AccountIds and options.AssumeRoleName) prints the full list
of account IDs; change it to avoid emitting full account lists at info level by
logging only the count and role name (e.g., "Will assume role X in N accounts")
or by logging a redacted/limited sample at debug level; update the call site
that invokes gologger.Info().Msgf to either use gologger.Debug().Msgf for the
full list or remove the joined options.AccountIds from the message and keep only
len(options.AccountIds), ensuring no raw account IDs are written to info logs.
---
Duplicate comments:
In `@pkg/providers/aws/aws.go`:
- Around line 107-112: The CSV parsing of account IDs currently uses
strings.Split and then sliceutil.Dedupe, which leaves whitespace and empty
entries; update the parsing for both accountIds and excludeAccountIds (the
branches that set p.AccountIds and p.ExcludeAccountIds after block.GetMetadata)
to: split the raw string, trim whitespace from each token, discard any empty
tokens (including those from trailing commas), then pass the cleaned slice into
sliceutil.Dedupe so stored IDs are normalized and free of blanks before further
ARN construction or matching.
- Line 218: The org discovery code calls blocking AWS SDK methods without a
context; update discoverOrgAccounts (and its callers such as the Resources()
path that triggers it) to accept and propagate a context.Context parameter, and
replace blocking SDK calls (e.g., AssumeRole and ListAccountsPages) with their
context-aware variants (AssumeRoleWithContext, ListAccountsPagesWithContext),
passing the same ctx through AssumeRoleWithContext, the STS/client calls, and
any paginator/page callbacks so cancellation is honored; ensure
discoverOrgAccounts returns promptly on ctx cancellation and update any call
sites that invoke provider.discoverOrgAccounts to supply the context (also apply
the same changes to the other discovery block around the 316-354 region).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/providers/aws/aws.go (2)
107-112:⚠️ Potential issue | 🟠 MajorNormalize and filter CSV account IDs before storing them.
Line 108 and Line 111 still keep whitespace and empty tokens. That breaks
exclude_account_idsmatching for inputs like"111..., 222...", and a trailing comma can leave"", which later produces an invalid role ARN increateAssumedRoleSession.Suggested fix
if accountIds, ok := block.GetMetadata(accountIds); ok { - p.AccountIds = sliceutil.Dedupe(strings.Split(accountIds, ",")) + parts := strings.Split(accountIds, ",") + parsed := make([]string, 0, len(parts)) + for _, id := range parts { + id = strings.TrimSpace(id) + if id != "" { + parsed = append(parsed, id) + } + } + p.AccountIds = sliceutil.Dedupe(parsed) } if eids, ok := block.GetMetadata(excludeAccountIds); ok { - p.ExcludeAccountIds = sliceutil.Dedupe(strings.Split(eids, ",")) + parts := strings.Split(eids, ",") + parsed := make([]string, 0, len(parts)) + for _, id := range parts { + id = strings.TrimSpace(id) + if id != "" { + parsed = append(parsed, id) + } + } + p.ExcludeAccountIds = sliceutil.Dedupe(parsed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` around lines 107 - 112, The values returned from block.GetMetadata for accountIds and excludeAccountIds must be split, trimmed, filtered for empty tokens, deduplicated, and then assigned to p.AccountIds/p.ExcludeAccountIds to avoid whitespace and empty-string entries that break createAssumedRoleSession; update the code that uses strings.Split(..., ",") + sliceutil.Dedupe to instead map over the split tokens with strings.TrimSpace, drop any token == "" before calling sliceutil.Dedupe, and assign the result to p.AccountIds and p.ExcludeAccountIds respectively (references: block.GetMetadata, p.AccountIds, p.ExcludeAccountIds, sliceutil.Dedupe, strings.Split, createAssumedRoleSession).
218-218:⚠️ Potential issue | 🟠 Major
context.Background()here defeats the new cancellation path.Line 218 still creates an uncancellable context for organization discovery, so
AssumeRoleWithContextandListAccountsPagesWithContextcan block provider construction until the SDK times out. IfNewcannot accept a caller context, bound this call with a timeout or move discovery into a ctx-aware phase. As per coding guidelines, "Always pass and honor context.Context in Resources() and long-running API calls to support cancellation".Minimal mitigation
+ discoveryCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() - discovered, err := provider.discoverOrgAccounts(context.Background(), sess, config) + discovered, err := provider.discoverOrgAccounts(discoveryCtx, sess, config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/aws/aws.go` at line 218, The discoverOrgAccounts call currently uses context.Background() which bypasses caller cancellation; change the call site so provider.discoverOrgAccounts receives a caller-controlled context (propagate an existing ctx parameter into New/constructor or Resources) or, if that API cannot be changed immediately, wrap the call with a bounded cancellable context (context.WithTimeout or WithCancel) that is derived from the caller's context and deferred cancelled; update calls to AssumeRoleWithContext/ListAccountsPagesWithContext inside discoverOrgAccounts to use that propagated ctx so discovery honors cancellation and timeouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/providers/aws/aws.go`:
- Around line 218-223: The current New() call swallows errors from
provider.discoverOrgAccounts(...) by only logging a warning; change the behavior
to fail fast when org discovery was explicitly requested: check the
configuration flag/field that indicates org discovery (e.g., OrgDiscoveryRoleArn
or org_discovery_role_arn on the passed config) and if it is set and
discoverOrgAccounts returns an error, return that error from New() (or wrap it
with context) instead of continuing; keep the existing Info() path when
discovery succeeds. Also update any callers/tests that expect New() to error on
configured org discovery failure.
---
Duplicate comments:
In `@pkg/providers/aws/aws.go`:
- Around line 107-112: The values returned from block.GetMetadata for accountIds
and excludeAccountIds must be split, trimmed, filtered for empty tokens,
deduplicated, and then assigned to p.AccountIds/p.ExcludeAccountIds to avoid
whitespace and empty-string entries that break createAssumedRoleSession; update
the code that uses strings.Split(..., ",") + sliceutil.Dedupe to instead map
over the split tokens with strings.TrimSpace, drop any token == "" before
calling sliceutil.Dedupe, and assign the result to p.AccountIds and
p.ExcludeAccountIds respectively (references: block.GetMetadata, p.AccountIds,
p.ExcludeAccountIds, sliceutil.Dedupe, strings.Split, createAssumedRoleSession).
- Line 218: The discoverOrgAccounts call currently uses context.Background()
which bypasses caller cancellation; change the call site so
provider.discoverOrgAccounts receives a caller-controlled context (propagate an
existing ctx parameter into New/constructor or Resources) or, if that API cannot
be changed immediately, wrap the call with a bounded cancellable context
(context.WithTimeout or WithCancel) that is derived from the caller's context
and deferred cancelled; update calls to
AssumeRoleWithContext/ListAccountsPagesWithContext inside discoverOrgAccounts to
use that propagated ctx so discovery honors cancellation and timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1bbc55d-89da-44ba-822a-fd5e31a6cc9a
📒 Files selected for processing (1)
pkg/providers/aws/aws.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/schema/schema.go (1)
243-253:⚠️ Potential issue | 🟠 MajorRequire quoted strings for AWS account IDs.
account_idsandexclude_account_idsare identifiers and must always be quoted in YAML. Unquoted numeric values can cause issues: YAML with leading zeros (e.g.,012345678901) parses asfloat64, causing an "unsupported type" error, while values without leading zeros parse asintand lose the distinction. Reject numeric types entirely for these fields to prevent user confusion and ensure consistent string-based handling.Suggested implementation
case "account_ids", "exclude_account_ids", "urls", "services", "project_ids": if valueArr, ok := value.([]interface{}); ok { var strArr []string for _, v := range valueArr { switch v := v.(type) { case string: strArr = append(strArr, v) case int: + if key == "account_ids" || key == "exclude_account_ids" { + return fmt.Errorf("%s entries must be quoted strings", key) + } strArr = append(strArr, fmt.Sprint(v)) default: return fmt.Errorf("unsupported type %T in %s", v, key) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/schema.go` around lines 243 - 253, The validation currently accepts ints for keys "account_ids" and "exclude_account_ids" (converting them to strings), but we must reject numeric types for those two keys so AWS account IDs are always quoted strings; update the block that iterates valueArr (the switch on v.(type) that appends to strArr) to: if key == "account_ids" || key == "exclude_account_ids" then only accept case string and return an error for any non-string type (use a clear message like "account_ids must be quoted strings"), otherwise keep the existing behavior (allow string and int -> fmt.Sprint) so other keys ("urls","services","project_ids") remain unchanged; reference variables: key, valueArr, v, strArr in pkg/schema/schema.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/schema/schema.go`:
- Around line 243-253: The validation currently accepts ints for keys
"account_ids" and "exclude_account_ids" (converting them to strings), but we
must reject numeric types for those two keys so AWS account IDs are always
quoted strings; update the block that iterates valueArr (the switch on v.(type)
that appends to strArr) to: if key == "account_ids" || key ==
"exclude_account_ids" then only accept case string and return an error for any
non-string type (use a clear message like "account_ids must be quoted strings"),
otherwise keep the existing behavior (allow string and int -> fmt.Sprint) so
other keys ("urls","services","project_ids") remain unchanged; reference
variables: key, valueArr, v, strArr in pkg/schema/schema.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d977374-1edf-4de5-a661-4d9b9367d75c
📒 Files selected for processing (1)
pkg/schema/schema.go
…nd add tests for account ID parsing and zero-padding
ShubhamRasal
approved these changes
Mar 10, 2026
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
2 participants
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 by CodeRabbit
New Features
Bug Fixes