Skip to content

fix: extend offline secret caching to token-based auth and fix connection check #272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devin-ai-integration wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −2
Open

fix: extend offline secret caching to token-based auth and fix connection check #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e532e7
fix: extend offline secret caching to token-based auth and fix connec…
devin-ai-integration[bot] Jun 19, 2026
d7e51f3
fix: close resp.Body in connection check and restrict cache fallback …
devin-ai-integration[bot] Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/util/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@ func WriteToFile(fileName string, dataToWrite []byte, filePerm os.FileMode) erro
}

func ValidateInfisicalAPIConnection() (ok bool) {
_, err := http.Get(fmt.Sprintf("%v/status", config.INFISICAL_URL))
return err == nil
resp, err := http.Get(fmt.Sprintf("%v/status", config.INFISICAL_URL))

@greptile-apps greptile-apps Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security SSRF via user-controlled INFISICAL_URL

config.INFISICAL_URL is user-supplied and passed directly to http.Get. An attacker who can influence this value (e.g., via a misconfigured environment variable) could use it to probe internal network endpoints — making the CLI an SSRF oracle. Consider validating that the URL is a known-safe external host, or at minimum ensuring the scheme is https and the host is not a private/loopback address. This vector existed before this PR but the change makes it more prominent as the health-check path is now load-bearing for the offline fallback decision.

Context Used: Flag SSRF risks (source)

@devin-ai-integration devin-ai-integration Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — this is a pre-existing pattern (the URL was already used in http.Get before this PR). Hardening INFISICAL_URL validation is a good idea but out of scope for this bugfix PR.

if err != nil {
return false
}
defer resp.Body.Close()
return resp.StatusCode >= 200 && resp.StatusCode < 300
Comment on lines +44 to +49

@greptile-apps greptile-apps Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The response body is never closed after http.Get succeeds, leaking the underlying TCP connection. Per Go's net/http docs, callers must always close resp.Body, even if they don't read from it — otherwise the HTTP client cannot reuse the connection and the goroutine/socket is leaked. Since ValidateInfisicalAPIConnection is called on every CLI invocation that touches secrets, repeated calls will accumulate leaks.

Suggested change
resp, err := http.Get(fmt.Sprintf("%v/status", config.INFISICAL_URL))
if err != nil {
return false
}
return resp.StatusCode >= 200 && resp.StatusCode < 300
resp, err := http.Get(fmt.Sprintf("%v/status", config.INFISICAL_URL))
if err != nil {
return false
}
defer resp.Body.Close()
return resp.StatusCode >= 200 && resp.StatusCode < 300

@devin-ai-integration devin-ai-integration Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Added defer resp.Body.Close() to prevent the connection leak.

}

func GetRestyClientWithCustomHeaders() (*resty.Client, error) {
Expand Down
27 changes: 27 additions & 0 deletions packages/util/secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,33 @@ func GetAllEnvironmentVariables(params models.GetAllSecretsParameters, projectCo
errorToReturn = err
secretsToReturn = res.Secrets
}

// cache secrets on success, fallback to cached secrets on connection/server failure
if errorToReturn == nil && params.WorkspaceId != "" {
if backupEncryptionKey, err := GetBackupEncryptionKey(); err == nil {
WriteBackupSecrets(params.WorkspaceId, params.Environment, params.SecretsPath, backupEncryptionKey, secretsToReturn)
}
} else if errorToReturn != nil && params.WorkspaceId != "" {
// Only fall back to cache for connection errors or server errors (5xx).
// Do not mask client errors (4xx) like 401/403 which indicate auth issues.
shouldFallback := true
var apiErr *api.APIError
if errors.As(errorToReturn, &apiErr) && apiErr.StatusCode >= 400 && apiErr.StatusCode < 500 {
shouldFallback = false
}

if shouldFallback {
backupEncryptionKey, _ := GetBackupEncryptionKey()
if backupEncryptionKey != nil {
backedUpSecrets, err := ReadBackupSecrets(params.WorkspaceId, params.Environment, params.SecretsPath, backupEncryptionKey)
if len(backedUpSecrets) > 0 {
PrintWarning("Unable to fetch the latest secret(s) due to connection error, serving secrets from last successful fetch. For more info, run with --debug")
secretsToReturn = backedUpSecrets
errorToReturn = err
}
}
}
}
Comment on lines +383 to +403

@greptile-apps greptile-apps Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fallback triggered on all errors, not just connection failures

The user-auth path (line 349) only reads from cache when !isConnected, so auth errors (401/403) propagate correctly. The new token-based path falls back on any errorToReturn != nil, meaning a revoked token or permission error will silently serve stale cached secrets instead of surfacing the error to the caller. A user whose token is rotated would see old secrets with only a vague "connection error" warning, with no indication that their token is actually invalid.

@devin-ai-integration devin-ai-integration Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed. The fallback now only triggers on connection errors or server errors (5xx). Client errors (4xx) like 401/403 are no longer masked — they propagate as-is so the user sees an actionable auth failure message instead of stale cached secrets.

shouldFallback := true
var apiErr *api.APIError
if errors.As(errorToReturn, &apiErr) && apiErr.StatusCode >= 400 && apiErr.StatusCode < 500 {
    shouldFallback = false
}

}

return secretsToReturn, errorToReturn
Expand Down
Loading