feat: support env credential files#1863
Open
aihes wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
the
size/M
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCredential providers now accept values from direct environment variables or validated ChangesCredential file support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant readCredentialValue
participant SafeEnvFilePath
participant vfs.ReadFile
Provider->>readCredentialValue: Resolve credential
readCredentialValue->>SafeEnvFilePath: Validate *_FILE path
SafeEnvFilePath-->>readCredentialValue: Canonical path
readCredentialValue->>vfs.ReadFile: Read file contents
vfs.ReadFile-->>readCredentialValue: Credential value
readCredentialValue-->>Provider: Value and source
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
extension/credential/env/env.go (1)
52-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing
_FILEvariant in error message.Unlike the sibling messages at Lines 45 and 47 which use
tokenName(envvars.CliAppID, envvars.CliAppIDFile), this message only referencesenvvars.CliAppID, omitting the file-based alternative. Since AppID can now come from either variable, the error should mention both.🐛 Proposed fix
if appID == "" { - return nil, &credential.BlockError{Provider: "env", Reason: envvars.CliAppSecret + " is set but " + envvars.CliAppID + " is missing"} + return nil, &credential.BlockError{Provider: "env", Reason: envvars.CliAppSecret + " is set but " + tokenName(envvars.CliAppID, envvars.CliAppIDFile) + " is missing"} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/credential/env/env.go` around lines 52 - 54, Update the missing-AppID error in the env credential validation flow to use tokenName with envvars.CliAppID and envvars.CliAppIDFile, matching the sibling errors and mentioning both supported variable forms. Keep the existing provider and missing-secret context unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extension/credential/env/env_test.go`:
- Around line 396-403: Update the writeEnvFile test helper to create fixtures
with os.WriteFile instead of vfs.WriteFile, and replace the vfs import with the
standard library os import. Preserve the existing path construction, file
permissions, error handling, and returned path.
In `@extension/credential/env/env.go`:
- Around line 154-160: Update the file-backed credential loading flow around
vfs.ReadFile to stat the path first and reject files larger than
maxCredentialFileBytes before reading any contents. Preserve the existing
BlockError provider, reason context, and successful data handling, while
ensuring the size guard also covers files that would otherwise be fully loaded
or never reach EOF.
---
Outside diff comments:
In `@extension/credential/env/env.go`:
- Around line 52-54: Update the missing-AppID error in the env credential
validation flow to use tokenName with envvars.CliAppID and envvars.CliAppIDFile,
matching the sibling errors and mentioning both supported variable forms. Keep
the existing provider and missing-secret context unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7b26749-fcc6-4a0a-8244-7f0dae7a18d6
📒 Files selected for processing (6)
extension/credential/env/env.goextension/credential/env/env_test.gointernal/envvars/envvars.gointernal/validate/path.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.go
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
Add file-backed credential inputs for the env credential provider so sandboxed agents can pass app IDs and short-lived access tokens via protected files instead of expanding token values into command-line arguments or shell environment setup scripts.
Changes
LARKSUITE_CLI_APP_ID_FILE,LARKSUITE_CLI_USER_ACCESS_TOKEN_FILE, andLARKSUITE_CLI_TENANT_ACCESS_TOKEN_FILE.SafeEnvFilePathvalidation for environment-provided credential files.Test Plan
go test ./internal/vfs/localfileio ./internal/validate ./internal/envvars ./extension/credential/envgit diff --checkgofmton changed Go fileslark-cli <domain> <command>flow works as expected with real file-backed credentialsRelated Issues
Summary by CodeRabbit
*_FILEenvironment variables, including application IDs and access tokens.