Fix session token expiry with automatic re-authentication#27
Fix session token expiry with automatic re-authentication#27c1-dev-bot[bot] wants to merge 3 commits intomainfrom
Conversation
The op CLI session token expires after 30 minutes of inactivity, causing subsequent syncs to fail with "no active session". Store account details in OnePasswordClient and automatically re-authenticate when a session expiry error is detected, then retry the failed command. Fixes: CXH-1183
|
Claude finished @c1-dev-bot[bot]'s task in 1m 22s —— View job PR Review: Fix session token expiry with automatic re-authentication
Overall AssessmentThis is a well-structured change that solves a real operational problem — session tokens expiring during long syncs. The approach of retry-once on session expiry is sound and appropriately scoped. What's Good
Issues & Suggestions1. Token logged in error output (security concern) 2. Case sensitivity in error matching (minor) 3. Error wrapping (minor) 4. Concurrency note (informational) 5. No tests for the new logic VerdictLooks good overall — the core logic is correct and the refactoring is clean. The token-in-logs issue (point 1) is worth addressing before merge. The other items are minor suggestions. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return strings.Contains(stderr, "no active session") || | ||
| strings.Contains(stderr, "session expired") || | ||
| strings.Contains(stderr, "not currently signed in") || | ||
| strings.Contains(stderr, "Invalid session token") |
There was a problem hiding this comment.
Nit: case sensitivity could be an issue. The op CLI error messages may vary across versions. Consider using strings.Contains(strings.ToLower(stderr), ...) with lowercase patterns for more robust matching. However, this is low-risk given you're matching known 1Password CLI output.
| } | ||
|
|
||
| c.token = token | ||
| return nil |
There was a problem hiding this comment.
Good: the refreshToken method correctly stores credentials in the client struct, but note that AccountDetails contains sensitive fields (password, secret). This is acceptable since they were already being stored in the OnePassword connector struct — now they're also referenced in the client. Just be mindful that OnePasswordClient now holds credential material for its entire lifetime.
Also, c.token = token is not concurrency-safe. If executeCommand were ever called from multiple goroutines, this would be a data race. Currently the SDK drives execution sequentially so this is fine, but worth noting with a comment if this assumption is important.
| cmdArgs := make([]string, len(args)) | ||
| copy(cmdArgs, args) |
There was a problem hiding this comment.
Good fix here — the original code had a subtle bug where args was mutated in place (args = append(args, ...)) which could modify the caller's slice. The new runCommand correctly copies the args before modifying them. This is a nice side-effect of the refactoring.
pkg/client/client.go
Outdated
| // If the session token has expired and we have account details to re-authenticate, retry once. | ||
| if c.authType == "user" && c.accountDetails != nil && isSessionExpiredError(err) { | ||
| if refreshErr := c.refreshToken(ctx); refreshErr != nil { | ||
| return fmt.Errorf("error: %w (token refresh also failed: %v)", err, refreshErr) |
There was a problem hiding this comment.
This error message includes the refresh failure via %v but wraps the original error via %w. That's fine, but the refreshErr won't be unwrappable. Consider also using errors.Join or fmt.Errorf with %w for both if callers might want to inspect the refresh error. Not blocking, but a suggestion for better error ergonomics:
| return fmt.Errorf("error: %w (token refresh also failed: %v)", err, refreshErr) | |
| return fmt.Errorf("session expired error: %w (token refresh also failed: %w)", err, refreshErr) |
Note: Go 1.20+ supports multiple %w verbs in a single fmt.Errorf.
- Use %w instead of %v for refreshErr to satisfy errorlint - Use exec.CommandContext in runCommand to satisfy noctx
|
Claude finished @c1-dev-bot[bot]'s task in 1m 57s —— View job PR Review: Fix session token expiry with automatic re-authentication
Blocking Issues: 1 | Suggestions: 2 Security Issues
Correctness IssuesNone found. Suggestions
NotesThe overall approach is sound — retry-once on session expiry with auth-type gating is well-scoped. The |
| return strings.Contains(stderr, "no active session") || | ||
| strings.Contains(stderr, "session expired") || | ||
| strings.Contains(stderr, "not currently signed in") || | ||
| strings.Contains(stderr, "Invalid session token") |
There was a problem hiding this comment.
🟡 Suggestion: The string patterns use mixed casing — "no active session" (lowercase) vs "Invalid session token" (capitalized). If the op CLI ever changes casing across versions, some patterns could silently stop matching. Consider normalizing with strings.ToLower:
| strings.Contains(stderr, "Invalid session token") | |
| stderr := strings.ToLower(string(exitErr.Stderr)) | |
| return strings.Contains(stderr, "no active session") || | |
| strings.Contains(stderr, "session expired") || | |
| strings.Contains(stderr, "not currently signed in") || | |
| strings.Contains(stderr, "invalid session token") |
|
|
||
| if c.authType == "user" { | ||
| args = append(args, []string{"--session", c.token}...) | ||
| cmdArgs = append(cmdArgs, "--session", c.token) |
There was a problem hiding this comment.
🔴 Security (H5): In runCommand, on error, zap.Strings("command_args", cmd.Args) (line 449) will log the session token since --session <token> is appended to cmdArgs on line 433. Consider removing the command_args log field entirely, or redacting the --session value before logging.
This is the most important finding — credentials in logs can propagate to log aggregators and persist indefinitely.
Use exec.CommandContext instead of exec.Command in GetLocalAccounts, AddLocalAccount, and SignIn to respect context cancellation.
|
Claude finished @c1-dev-bot[bot]'s task in 1m 18s —— View job PR Review: Fix session token expiry with automatic re-authentication
Overall AssessmentThis is a well-scoped fix for a real operational issue — the What's Good
Issues1. Session token logged in error output —
Fix: Remove the 2. Case sensitivity in error matching —
stderr := strings.ToLower(string(exitErr.Stderr))
return strings.Contains(stderr, "no active session") ||
strings.Contains(stderr, "session expired") ||
strings.Contains(stderr, "not currently signed in") ||
strings.Contains(stderr, "invalid session token")Low risk given known CLI output, but a simple defensive improvement. Fix this → Suggestions (non-blocking)
SummaryThe core logic is correct and the refactoring is well done. The token-in-logs issue (point 1) should be addressed before merge as it's a security concern. The case-sensitivity fix (point 2) is low-effort and worth doing. The rest are minor suggestions. |
|
CI Note: The This PR actually fixes 3 pre-existing |
Summary
The
opCLI session token expires after 30 minutes of inactivity, causing subsequent syncs to fail with "no active session found". The token was obtained once at startup and stored as a static string — there was no mechanism to detect expiry or re-authenticate.Changes:
AccountDetailsinOnePasswordClientso re-authentication is possibleisSessionExpiredError()to detect session expiry fromopCLI stderr output (matches "no active session", "session expired", "not currently signed in", "Invalid session token")executeCommandto extractrunCommandhelper, and on session expiry errors foruserauth type, automatically callrefreshToken()(which re-signs in viaGetUserToken) and retry the failed command onceNewClito acceptaccountDetailsparameter, and thread it through fromconnector.NewTest plan
go build ./...passesgo test ./...passesFixes: CXH-1183
Automated PR Notice
This PR was automatically created by c1-dev-bot as a potential implementation.
This code requires: