GCP and AWS interactive login, with GCP WIF provider github token support#1956
Merged
GCP and AWS interactive login, with GCP WIF provider github token support#1956
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:
📝 WalkthroughWalkthroughReplaces manual session construction with newCommandSessionWithOpts; adds Provider.Authenticate hooks; implements PKCE/DPoP OAuth flows with an in-process callback server and browser helper; introduces a TokenStore interface and LocalDirTokenStore; and propagates token/client option plumbing across AWS and GCP clients. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Session
participant Provider
participant Browser
participant Callback
participant OAuth
participant Store
User->>CLI: run command requiring auth
CLI->>Session: newCommandSessionWithOpts(CheckAccountInfo, AllowStackCreation)
Session->>Provider: Authenticate(ctx, interactive)
Provider->>Callback: start local callback server & BuildAuthURL
Provider->>Browser: prompt URL / OpenBrowserOnEnter
Browser->>OAuth: user navigates and approves
OAuth->>Callback: redirect with code
Callback->>Provider: return code (validate state)
Provider->>OAuth: exchange code (PKCE/DPoP)
OAuth-->>Provider: access/refresh tokens
Provider->>Store: Save(token metadata)
Store-->>Provider: saved
Provider-->>Session: authentication complete
Session-->>CLI: proceed with command
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pkg/clouds/gcp/artifactregistry.go (1)
12-16:⚠️ Potential issue | 🟠 MajorClose the Artifact Registry client to avoid leaking resources.
The client created at line 12 is used in subsequent operations but never closed. All return paths (lines 14, 21, 28, 38, 42, and 45) exit without cleanup, causing the underlying connection pool to leak. Add a deferred close immediately after the error check.
♻️ Proposed fix
func (gcp Gcp) EnsureArtifactRegistryExists(ctx context.Context, repoName string) (string, error) { client, err := artifactregistry.NewClient(ctx, gcp.Options...) if err != nil { return "", fmt.Errorf("failed to create artifactregistry client: %w", err) } + defer client.Close() parent := fmt.Sprintf("projects/%s/locations/%s", gcp.ProjectId, gcp.Region)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/artifactregistry.go` around lines 12 - 16, The Artifact Registry client created with artifactregistry.NewClient is never closed, leaking resources; after the error check where client is assigned (client, err := artifactregistry.NewClient(...)), add a deferred close (defer client.Close()) immediately so the client is closed on all return paths, and ensure any named return or early returns still allow the deferred Close to run; reference the artifactregistry.NewClient call and the client's Close method when making the change.src/pkg/clouds/gcp/cloudrun.go (1)
152-156:⚠️ Potential issue | 🔴 CriticalReturn on executions client creation failure to prevent nil-client panic.
At Line 153-156, the function logs constructor failure but continues;
defer client.Close()and laterclient.GetExecutioncan panic whenclientis nil.🐛 Proposed fix
func (gcp Gcp) GetExecutionEnv(ctx context.Context, executionName string) (map[string]string, error) { client, err := run.NewExecutionsClient(ctx, gcp.Options...) if err != nil { - fmt.Fprintf(os.Stderr, "failed to create cloud run executions client: %v\n", err) + return nil, fmt.Errorf("failed to create cloud run executions client: %w", err) } defer client.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/cloudrun.go` around lines 152 - 156, The code currently logs an error when run.NewExecutionsClient(ctx, gcp.Options...) fails but continues, causing a nil-client panic when defer client.Close() or client.GetExecution is called; modify the function to return immediately on error (or propagate the error) after logging so you never call defer client.Close() on a nil pointer—specifically update the block around run.NewExecutionsClient to check err and return the error (or wrapped fmt.Errorf) instead of proceeding, ensuring subsequent uses of client (defer client.Close, client.GetExecution) only run when client is non-nil.src/pkg/clouds/aws/ecs/tail.go (1)
92-106:⚠️ Potential issue | 🟡 MinorSet the region in the loaded config before calling
getTaskStatus.
TailTaskIDreloads the AWS config without accounting fora.Region. When called directly, the config may use a different region than the cluster, causinggetTaskStatusto query the wrong region's ECS API. Even when called throughTail(), the region set at line 25 is not carried forward to the newly loaded config at line 92.Align the region in the reloaded config:
🛠️ Proposed fix
cfg, err := a.LoadConfig(ctx) if err != nil { return nil, err } + if a.Region != "" { + cfg.Region = string(a.Region) + } lgi := cw.LogGroupInput{LogGroupARN: a.LogGroupARN, LogStreamNames: []string{GetCDLogStreamForTaskID(taskID)}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/ecs/tail.go` around lines 92 - 106, The reloaded AWS config from a.LoadConfig in TailTaskID does not inherit the target region (a.Region), causing getTaskStatus to call ECS in the wrong region; after calling cfg, set its region to the agent's region (e.g., assign cfg.Region = a.Region or otherwise apply a.Region into the returned config) before calling getTaskStatus so getTaskStatus(ctx, cfg, a.ClusterName, taskID) queries the correct ECS region.
🧹 Nitpick comments (8)
src/pkg/cli/client/byoc/do/byoc.go (1)
82-85: Placeholder implementation noted.The stub
Authenticatemethod satisfies theProviderinterface. The TODO indicates this needs implementation for DigitalOcean authentication support.Would you like me to help outline the implementation for DigitalOcean interactive authentication, or should I open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/do/byoc.go` around lines 82 - 85, Implement the DigitalOcean authentication flow inside the ByocDo.Authenticate(ctx context.Context, interactive bool) method: detect interactive vs non-interactive mode, validate or load existing credentials (e.g., from the ByocDo struct fields or config store), prompt the user for a Personal Access Token when interactive, securely store or return the token to the caller, and return appropriate errors on failure; update any helper functions or config loaders used by ByocDo (e.g., methods that read/write saved credentials) to persist the token and ensure Authenticate satisfies the Provider interface's expectations.src/pkg/login/login.go (2)
217-229: Consider usingfilepath.Joinfor OS paths.
path.Joinuses forward slashes regardless of OS. While this works on Unix systems,filepath.Joinis the idiomatic choice for filesystem paths as it uses the correct separator for the current OS.Suggested change
+import "path/filepath" + func writeCredentialsFile(cluster string, creds GoogleAuthCredentials) (string, error) { credsBytes, err := json.Marshal(creds) if err != nil { return "", fmt.Errorf("failed to marshal credentials: %w", err) } - credsPath := path.Join(client.StateDir, client.TokenStorageName(cluster+"-gcp-creds")) + ".json" + credsPath := filepath.Join(client.StateDir, client.TokenStorageName(cluster+"-gcp-creds")) + ".json" term.Debugf("writing credentials file to %s", credsPath) if err := os.WriteFile(credsPath, credsBytes, 0600); err != nil { return "", fmt.Errorf("failed to save credentials file: %w", err) } return credsPath, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/login/login.go` around lines 217 - 229, The code in writeCredentialsFile uses path.Join which is for URL paths; replace path.Join with filepath.Join to build OS-native filesystem paths (i.e., credsPath := filepath.Join(client.StateDir, client.TokenStorageName(cluster+"-gcp-creds")) + ".json"), and add the "path/filepath" import (removing or keeping "path" if still used elsewhere). Ensure references to client.StateDir and client.TokenStorageName remain unchanged and that file permissions and os.WriteFile call are kept as-is.
205-205: Returnnilexplicitly for clarity.At this point,
erris alwaysnil(earlier errors cause early returns). Returningnildirectly makes the success path clearer.Suggested change
- return err + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/login/login.go` at line 205, The final return in the login function currently does "return err" even though earlier returns handle all error paths and err is always nil; change that final statement to "return nil" to make the success path explicit and clearer—update the return at the end of the function (the line with "return err" that returns the variable err) to return nil instead.src/pkg/clouds/gcp/storage.go (1)
257-269: Consider includinggcp.Optionswhen creating the impersonated storage client.The impersonated token source is created with
gcp.Options...at line 261, but the subsequent storage client at line 265 only receivesoption.WithTokenSource(ts). Ifgcp.Optionscontains non-credential options (custom endpoints, HTTP client settings, etc.), they won't apply to this client.If this is intentional (impersonated clients should use defaults), the current code is fine. Otherwise, consider:
♻️ Optional: Include gcp.Options in impersonated client
- client, err := newStorageClient(ctx, option.WithTokenSource(ts)) + opts := append([]option.ClientOption{option.WithTokenSource(ts)}, gcp.Options...) + client, err := newStorageClient(ctx, opts...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/storage.go` around lines 257 - 269, The getCloudStorageClientWithServiceAccount function creates an impersonated token source with gcp.Options but then calls newStorageClient only with option.WithTokenSource(ts); update the call to newStorageClient to pass through gcp.Options as well (e.g., include gcp.Options... alongside option.WithTokenSource(ts)) so any non-credential options (custom endpoints, HTTP client, etc.) are applied to the impersonated storage client; locate getCloudStorageClientWithServiceAccount, newStorageClient, gcp.Options, and option.WithTokenSource(ts) to make the change.src/pkg/tokenstore/store.go (1)
92-94: MakeDeleteidempotent on missing files.Treat
os.ErrNotExistas success to avoid noisy warnings in expected logout paths.Suggested fix
- if err := os.Remove(tokenFile); err != nil { + if err := os.Remove(tokenFile); err != nil && !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("failed to delete token: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/tokenstore/store.go` around lines 92 - 94, The Delete implementation in tokenstore (where it calls os.Remove(tokenFile)) should treat a missing file as success; update the error handling in the Delete method to check for os.IsNotExist(err) (or compare to os.ErrNotExist) and return nil in that case, only returning fmt.Errorf("failed to delete token: %w", err) for other errors, referencing the tokenFile variable and the Delete function to locate the change.src/pkg/clouds/aws/login.go (2)
313-323: Fix typo in comment.Line 316 has "corss" which should be "cross".
✏️ Fix typo
// CrossDeviceLogin runs the cross-device flow for remote/SSH sessions where the // browser runs on a different machine. It prints the auth URL and prompts the user // to paste the base64-encoded verification code displayed in their browser. -// TODO: Support corss device login workflow with a flag +// TODO: Support cross device login workflow with a flag🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 313 - 323, The function comment for CrossDeviceLogin contains a typo "corss" which should be "cross"; update the doc comment above func (a *Aws) CrossDeviceLogin to correct "Support corss device login workflow with a flag" to "Support cross device login workflow with a flag" so the comment reads correctly and matches the function name CrossDeviceLogin.
69-96: Consider adding a buffer before token expiry to prevent race conditions.The current check
time.Now().Before(p.cached.AccessToken.ExpiresAt)uses the exact expiry time. If a request is made very close to expiry, the token might expire during the request. Adding a small buffer (e.g., 30-60 seconds) would help avoid failures.✏️ Suggested improvement
+const tokenExpiryBuffer = 60 * time.Second + func (p *awsOAuthCredentialsProvider) Retrieve(ctx context.Context) (awssdk.Credentials, error) { - if time.Now().Before(p.cached.AccessToken.ExpiresAt) { + if time.Now().Add(tokenExpiryBuffer).Before(p.cached.AccessToken.ExpiresAt) { return p.toCredentials(), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 69 - 96, The Retrieve method currently checks token validity with time.Now().Before(p.cached.AccessToken.ExpiresAt) which allows requests to start right at expiry; change this to use a safety buffer (e.g., 30s) by comparing time.Now().Add(buffer).Before(p.cached.AccessToken.ExpiresAt) (or equivalently check ExpiresAt.After(time.Now().Add(buffer))) so the provider refreshes earlier and avoids race conditions; update the check in awsOAuthCredentialsProvider.Retrieve and keep subsequent logic (refreshToken, persisting via p.tokenStore and p.storeKey, and p.cached assignment) unchanged.src/pkg/clouds/gcp/login.go (1)
94-97: Add documentation link to the error message.The TODO on line 96 indicates a documentation link should be added. Consider adding a link similar to the AWS error handling pattern seen in
byoc.go.✏️ Suggested improvement
if !interactive { - return errors.New("No valid gcloud credentials found") // TODO: Better error message with possible doc link + return errors.New("no valid GCP credentials found; please run 'gcloud auth application-default login' or set GOOGLE_APPLICATION_CREDENTIALS (https://s.defang.io/gcp-setup)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` around lines 94 - 97, The error returned when interactive is false currently says "No valid gcloud credentials found" and needs to include a documentation link and clearer guidance like the AWS pattern in byoc.go; update the return in the !interactive branch in login.go to return an error string that includes a short actionable message and a URL to the docs (e.g., "No valid gcloud credentials found; see <docs link> for setup and non-interactive configuration"), preserving the same control flow and variable names (interactive) so callers of this branch keep the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/client/byoc/aws/list.go`:
- Around line 19-25: The code ignores the passed-in region when creating the S3
client in listPulumiStacksInBucket; fix by applying the discovered region to the
AWS config before creating the s3 client (e.g., set cfg.Region = string(region)
or reload config with the Region option) so s3.NewFromConfig(cfg) targets the
correct regional endpoint; keep using b.driver.LoadConfig(ctx) but override the
Region field on the returned cfg before calling s3.NewFromConfig to avoid
reloading config per bucket.
In `@src/pkg/cli/logout.go`:
- Around line 20-21: When deleting the token with
client.TokenStore.Delete(client.TokenStorageName(fabricAddr)) in logout
handling, treat a missing token (os.ErrNotExist) as success instead of warning:
check the error returned, and only call term.Warn(...) for errors that are not
os.ErrNotExist; if the error is os.ErrNotExist simply ignore it (or return nil)
so logout is idempotent and silent for already-logged-out users.
In `@src/pkg/clouds/aws/login.go`:
- Around line 83-91: The code currently discards errors from
json.Marshal(refreshed) inside the block guarded by p.tokenStore and p.storeKey;
update the logic in the token persistence block (where json.Marshal(refreshed)
is called) to check and handle the marshal error by logging it (e.g., call
term.Warnf or term.Errorf with the error and context like p.storeKey) and avoid
attempting to save when marshal fails; keep the existing Save(p.storeKey,
string(tokenBytes)) path unchanged for the success case.
- Around line 546-551: The JWK construction currently uses pub.X.Bytes() and
pub.Y.Bytes() which drop leading zeros; change to allocate 32-byte slices and
call pub.X.FillBytes(x) and pub.Y.FillBytes(y) before
base64.RawURLEncoding.EncodeToString so coordinates are zero-padded to exactly
32 bytes per P-256; update the token.Header["jwk"] assignment to encode those
fixed-size x and y slices instead of the raw .Bytes() output.
In `@src/pkg/clouds/gcp/iam.go`:
- Around line 246-249: Replace the process-killing log.Fatalf call after
creating the IAM client with a proper error return: when
iamadm.NewIamClient(ctx, gcp.Options...) returns err, return fmt.Errorf("unable
to ensure user service account role: failed to create IAM client: %w", err) (and
add fmt to imports if missing) instead of calling log.Fatalf; keep the
surrounding code path in the function that ensures the user service account role
so upstream callers can handle the error.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 58-92: The loop over oauthTokenNames currently returns immediately
on gcp.TokenStore.Load(name) or json.Unmarshal errors which prevents trying
other tokens; modify the loop in the function handling token loading so that on
load/unmarshal errors (from gcp.TokenStore.Load and json.Unmarshal into
oauth2.Token) you log or record the error and continue to the next name instead
of returning, while preserving the existing behavior of returning on fatal
errors later (e.g., failing to retrieve currentToken from tokenSource). Ensure
you still set gcp.Options and gcp.TokenSource and persist updated tokens when
testTokenProjectPermissions(ctx, gcp.ProjectId, requiredPerms, tokenSource)
succeeds.
In `@src/pkg/term/browser.go`:
- Around line 19-21: The Ctrl-C handler currently calls cancel() but continues
the input loop; modify the input-reading switch (the case 3: // Ctrl-C branch)
to exit the goroutine immediately after calling cancel()—e.g., add a return (or
break out of the loop) right after cancel() so the reader stops and doesn't
linger. Ensure you change only the case 3 branch in the input loop where
cancel() is invoked.
In `@src/pkg/tokenstore/store.go`:
- Around line 62-69: The List method (and other methods handling keys/prefixes
such as Get/Set/Delete) currently use prefix/key directly with filepath
operations allowing path traversal; fix by normalizing and validating inputs:
use filepath.Join(s.Dir, prefix) then filepath.Clean the result, reject absolute
paths or inputs that resolve outside s.Dir (e.g., use filepath.Rel(s.Dir,
cleanedPath) and ensure it does not start with ".."), and return an error for
invalid prefixes/keys before calling os.ReadDir or other file ops; apply the
same normalization/validation to the other token-store methods referenced (e.g.,
the code around lines 99-107).
---
Outside diff comments:
In `@src/pkg/clouds/aws/ecs/tail.go`:
- Around line 92-106: The reloaded AWS config from a.LoadConfig in TailTaskID
does not inherit the target region (a.Region), causing getTaskStatus to call ECS
in the wrong region; after calling cfg, set its region to the agent's region
(e.g., assign cfg.Region = a.Region or otherwise apply a.Region into the
returned config) before calling getTaskStatus so getTaskStatus(ctx, cfg,
a.ClusterName, taskID) queries the correct ECS region.
In `@src/pkg/clouds/gcp/artifactregistry.go`:
- Around line 12-16: The Artifact Registry client created with
artifactregistry.NewClient is never closed, leaking resources; after the error
check where client is assigned (client, err := artifactregistry.NewClient(...)),
add a deferred close (defer client.Close()) immediately so the client is closed
on all return paths, and ensure any named return or early returns still allow
the deferred Close to run; reference the artifactregistry.NewClient call and the
client's Close method when making the change.
In `@src/pkg/clouds/gcp/cloudrun.go`:
- Around line 152-156: The code currently logs an error when
run.NewExecutionsClient(ctx, gcp.Options...) fails but continues, causing a
nil-client panic when defer client.Close() or client.GetExecution is called;
modify the function to return immediately on error (or propagate the error)
after logging so you never call defer client.Close() on a nil
pointer—specifically update the block around run.NewExecutionsClient to check
err and return the error (or wrapped fmt.Errorf) instead of proceeding, ensuring
subsequent uses of client (defer client.Close, client.GetExecution) only run
when client is non-nil.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/do/byoc.go`:
- Around line 82-85: Implement the DigitalOcean authentication flow inside the
ByocDo.Authenticate(ctx context.Context, interactive bool) method: detect
interactive vs non-interactive mode, validate or load existing credentials
(e.g., from the ByocDo struct fields or config store), prompt the user for a
Personal Access Token when interactive, securely store or return the token to
the caller, and return appropriate errors on failure; update any helper
functions or config loaders used by ByocDo (e.g., methods that read/write saved
credentials) to persist the token and ensure Authenticate satisfies the Provider
interface's expectations.
In `@src/pkg/clouds/aws/login.go`:
- Around line 313-323: The function comment for CrossDeviceLogin contains a typo
"corss" which should be "cross"; update the doc comment above func (a *Aws)
CrossDeviceLogin to correct "Support corss device login workflow with a flag" to
"Support cross device login workflow with a flag" so the comment reads correctly
and matches the function name CrossDeviceLogin.
- Around line 69-96: The Retrieve method currently checks token validity with
time.Now().Before(p.cached.AccessToken.ExpiresAt) which allows requests to start
right at expiry; change this to use a safety buffer (e.g., 30s) by comparing
time.Now().Add(buffer).Before(p.cached.AccessToken.ExpiresAt) (or equivalently
check ExpiresAt.After(time.Now().Add(buffer))) so the provider refreshes earlier
and avoids race conditions; update the check in
awsOAuthCredentialsProvider.Retrieve and keep subsequent logic (refreshToken,
persisting via p.tokenStore and p.storeKey, and p.cached assignment) unchanged.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 94-97: The error returned when interactive is false currently says
"No valid gcloud credentials found" and needs to include a documentation link
and clearer guidance like the AWS pattern in byoc.go; update the return in the
!interactive branch in login.go to return an error string that includes a short
actionable message and a URL to the docs (e.g., "No valid gcloud credentials
found; see <docs link> for setup and non-interactive configuration"), preserving
the same control flow and variable names (interactive) so callers of this branch
keep the same behavior.
In `@src/pkg/clouds/gcp/storage.go`:
- Around line 257-269: The getCloudStorageClientWithServiceAccount function
creates an impersonated token source with gcp.Options but then calls
newStorageClient only with option.WithTokenSource(ts); update the call to
newStorageClient to pass through gcp.Options as well (e.g., include
gcp.Options... alongside option.WithTokenSource(ts)) so any non-credential
options (custom endpoints, HTTP client, etc.) are applied to the impersonated
storage client; locate getCloudStorageClientWithServiceAccount,
newStorageClient, gcp.Options, and option.WithTokenSource(ts) to make the
change.
In `@src/pkg/login/login.go`:
- Around line 217-229: The code in writeCredentialsFile uses path.Join which is
for URL paths; replace path.Join with filepath.Join to build OS-native
filesystem paths (i.e., credsPath := filepath.Join(client.StateDir,
client.TokenStorageName(cluster+"-gcp-creds")) + ".json"), and add the
"path/filepath" import (removing or keeping "path" if still used elsewhere).
Ensure references to client.StateDir and client.TokenStorageName remain
unchanged and that file permissions and os.WriteFile call are kept as-is.
- Line 205: The final return in the login function currently does "return err"
even though earlier returns handle all error paths and err is always nil; change
that final statement to "return nil" to make the success path explicit and
clearer—update the return at the end of the function (the line with "return err"
that returns the variable err) to return nil instead.
In `@src/pkg/tokenstore/store.go`:
- Around line 92-94: The Delete implementation in tokenstore (where it calls
os.Remove(tokenFile)) should treat a missing file as success; update the error
handling in the Delete method to check for os.IsNotExist(err) (or compare to
os.ErrNotExist) and return nil in that case, only returning fmt.Errorf("failed
to delete token: %w", err) for other errors, referencing the tokenFile variable
and the Delete function to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce628fbf-0dad-4031-8279-2970a9294b69
📒 Files selected for processing (44)
src/cmd/cli/command/compose.gosrc/cmd/cli/command/config.gosrc/cmd/cli/command/session.gosrc/pkg/auth/auth.gosrc/pkg/auth/callback.gosrc/pkg/auth/client.gosrc/pkg/auth/pkce.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/cluster.gosrc/pkg/cli/client/cluster_test.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/logout.gosrc/pkg/cli/logout_test.gosrc/pkg/cli/tail.gosrc/pkg/cli/tailAndMonitor.gosrc/pkg/cli/whoami.gosrc/pkg/clouds/aws/common.gosrc/pkg/clouds/aws/cw/logs.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/gcp/account.gosrc/pkg/clouds/gcp/api.gosrc/pkg/clouds/gcp/artifactregistry.gosrc/pkg/clouds/gcp/cloudbuild.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/clouds/gcp/dns.gosrc/pkg/clouds/gcp/iam.gosrc/pkg/clouds/gcp/logging.gosrc/pkg/clouds/gcp/login.gosrc/pkg/clouds/gcp/project.gosrc/pkg/clouds/gcp/secret.gosrc/pkg/clouds/gcp/storage.gosrc/pkg/clouds/gcp/storage_test.gosrc/pkg/login/login.gosrc/pkg/login/login_test.gosrc/pkg/session/session.gosrc/pkg/term/browser.gosrc/pkg/tokenstore/store.gosrc/pkg/tokenstore/store_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/pkg/clouds/aws/login.go (2)
558-563:⚠️ Potential issue | 🟠 MajorUse fixed-size coordinate encoding for ES256 JWK fields.
pub.X.Bytes()/pub.Y.Bytes()can strip leading zeroes; this can produce non-compliant P-256 JWKx/yvalues.Proposed fix
// Add JWK header pub := key.PublicKey + x := make([]byte, 32) + y := make([]byte, 32) + pub.X.FillBytes(x) + pub.Y.FillBytes(y) token.Header["jwk"] = map[string]string{ "kty": "EC", "crv": "P-256", - "x": base64.RawURLEncoding.EncodeToString(pub.X.Bytes()), - "y": base64.RawURLEncoding.EncodeToString(pub.Y.Bytes()), + "x": base64.RawURLEncoding.EncodeToString(x), + "y": base64.RawURLEncoding.EncodeToString(y), }#!/bin/bash set -euo pipefail echo "== Go module version constraints ==" fd -a go.mod | while read -r mod; do echo "--- ${mod} ---" sed -n '1,8p' "${mod}" done echo echo "== Current JWK coordinate encoding usage in AWS login code ==" rg -n --type go -C2 'EncodeToString\(pub\.[XY]\.Bytes\(\)\)|FillBytes\(' src/pkg/clouds/aws/login.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 558 - 563, The JWK x/y values are produced from pub.X.Bytes()/pub.Y.Bytes() which can drop leading zeros; update the ES256 P-256 coordinate encoding where token.Header["jwk"] is set so that pub.X and pub.Y are encoded as fixed 32-byte big-endian values (use pub.X.FillBytes(...) and pub.Y.FillBytes(...) to produce 32-byte slices) before calling base64.RawURLEncoding.EncodeToString; ensure both coordinates are zero-padded to 32 bytes to produce RFC-compliant JWK x/y fields.
84-92:⚠️ Potential issue | 🟡 MinorHandle refreshed-token marshal failures explicitly.
json.Marshal(refreshed)failures are silently ignored, which can hide persistence regressions.Proposed fix
if p.tokenStore != nil && p.storeKey != "" { tokenBytes, err := json.Marshal(refreshed) - if err == nil { + if err != nil { + term.Warnf("failed to marshal refreshed AWS OAuth token for %q: %v", p.storeKey, err) + } else { if err := p.tokenStore.Save(p.storeKey, string(tokenBytes)); err != nil { term.Warnf("failed to persist refreshed AWS OAuth token: %v", err) } else { term.Debugf("persisted refreshed AWS OAuth token for %q", p.storeKey) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 84 - 92, The code currently ignores errors from json.Marshal(refreshed) inside the block guarded by p.tokenStore and p.storeKey; update the logic in the token persistence section (the block using json.Marshal, p.tokenStore.Save and term.Warnf/Debugf) to handle marshal failures explicitly by checking the marshal error and calling term.Warnf (or term.Errorf as appropriate) with the error and context (e.g., "failed to marshal refreshed AWS OAuth token: %v") and do not attempt p.tokenStore.Save when marshal failed; keep the existing Save/error logging path for successful marshals and consider returning or propagating the marshal error if the surrounding function expects error handling.src/pkg/clouds/gcp/login.go (1)
162-170:⚠️ Potential issue | 🟡 MinorContinue iterating when one stored token is unreadable/corrupt.
Returning on Line 165 or Line 169 aborts evaluation of other stored tokens that may still be valid.
🛠️ Proposed fix
for _, name := range oauthTokenNames { tokenJson, err := gcp.TokenStore.Load(name) if err != nil { - return fmt.Errorf("failed to load token %q: %w", name, err) + term.Debugf("failed to load token %q: %v", name, err) + continue } var token oauth2.Token if err = json.Unmarshal([]byte(tokenJson), &token); err != nil { - return fmt.Errorf("failed to unmarshal token %q: %w", name, err) + term.Debugf("failed to unmarshal token %q: %v", name, err) + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` around lines 162 - 170, The loop over oauthTokenNames aborts on the first load/unmarshal error (calls return inside the loop), so unreadable/corrupt stored tokens prevent checking others; change the behavior in the loop (in the function containing oauthTokenNames and calls to gcp.TokenStore.Load and json.Unmarshal) to skip the failing token and continue iterating: log or collect the specific error for the token name (instead of returning) when gcp.TokenStore.Load(name) or json.Unmarshal fails, and only return an error after the loop if no valid token was found or aggregate errors as appropriate.
🧹 Nitpick comments (1)
src/pkg/clouds/gcp/login.go (1)
314-314: Usefilepath.Joinfor local filesystem paths.At Line 314, prefer
filepath.Joinoverpath.Joinfor OS-native path handling.♻️ Proposed fix
- credsPath := path.Join(client.StateDir, key+"-gcp-creds") + ".json" + credsPath := filepath.Join(client.StateDir, key+"-gcp-creds.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` at line 314, The code constructs a local filesystem path using path.Join in the credsPath assignment (credsPath := path.Join(client.StateDir, key+"-gcp-creds") + ".json"); change this to use filepath.Join for OS-native behavior (use filepath.Join(client.StateDir, key+"-gcp-creds") + ".json") and update imports to import "path/filepath" (remove or stop using the "path" package for local paths). Ensure any other nearby uses of path.Join for filesystem paths are similarly switched to filepath.Join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/aws/login.go`:
- Around line 226-229: The current check "if err != nil || identity.Arn == nil"
can return (nil, nil) when err==nil but identity.Arn==nil; change the logic so
that if err != nil you log and return that error, and if err==nil but
identity.Arn==nil you log and return a real error (e.g. errors.New or
fmt.Errorf) describing "missing caller identity ARN" instead of returning nil;
update the branch around identity.Arn, the term.Debugf call, and the return to
ensure callers never receive a nil error when identity.Arn is absent.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 26-28: Replace the real-looking literal values for clientID and
clientSecret with explicit non-secret placeholders (e.g. "<REDACTED_CLIENT_ID>"
and "<PUBLIC_CLIENT_SECRET_PLACEHOLDER>"), remove any example secret values from
comments, and add a clear secret-scanner allowlist comment (for example: "//
secret-scanner: allowlist - public placeholder, not a secret") adjacent to the
clientID/clientSecret declarations so scanners ignore these intentional
placeholders; ensure the scopes slice (scopes) remains unchanged and
audit/comment any other literals around lines referencing clientID/clientSecret
(including the example at line 52) to use the same redaction + allowlist
pattern.
---
Duplicate comments:
In `@src/pkg/clouds/aws/login.go`:
- Around line 558-563: The JWK x/y values are produced from
pub.X.Bytes()/pub.Y.Bytes() which can drop leading zeros; update the ES256 P-256
coordinate encoding where token.Header["jwk"] is set so that pub.X and pub.Y are
encoded as fixed 32-byte big-endian values (use pub.X.FillBytes(...) and
pub.Y.FillBytes(...) to produce 32-byte slices) before calling
base64.RawURLEncoding.EncodeToString; ensure both coordinates are zero-padded to
32 bytes to produce RFC-compliant JWK x/y fields.
- Around line 84-92: The code currently ignores errors from
json.Marshal(refreshed) inside the block guarded by p.tokenStore and p.storeKey;
update the logic in the token persistence section (the block using json.Marshal,
p.tokenStore.Save and term.Warnf/Debugf) to handle marshal failures explicitly
by checking the marshal error and calling term.Warnf (or term.Errorf as
appropriate) with the error and context (e.g., "failed to marshal refreshed AWS
OAuth token: %v") and do not attempt p.tokenStore.Save when marshal failed; keep
the existing Save/error logging path for successful marshals and consider
returning or propagating the marshal error if the surrounding function expects
error handling.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 162-170: The loop over oauthTokenNames aborts on the first
load/unmarshal error (calls return inside the loop), so unreadable/corrupt
stored tokens prevent checking others; change the behavior in the loop (in the
function containing oauthTokenNames and calls to gcp.TokenStore.Load and
json.Unmarshal) to skip the failing token and continue iterating: log or collect
the specific error for the token name (instead of returning) when
gcp.TokenStore.Load(name) or json.Unmarshal fails, and only return an error
after the loop if no valid token was found or aggregate errors as appropriate.
---
Nitpick comments:
In `@src/pkg/clouds/gcp/login.go`:
- Line 314: The code constructs a local filesystem path using path.Join in the
credsPath assignment (credsPath := path.Join(client.StateDir, key+"-gcp-creds")
+ ".json"); change this to use filepath.Join for OS-native behavior (use
filepath.Join(client.StateDir, key+"-gcp-creds") + ".json") and update imports
to import "path/filepath" (remove or stop using the "path" package for local
paths). Ensure any other nearby uses of path.Join for filesystem paths are
similarly switched to filepath.Join.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a021603f-05c0-4cdd-9222-109c1bd1d7ad
📒 Files selected for processing (4)
src/pkg/clouds/aws/login.gosrc/pkg/clouds/aws/login_test.gosrc/pkg/clouds/gcp/login.gosrc/pkg/login/login.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/login/login.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/clouds/aws/common.go (1)
65-68:⚠️ Potential issue | 🔴 CriticalExplicit credentials can be bypassed by CLI credentials in the current chain order.
At Line 65-68,
newChainProvider(cliProvider, cfg.Credentials)tries AWS CLI process credentials first. The chain iterates through providers in order and returns on first success, so if AWS CLI credentials are available, the explicit credentials passed viaconfig.WithCredentialsProvider(...)are never consulted. This causes validation calls (e.g.,src/pkg/clouds/aws/login.goLine 264 and Line 598) to authenticate against CLI credentials instead of the intended provider, potentially invalidating role assumption security checks.Proposed fix
- cfg.Credentials = newChainProvider( - cliProvider, - cfg.Credentials, - ) + cfg.Credentials = newChainProvider( + cfg.Credentials, // explicit/default chain first + cliProvider, // CLI fallback + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/common.go` around lines 65 - 68, The chain currently calls newChainProvider(cliProvider, cfg.Credentials) which makes CLI process credentials take precedence over explicit credentials set via config.WithCredentialsProvider(...); change the chain order so the explicit provider is first: construct the chain with cfg.Credentials before cliProvider (e.g., newChainProvider(cfg.Credentials, cliProvider)), and ensure you handle a nil cfg.Credentials by falling back to cliProvider (or only wrapping when cfg.Credentials != nil) so explicit credentials always win when provided; update the use sites that rely on cfg.Credentials (e.g., newChainProvider call and any initialization that sets cfg.Credentials) accordingly.
♻️ Duplicate comments (2)
src/pkg/clouds/gcp/iam.go (1)
247-247:⚠️ Potential issue | 🟡 MinorFix inaccurate client name in the returned error.
Line 247 says “artifact registry client” in an IAM client initialization path. This makes troubleshooting noisy.
🛠️ Proposed fix
- return fmt.Errorf("unable to ensure user service account role, failed to create artifact registry client: %w", err) + return fmt.Errorf("unable to ensure user service account role, failed to create IAM client: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/iam.go` at line 247, The error message returned when initializing the IAM client is mislabeled as "artifact registry client"; update the fmt.Errorf call in the IAM initialization/ensureUserServiceAccountRole path to reference "IAM client" (or the exact symbol like iam client) instead of "artifact registry client" and keep the wrapped error (err) intact so the returned error becomes something like "unable to ensure user service account role, failed to create IAM client: %w".src/pkg/clouds/gcp/login.go (1)
26-27:⚠️ Potential issue | 🟡 MinorSecret-scanner signal is still noisy with credential-like literals.
Line 27 continues to look like an API secret to scanners. Even if this value is intentionally public, this pattern tends to produce high-noise alerts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` around lines 26 - 27, The clientID and clientSecret package-level literals in login.go (clientID, clientSecret) trigger secret scanners; replace these hardcoded credential-like strings with values loaded from configuration or environment variables (e.g., read from os.Getenv or a secure config provider during init or from a function that returns the OAuth config), remove the literal values and associated nolint comment, and ensure any callers of clientID/clientSecret use the new config accessors or variables so no credential-like literals remain in the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/aws/login.go`:
- Around line 494-497: The token HTTP call uses http.DefaultClient with no
timeout, which can hang; replace that call so the request has a bounded timeout
by either creating a dedicated http.Client with a Timeout (e.g., client :=
&http.Client{Timeout: 10 * time.Second} and use client.Do(req)) or by attaching
a context with deadline (ctx, cancel := context.WithTimeout(req.Context(),
10*time.Second); defer cancel(); req = req.WithContext(ctx)) before calling Do.
Update the token request code path (the block that calls
http.DefaultClient.Do(req) and processes resp, err) to use the timed client or
request context and import time/context as needed.
- Around line 270-274: The role-validation path calls GetAccountID(roleArn)
directly (in the block around a.testCredentials and identity.Account) which can
panic on malformed ARNs; change this to safely parse/validate the ARN once
before use (e.g., call a safeParse/GetAccountIDWithError variant or wrap
GetAccountID in a validation that returns an error), store the parsed account id
in a local variable, and use that variable in the comparison and error message
instead of calling GetAccountID twice; if parsing fails, return a clear error
(including the offending roleArn and profile) rather than panicking.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 106-107: The code dereferences gcp.TokenStore without nil checks
causing possible panics; update Authenticate, findStoredCredentials, and
tryInteractiveLogin to guard all TokenStore uses: in Authenticate (where
findStoredCredentials is called) skip calling findStoredCredentials if
gcp.TokenStore is nil or ensure findStoredCredentials immediately returns nil
without dereferencing when TokenStore == nil; in findStoredCredentials add an
early return when gcp.TokenStore == nil; and in tryInteractiveLogin, check
gcp.TokenStore for nil before calling TokenStore.Save(...) and handle the
no-store case gracefully (e.g., skip save and log a warning). Ensure these
changes reference the methods Authenticate, findStoredCredentials, and
tryInteractiveLogin and do not change other behavior.
---
Outside diff comments:
In `@src/pkg/clouds/aws/common.go`:
- Around line 65-68: The chain currently calls newChainProvider(cliProvider,
cfg.Credentials) which makes CLI process credentials take precedence over
explicit credentials set via config.WithCredentialsProvider(...); change the
chain order so the explicit provider is first: construct the chain with
cfg.Credentials before cliProvider (e.g., newChainProvider(cfg.Credentials,
cliProvider)), and ensure you handle a nil cfg.Credentials by falling back to
cliProvider (or only wrapping when cfg.Credentials != nil) so explicit
credentials always win when provided; update the use sites that rely on
cfg.Credentials (e.g., newChainProvider call and any initialization that sets
cfg.Credentials) accordingly.
---
Duplicate comments:
In `@src/pkg/clouds/gcp/iam.go`:
- Line 247: The error message returned when initializing the IAM client is
mislabeled as "artifact registry client"; update the fmt.Errorf call in the IAM
initialization/ensureUserServiceAccountRole path to reference "IAM client" (or
the exact symbol like iam client) instead of "artifact registry client" and keep
the wrapped error (err) intact so the returned error becomes something like
"unable to ensure user service account role, failed to create IAM client: %w".
In `@src/pkg/clouds/gcp/login.go`:
- Around line 26-27: The clientID and clientSecret package-level literals in
login.go (clientID, clientSecret) trigger secret scanners; replace these
hardcoded credential-like strings with values loaded from configuration or
environment variables (e.g., read from os.Getenv or a secure config provider
during init or from a function that returns the OAuth config), remove the
literal values and associated nolint comment, and ensure any callers of
clientID/clientSecret use the new config accessors or variables so no
credential-like literals remain in the source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dff20e3f-ae3e-4734-9bcd-389269479f77
📒 Files selected for processing (10)
src/pkg/cli/client/byoc/aws/list.gosrc/pkg/clouds/aws/common.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/gcp/artifactregistry.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/clouds/gcp/iam.gosrc/pkg/clouds/gcp/login.gosrc/pkg/term/browser.gosrc/pkg/tokenstore/store.gosrc/pkg/tokenstore/store_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pkg/tokenstore/store_test.go
- src/pkg/tokenstore/store.go
- src/pkg/clouds/gcp/artifactregistry.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pkg/tokenstore/store.go (1)
82-85:⚠️ Potential issue | 🔴 CriticalFix path containment validation;
HasPrefixallows sibling-directory escape.Using
strings.HasPrefixfor filesystem containment is unsafe (/tmp/tokens2/...passes for base/tmp/tokens). This can still escapeDirin bothListandgetTokenFile.🔒 Proposed fix
@@ - if !strings.HasPrefix(dir, baseDir) { + relDir, err := filepath.Rel(baseDir, dir) + if err != nil { + return nil, fmt.Errorf("failed to compute relative token directory: %w", err) + } + if relDir == ".." || strings.HasPrefix(relDir, ".."+string(filepath.Separator)) { term.Warnf("Invalid token prefix %q: resolved directory %q is outside of token store base directory %q", prefix, dir, baseDir) return nil, errors.New("invalid token prefix") } @@ - if !strings.HasPrefix(absTokenFilePath, absDir) { + relFile, err := filepath.Rel(absDir, absTokenFilePath) + if err != nil { + return "", fmt.Errorf("failed to compute relative token file path: %w", err) + } + if relFile == ".." || strings.HasPrefix(relFile, ".."+string(filepath.Separator)) { return "", errors.New("invalid token key") }#!/bin/bash # Verify vulnerable pattern still exists in this file. rg -n 'HasPrefix\((dir|absTokenFilePath),\s*(baseDir|absDir)\)' src/pkg/tokenstore/store.go # Demonstrate why prefix checks are unsafe for path containment. python - <<'PY' import os base = "/tmp/tokens" candidate = os.path.abspath(os.path.join(base, "../tokens2/secret")) print("base =", os.path.abspath(base)) print("candidate =", candidate) print("startswith=", candidate.startswith(os.path.abspath(base))) # True (unsafe) print("relpath =", os.path.relpath(candidate, os.path.abspath(base))) # ../tokens2/secret PYAlso applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/tokenstore/store.go` around lines 82 - 85, The path containment check using strings.HasPrefix is unsafe and can be bypassed by sibling paths; update the validation in the functions that compute dir/baseDir and absTokenFilePath/absDir (used in List and getTokenFile) to use filepath.Abs on both paths and then filepath.Rel (or equivalent) to obtain the relative path and reject if the relative path starts with ".." or is an absolute escape; apply the same fix for the second occurrence around lines 133-135 so both List and getTokenFile properly ensure candidate paths are contained within the token store base directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pkg/tokenstore/store.go`:
- Around line 82-85: The path containment check using strings.HasPrefix is
unsafe and can be bypassed by sibling paths; update the validation in the
functions that compute dir/baseDir and absTokenFilePath/absDir (used in List and
getTokenFile) to use filepath.Abs on both paths and then filepath.Rel (or
equivalent) to obtain the relative path and reject if the relative path starts
with ".." or is an absolute escape; apply the same fix for the second occurrence
around lines 133-135 so both List and getTokenFile properly ensure candidate
paths are contained within the token store base directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7c37b34-519f-4889-996e-895aa50a9a31
📒 Files selected for processing (1)
src/pkg/tokenstore/store.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pkg/clouds/gcp/login.go (1)
147-149:⚠️ Potential issue | 🔴 CriticalGuard token persistence when
TokenStoreis not configured.
gcp.TokenStore.Save(...)is called unconditionally at Line 147. If interactive login is used without a configured store, this can panic on a nil interface.🐛 Proposed fix
- if err := gcp.TokenStore.Save(tokenName, string(bytes)); err != nil { - return fmt.Errorf("failed to save token: %w", err) - } + if gcp.TokenStore != nil { + if err := gcp.TokenStore.Save(tokenName, string(bytes)); err != nil { + return fmt.Errorf("failed to save token: %w", err) + } + } else { + term.Debug("token store is nil; skipping token persistence") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` around lines 147 - 149, The code calls gcp.TokenStore.Save(tokenName, string(bytes)) unconditionally which can panic if gcp.TokenStore is nil; guard the persistence by checking if gcp.TokenStore != nil (or otherwise configured) before calling Save and only attempt to Save when present, otherwise skip saving (or return a non-fatal path); reference TokenStore, Save, tokenName and bytes when locating the call to update.src/pkg/clouds/aws/login.go (1)
270-274:⚠️ Potential issue | 🔴 CriticalHandle assume-role failures deterministically and avoid malformed-ARN panic.
At Line 270, assume-role failures are only handled when
identity.Account != nil; ifidentity.Account == nil, this falls through and returnsassumeRoleProvideras if success. Also, Lines 272-273 callGetAccountID(roleArn), which can panic on malformed ARN input.🛠️ Proposed fix
- if _, err := a.testCredentials(ctx, assumeRoleProvider); err != nil && identity.Account != nil { - // If unable to assume, and also not the same account, then this token is not valid for the specified AWS_PROFILE role - if *identity.Account != GetAccountID(roleArn) { - return nil, fmt.Errorf("login successful, but does not have access to role %q in used by stack aws profile %q; token account %v does not match stack aws profile account %v", roleArn, profile, *identity.Account, GetAccountID(roleArn)) - } - // If cannot assume but it's the same account, we assume its a valid token - term.Warnf("login successful for AWS account %v which is same as the account specified by stack aws profile %q, assume its valid", *identity.Account, profile) - return creds, nil - } + if _, err := a.testCredentials(ctx, assumeRoleProvider); err != nil { + parsedRoleArn, parseErr := arn.Parse(roleArn) + if parseErr != nil { + return nil, fmt.Errorf("invalid AWS_PROFILE role ARN %q for profile %q: %w", roleArn, profile, parseErr) + } + if identity.Account == nil { + return nil, fmt.Errorf("unable to assume AWS_PROFILE role %q: %w", roleArn, err) + } + // If unable to assume, and also not the same account, then this token is not valid for the specified AWS_PROFILE role + if *identity.Account != parsedRoleArn.AccountID { + return nil, fmt.Errorf("login successful, but does not have access to role %q used by stack aws profile %q; token account %v does not match stack aws profile account %v", roleArn, profile, *identity.Account, parsedRoleArn.AccountID) + } + // If cannot assume but it's the same account, we assume it's a valid token + term.Warnf("login successful for AWS account %v which is same as the account specified by stack aws profile %q, assuming it's valid", *identity.Account, profile) + return creds, nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 270 - 274, The current logic treats assume-role failures as success when identity.Account == nil and calls GetAccountID(roleArn) without validating the ARN; change the branch around a.testCredentials and assumeRoleProvider so that if a.testCredentials(ctx, assumeRoleProvider) returns an error you return that error (wrap with context) immediately instead of falling through, and if no error but identity.Account == nil return a clear error about missing identity account info rather than proceeding; also avoid calling GetAccountID(roleArn) blindly—either call a safe parser that returns (accountID, error) or validate roleArn first and handle parse errors before comparing to *identity.Account (use symbols a.testCredentials, assumeRoleProvider, identity.Account, GetAccountID, roleArn, profile).
🧹 Nitpick comments (3)
src/pkg/clouds/gcp/login.go (1)
120-123: Fix typo in helper nametryInteraciveLogin.Please rename to
tryInteractiveLoginfor consistency and easier code search/navigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` around lines 120 - 123, Rename the misspelled helper function tryInteraciveLogin to tryInteractiveLogin and update all references (e.g., the call return gcp.tryInteraciveLogin(ctx, 3)) to use the new name; ensure the method receiver signature func (gcp *Gcp) tryInteractiveLogin(ctx context.Context, n int) error is updated and that any tests or callers compile after the rename.src/pkg/clouds/aws/login.go (2)
253-253: Use debug level whenAWS_PROFILEis unset.Line 253 currently warns for a normal environment state, which may create noisy/non-actionable output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` at line 253, The log at term.Warn on AWS_PROFILE being unset is using a warning level for a normal non-actionable state; change that call to a debug-level log (e.g., term.Debug or term.Debugf) so unconfigured AWS_PROFILE emits debug output rather than a warning. Locate the call to term.Warn in the AWS login flow (in src/pkg/clouds/aws/login.go where AWS_PROFILE is checked) and replace it with the appropriate term.Debug/Debugf invocation preserving the message text and any formatting.
168-179: Persist token after profile validation to avoid storing unusable sessions.
tryInteractiveLoginsaves the token beforetestCredentialsWithProfile. If validation fails, unusable tokens are still persisted and repeatedly retried later.♻️ Refactor sketch
- var storeKey string - if a.TokenStore != nil { - tokenBytes, err := json.Marshal(cached) - if err != nil { - return nil, fmt.Errorf("failed to marshal token: %w", err) - } - sum := sha256.Sum256([]byte(cached.LoginSession)) - storeKey = fmt.Sprintf("%s%x", tokenStoreKeyPrefix, sum) - if err := a.TokenStore.Save(storeKey, string(tokenBytes)); err != nil { - term.Warnf("failed to save AWS OAuth token: %v", err) - } - } + var storeKey string + if a.TokenStore != nil { + sum := sha256.Sum256([]byte(cached.LoginSession)) + storeKey = fmt.Sprintf("%s%x", tokenStoreKeyPrefix, sum) + } @@ creds, err := a.testCredentialsWithProfile(ctx, storeKey, provider) if err != nil { term.Warnf("Cannot use login credentials: %v, please try again.", err) continue } + if a.TokenStore != nil { + tokenBytes, err := json.Marshal(cached) + if err != nil { + return nil, fmt.Errorf("failed to marshal token: %w", err) + } + if err := a.TokenStore.Save(storeKey, string(tokenBytes)); err != nil { + term.Warnf("failed to save AWS OAuth token: %v", err) + } + } return creds, nilAlso applies to: 183-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 168 - 179, The code currently marshals and saves the AWS OAuth token (using TokenStore, tokenBytes, storeKey, tokenStoreKeyPrefix and cached.LoginSession) before validating it via testCredentialsWithProfile, causing invalid sessions to be persisted; change the flow in tryInteractiveLogin so you first call testCredentialsWithProfile and only on success marshal the cached token and call TokenStore.Save (constructing storeKey the same way from cached.LoginSession), remove the early save path, and apply the same change to the other save site (the block around the later use of tokenBytes/storeKey at lines ~183-187) so tokens are only persisted after successful profile validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/aws/login.go`:
- Around line 528-531: GetAccountID(loginSession) can panic when the subject
isn't an ARN; replace the panic-prone call with a safe extraction that returns
an error: update GetAccountID to return (string, error) (or add a new safe
helper like ParseAccountIDFromSubject) and here call it, check the error and
return fmt.Errorf("failed to extract account ID from login session: %w", err)
instead of letting the process panic; ensure any other callers of GetAccountID
are updated to handle the error return.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 327-329: The code in testTokenProjectPermissions currently returns
a generic error for missingPerms which conflates auth gaps with transport/client
failures; change the return to a distinct typed error (e.g., define a
MissingPermissionsError or ErrMissingPermissions type that holds the
missingPerms slice and implements error) and return that from
testTokenProjectPermissions instead of fmt.Errorf("token is missing required
permissions: %v", missingPerms); update callers to branch on that concrete type
(type assertion or errors.As on MissingPermissionsError) so IAM permission
failures are handled differently from transport or client errors.
---
Duplicate comments:
In `@src/pkg/clouds/aws/login.go`:
- Around line 270-274: The current logic treats assume-role failures as success
when identity.Account == nil and calls GetAccountID(roleArn) without validating
the ARN; change the branch around a.testCredentials and assumeRoleProvider so
that if a.testCredentials(ctx, assumeRoleProvider) returns an error you return
that error (wrap with context) immediately instead of falling through, and if no
error but identity.Account == nil return a clear error about missing identity
account info rather than proceeding; also avoid calling GetAccountID(roleArn)
blindly—either call a safe parser that returns (accountID, error) or validate
roleArn first and handle parse errors before comparing to *identity.Account (use
symbols a.testCredentials, assumeRoleProvider, identity.Account, GetAccountID,
roleArn, profile).
In `@src/pkg/clouds/gcp/login.go`:
- Around line 147-149: The code calls gcp.TokenStore.Save(tokenName,
string(bytes)) unconditionally which can panic if gcp.TokenStore is nil; guard
the persistence by checking if gcp.TokenStore != nil (or otherwise configured)
before calling Save and only attempt to Save when present, otherwise skip saving
(or return a non-fatal path); reference TokenStore, Save, tokenName and bytes
when locating the call to update.
---
Nitpick comments:
In `@src/pkg/clouds/aws/login.go`:
- Line 253: The log at term.Warn on AWS_PROFILE being unset is using a warning
level for a normal non-actionable state; change that call to a debug-level log
(e.g., term.Debug or term.Debugf) so unconfigured AWS_PROFILE emits debug output
rather than a warning. Locate the call to term.Warn in the AWS login flow (in
src/pkg/clouds/aws/login.go where AWS_PROFILE is checked) and replace it with
the appropriate term.Debug/Debugf invocation preserving the message text and any
formatting.
- Around line 168-179: The code currently marshals and saves the AWS OAuth token
(using TokenStore, tokenBytes, storeKey, tokenStoreKeyPrefix and
cached.LoginSession) before validating it via testCredentialsWithProfile,
causing invalid sessions to be persisted; change the flow in tryInteractiveLogin
so you first call testCredentialsWithProfile and only on success marshal the
cached token and call TokenStore.Save (constructing storeKey the same way from
cached.LoginSession), remove the early save path, and apply the same change to
the other save site (the block around the later use of tokenBytes/storeKey at
lines ~183-187) so tokens are only persisted after successful profile
validation.
In `@src/pkg/clouds/gcp/login.go`:
- Around line 120-123: Rename the misspelled helper function tryInteraciveLogin
to tryInteractiveLogin and update all references (e.g., the call return
gcp.tryInteraciveLogin(ctx, 3)) to use the new name; ensure the method receiver
signature func (gcp *Gcp) tryInteractiveLogin(ctx context.Context, n int) error
is updated and that any tests or callers compile after the rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 090f9834-2bbb-462f-8c33-7f99091715cc
📒 Files selected for processing (2)
src/pkg/clouds/aws/login.gosrc/pkg/clouds/gcp/login.go
jordanstephens
approved these changes
Mar 9, 2026
lionello
approved these changes
Mar 11, 2026
Co-authored-by: Lio李歐 <lionello@users.noreply.github.com>
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
4 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.
Description
Initiate interactive oauth login for both GCP and AWS when no correct credentials supplied from the cli environment.
For GCP login, when both
ACTIONS_ID_TOKEN_REQUEST_URLandGOOGLE_WORKLOAD_IDENTITY_PROVIDERis present in the environment, handle the workload identity federation provider setup by creating a credentials.json to make use the retrieved github token.Linked Issues
Fixes: #1946 #1948
Checklist
Summary by CodeRabbit
New Features
Refactor