feat: support custom data dir and log directories#302
feat: support custom data dir and log directories#302JackZhao10086 wants to merge 2 commits intomainfrom
Conversation
github-actions
bot
added
the
size/L
📝 WalkthroughWalkthroughTwo functions now check environment variables first before falling back to defaults: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/auth_log.go`:
- Around line 24-26: The returned LARKSUITE_CLI_LOG_DIR value is used later by
vfs.MkdirAll and vfs.OpenFile and must be validated; update the function in
auth_log.go that reads os.Getenv("LARKSUITE_CLI_LOG_DIR") to call
validate.SafeInputPath (or the project’s equivalent) on the value and only
return it if validation succeeds, otherwise fall back to the existing default
path or an empty string and log/handle the invalid input; ensure the validation
happens before any use in vfs.MkdirAll or vfs.OpenFile so the path cannot be
used without passing validate.SafeInputPath.
In `@internal/keychain/keychain_other.go`:
- Around line 28-29: The function currently returns the raw
LARKSUITE_CLI_DATA_DIR env value (os.Getenv("LARKSUITE_CLI_DATA_DIR")) without
appending the service name, causing data/credential collisions; change the
branch in StorageDir to return filepath.Join(dir, service) instead of dir so the
environment override preserves per-service isolation (mirror the default branch
which uses filepath.Join(xdgData, service)).
- Around line 28-30: The code returns the LARKSUITE_CLI_DATA_DIR env value
directly; validate this user-controlled path with validate.SafeInputPath before
returning/using it to prevent path traversal or malformed paths. Modify the
function that reads os.Getenv("LARKSUITE_CLI_DATA_DIR") to call
validate.SafeInputPath(dir) and handle validation errors (fallback to default
dir or return an error) so all subsequent uses (the functions reading files in
this package) receive a validated path.
- Around line 28-30: The macOS StorageDir implementation (StorageDir in
keychain_darwin.go) is missing the LARKSUITE_CLI_DATA_DIR environment override
present in the other implementation (keychain_other.go); add the same env check
at the start of StorageDir in keychain_darwin.go to return that dir if set (or,
alternatively, add a comment in StorageDir documenting that the override is
intentionally Linux-only). Update the StorageDir function in keychain_darwin.go
to mirror the logic: read os.Getenv("LARKSUITE_CLI_DATA_DIR") and return it when
non-empty before falling back to the existing macOS directory resolution.
🪄 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: 00e3fb58-540e-40ac-8609-d64e0fdb7d01
📒 Files selected for processing (2)
internal/keychain/auth_log.gointernal/keychain/keychain_other.go
| if dir := os.Getenv("LARKSUITE_CLI_LOG_DIR"); dir != "" { | ||
| return dir | ||
| } |
There was a problem hiding this comment.
Validate the environment variable path before use.
The LARKSUITE_CLI_LOG_DIR environment variable is user-controlled input that is returned directly and later used in file I/O operations (line 48: vfs.MkdirAll, line 54: vfs.OpenFile). Without validation, malicious or malformed paths could cause path traversal vulnerabilities or unexpected filesystem behavior.
As per coding guidelines: Validate paths using validate.SafeInputPath before any file I/O operations.
🛡️ Proposed fix to add path validation
+import (
+ "github.com/larksuite/cli/internal/validate"
+)
+
func authLogDir() string {
if dir := os.Getenv("LARKSUITE_CLI_LOG_DIR"); dir != "" {
+ if err := validate.SafeInputPath(dir); err != nil {
+ fmt.Fprintf(os.Stderr, "warning: invalid LARKSUITE_CLI_LOG_DIR: %v, using default\n", err)
+ // Fall through to default logic
+ } else {
- return dir
+ return dir
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/auth_log.go` around lines 24 - 26, The returned
LARKSUITE_CLI_LOG_DIR value is used later by vfs.MkdirAll and vfs.OpenFile and
must be validated; update the function in auth_log.go that reads
os.Getenv("LARKSUITE_CLI_LOG_DIR") to call validate.SafeInputPath (or the
project’s equivalent) on the value and only return it if validation succeeds,
otherwise fall back to the existing default path or an empty string and
log/handle the invalid input; ensure the validation happens before any use in
vfs.MkdirAll or vfs.OpenFile so the path cannot be used without passing
validate.SafeInputPath.
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | ||
| return dir |
There was a problem hiding this comment.
Critical: Append service name to avoid data conflicts between services.
When LARKSUITE_CLI_DATA_DIR is set, the function returns the raw environment variable value without appending the service parameter. This means different services (e.g., StorageDir("service1") and StorageDir("service2")) will share the same directory, which can lead to:
- Master key conflicts: Multiple services would share the same
master.keyfile, breaking encryption isolation - Credential overwrites: Encrypted credential files with identical account names would collide across services
- Security boundary violation: Service isolation is a security design principle
The default behavior (line 38) correctly appends the service name: filepath.Join(xdgData, service). The environment variable override must maintain this structure.
🔧 Proposed fix to append service name
func StorageDir(service string) string {
if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" {
- return dir
+ return filepath.Join(dir, service)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | |
| return dir | |
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | |
| return filepath.Join(dir, service) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/keychain_other.go` around lines 28 - 29, The function
currently returns the raw LARKSUITE_CLI_DATA_DIR env value
(os.Getenv("LARKSUITE_CLI_DATA_DIR")) without appending the service name,
causing data/credential collisions; change the branch in StorageDir to return
filepath.Join(dir, service) instead of dir so the environment override preserves
per-service isolation (mirror the default branch which uses
filepath.Join(xdgData, service)).
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | ||
| return dir | ||
| } |
There was a problem hiding this comment.
Validate the environment variable path before use.
The LARKSUITE_CLI_DATA_DIR environment variable is user-controlled input that is returned directly and later used in file I/O operations (lines 52, 71, 148, 174, 183, 199). Without validation, malicious or malformed paths could cause path traversal vulnerabilities or unexpected filesystem behavior.
As per coding guidelines: Validate paths using validate.SafeInputPath before any file I/O operations.
🛡️ Proposed fix to add path validation
+import (
+ "github.com/larksuite/cli/internal/validate"
+)
+
func StorageDir(service string) string {
if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" {
+ if err := validate.SafeInputPath(dir); err != nil {
+ fmt.Fprintf(os.Stderr, "warning: invalid LARKSUITE_CLI_DATA_DIR: %v, using default\n", err)
+ // Fall through to default logic
+ } else {
- return dir
+ return dir
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/keychain_other.go` around lines 28 - 30, The code returns
the LARKSUITE_CLI_DATA_DIR env value directly; validate this user-controlled
path with validate.SafeInputPath before returning/using it to prevent path
traversal or malformed paths. Modify the function that reads
os.Getenv("LARKSUITE_CLI_DATA_DIR") to call validate.SafeInputPath(dir) and
handle validation errors (fallback to default dir or return an error) so all
subsequent uses (the functions reading files in this package) receive a
validated path.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if macOS StorageDir implementation supports LARKSUITE_CLI_DATA_DIR
rg -n "LARKSUITE_CLI_DATA_DIR" internal/keychain/keychain_darwin.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check the macOS StorageDir implementation
cat -n internal/keychain/keychain_darwin.go | sed -n '40,60p'Repository: larksuite/cli
Length of output: 967
🏁 Script executed:
# List all platform-specific keychain implementations
fd -e go internal/keychain/ | sortRepository: larksuite/cli
Length of output: 434
🏁 Script executed:
# Check what keychain_other.go represents and its full context
head -n 40 internal/keychain/keychain_other.goRepository: larksuite/cli
Length of output: 976
🏁 Script executed:
# Find all keychain implementation files
fd . internal/keychain/ --type f -e go | sortRepository: larksuite/cli
Length of output: 298
🏁 Script executed:
# Search for any documentation or reasoning about platform-specific StorageDir
rg -n "StorageDir|LARKSUITE_CLI_DATA_DIR" internal/keychain/ -B2 -A2Repository: larksuite/cli
Length of output: 5429
🏁 Script executed:
# Check if there's documentation in config or other places about this env var
rg -n "LARKSUITE_CLI_DATA_DIR" --type go | head -20Repository: larksuite/cli
Length of output: 155
🏁 Script executed:
# Check Windows implementation for StorageDir
cat -n internal/keychain/keychain_windows.go | sed -n '1,80p'Repository: larksuite/cli
Length of output: 3114
🏁 Script executed:
# Check if LARKSUITE_CLI_DATA_DIR is documented anywhere
rg -n "LARKSUITE_CLI_DATA_DIR" -B3 -A3 --type-add='markdown:*.md'Repository: larksuite/cli
Length of output: 549
🏁 Script executed:
# Check tests for keychain_other to see if env var override is tested
cat internal/keychain/keychain_other.go | grep -n "func Test"Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check if there's a test file for keychain_other
fd . internal/keychain/ --name '*other*test*'Repository: larksuite/cli
Length of output: 289
🏁 Script executed:
# Look for any test that uses LARKSUITE_CLI_DATA_DIR
rg -n "LARKSUITE_CLI_DATA_DIR" --type goRepository: larksuite/cli
Length of output: 155
🏁 Script executed:
# Check if there are tests that might use t.Setenv for this
rg -n "Setenv.*LARKSUITE_CLI" --type goRepository: larksuite/cli
Length of output: 3772
Add environment variable override to macOS StorageDir for consistency, or document as Linux-only.
The Linux implementation in keychain_other.go:28-30 checks LARKSUITE_CLI_DATA_DIR to allow runtime override of the storage directory, but the macOS implementation in keychain_darwin.go:46-53 does not. This creates inconsistent behavior across platforms where users on Linux can override the storage location but users on macOS cannot.
Consider either:
- Adding the same
LARKSUITE_CLI_DATA_DIRcheck to the macOSStorageDirfunction for feature parity - Explicitly documenting this as a Linux-only feature with reasoning (e.g., if intentional by design)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/keychain_other.go` around lines 28 - 30, The macOS
StorageDir implementation (StorageDir in keychain_darwin.go) is missing the
LARKSUITE_CLI_DATA_DIR environment override present in the other implementation
(keychain_other.go); add the same env check at the start of StorageDir in
keychain_darwin.go to return that dir if set (or, alternatively, add a comment
in StorageDir documenting that the override is intentionally Linux-only). Update
the StorageDir function in keychain_darwin.go to mirror the logic: read
os.Getenv("LARKSUITE_CLI_DATA_DIR") and return it when non-empty before falling
back to the existing macOS directory resolution.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f567cc9ad30e7dd1c799586733193179297764dc🧩 Skill updatenpx skills add larksuite/cli#feat/linux_env_data_dir -y -g |
Greptile SummaryThis PR adds two environment variable overrides —
Confidence Score: 4/5One P1 logic bug — Score is 4 because the
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant StorageDir
participant authLogDir
participant FS as File System
User->>StorageDir: StorageDir("lark-cli")
alt LARKSUITE_CLI_DATA_DIR set
StorageDir-->>User: $LARKSUITE_CLI_DATA_DIR (service dropped ⚠️)
else default
StorageDir-->>User: ~/.local/share/lark-cli
end
User->>authLogDir: authLogDir()
alt LARKSUITE_CLI_LOG_DIR set
authLogDir-->>User: $LARKSUITE_CLI_LOG_DIR
else LARKSUITE_CLI_CONFIG_DIR set
authLogDir-->>User: $LARKSUITE_CLI_CONFIG_DIR/logs
else default
authLogDir-->>User: ~/.lark-cli/logs
end
StorageDir->>FS: read/write master.key, *.enc
authLogDir->>FS: write auth-YYYY-MM-DD.log
Reviews (1): Last reviewed commit: "feat(keychain): support custom log direc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func StorageDir(service string) string { | ||
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | ||
| return dir | ||
| } |
There was a problem hiding this comment.
service parameter silently dropped when env var is set
When LARKSUITE_CLI_DATA_DIR is set, StorageDir returns the directory as-is, discarding the service argument. The default code path returns filepath.Join(xdgData, service), so a user who sets LARKSUITE_CLI_DATA_DIR=/home/user/.local/share expecting the same layout would instead find master.key and all .enc credential files directly in /home/user/.local/share rather than /home/user/.local/share/lark-cli/.
| func StorageDir(service string) string { | |
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | |
| return dir | |
| } | |
| if dir := os.Getenv("LARKSUITE_CLI_DATA_DIR"); dir != "" { | |
| return filepath.Join(dir, service) | |
| } |
1 participant
Summary
Adds environment variable overrides for keychain-related local storage paths. This lets users customize the Linux keychain data directory and the auth log directory without changing the default behavior.
Changes
LARKSUITE_CLI_DATA_DIRsupport ininternal/keychain/keychain_other.goso Linux keychain storage can use a custom directoryLARKSUITE_CLI_LOG_DIRsupport ininternal/keychain/auth_log.goso auth logs can be written to a custom directoryTest Plan
lark xxxcommands locallyExecuted:
go test ./internal/keychainGOOS=linux GOARCH=amd64 go test -c -o /tmp/keychain_linux.test ./internal/keychainRelated Issues
Summary by CodeRabbit
Release Notes
LARKSUITE_CLI_LOG_DIRenvironment variable to customize log directory locationLARKSUITE_CLI_DATA_DIRenvironment variable to customize data storage directory location