Better XDG directory variable fix for Snap#1701
Conversation
|
Warning Rate limit exceeded@esimkowitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces a new package The implementation adds functionality to retrieve and process environment variables from these configuration files, with robust parsing mechanisms that handle various file formats and edge cases. When an application is running in a Snap environment, the code now attempts to parse these files and update XDG-related environment variables accordingly. The changes include creating a new utility package for file parsing, adding test coverage for the parsing functionality, and integrating the new parsing logic into the existing shell execution process to ensure more accurate environment variable management. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (6)
pkg/util/pamparse/pamparse.go (3)
14-68: Consider wrapping errors for better context.The implementation is solid, but consider wrapping errors with additional context using
fmt.Errorf("failed to parse environment file %s: %w", path, err)to help with debugging.
70-85: Consider simplifying the regex pattern.The current regex pattern is complex and might be hard to maintain. Consider breaking it down into smaller patterns or using a more straightforward parsing approach. For example:
- Split the line by whitespace first
- Parse each component (DEFAULT=, OVERRIDE=) separately
87-110: Add validation for malformed input.The utility functions handle the happy path well, but consider adding validation for:
- Unmatched quotes
- Empty values after sanitization
- Escaped characters
pkg/util/pamparse/pamparse_test.go (2)
14-63: Add test cases for edge cases.Consider adding test cases for:
- Empty files
- Files with only comments
- Invalid UTF-8 encoding
- Very long lines
- Variables with special characters
65-102: Enhance test assertions.Consider adding assertions for:
- Error cases when file is not accessible
- Validation of the format of parsed values
- Cases where DEFAULT is missing but OVERRIDE is present
pkg/shellexec/shellexec.go (1)
456-474: Enhance error handling for XDG variable updates.The XDG variable handling is good, but consider:
- Validating the values of XDG variables before applying them
- Logging when variables are empty or invalid
- Adding fallback values for critical XDG variables
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/shellexec/shellexec.go(3 hunks)pkg/util/pamparse/pamparse.go(1 hunks)pkg/util/pamparse/pamparse_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
pkg/util/pamparse/pamparse.go (1)
4-12: LGTM! Clear package documentation and appropriate imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pkg/util/pamparse/pamparse.go (4)
4-13: Enhance package documentation with examples and specifications.Consider adding:
- Examples of valid file formats
- Description of parsing rules for each file type
- Links to official PAM environment documentation
16-33: Improve error handling and file operations in ParseEnvironmentFile.
- Use
os.Openinstead ofos.OpenFilefor read-only operations- Add scanner error checking
- Wrap errors with more context
func ParseEnvironmentFile(path string) (map[string]string, error) { rtn := make(map[string]string) - file, err := os.OpenFile(path, os.O_RDONLY, 0) + file, err := os.Open(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to open environment file %s: %w", path, err) } defer file.Close() scanner := bufio.NewScanner(file) for scanner.Scan() { line := scanner.Text() key, val := parseEnvironmentLine(line) if key == "" { continue } rtn[key] = val } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading environment file %s: %w", path, err) + } return rtn, nil }
90-116: Improve variable naming and use constants for array indices.Consider:
- Rename
mtomatchesfor better readability- Use constants for array indices in both parsing functions
+const ( + envLineKeyIndex = 1 + envLineValueIndex = 2 + confLineKeyIndex = 1 + confLineDefaultIndex = 2 + confLineOverrideIndex = 3 +) func parseEnvironmentLine(line string) (string, string) { - m := envFileLineRe.FindStringSubmatch(line) - if m == nil { + matches := envFileLineRe.FindStringSubmatch(line) + if matches == nil { return "", "" } - return m[1], sanitizeEnvVarValue(m[2]) + return matches[envLineKeyIndex], sanitizeEnvVarValue(matches[envLineValueIndex]) }
Line range hint
541-566: Improve error handling in tryGetPamEnvVars.The function continues silently when errors occur. Consider:
- Adding debug logging for successful parsing
- Returning an error if all parsing attempts fail
-func tryGetPamEnvVars() map[string]string { +func tryGetPamEnvVars() (map[string]string, error) { envVars, err := pamparse.ParseEnvironmentFile(etcEnvironmentPath) if err != nil { log.Printf("error parsing %s: %v", etcEnvironmentPath, err) } + if envVars != nil { + log.Printf("successfully parsed %d variables from %s", len(envVars), etcEnvironmentPath) + } // Similar changes for envVars2 and envVars3 + if envVars == nil && envVars2 == nil && envVars3 == nil { + return nil, fmt.Errorf("failed to parse any environment files") + } // ... rest of the function }pkg/shellexec/shellexec.go (4)
457-458: Format multi-line comment properly.The comment spans multiple lines and should use proper multi-line comment formatting.
- // For Snap installations, we need to correct the XDG environment variables as Snap overrides them to point to snap directories. We will get the correct values, if set, from the PAM environment. If the XDG variables are set in - // profile or in an RC file, it will be overridden when the shell initializes. + /* + * For Snap installations, we need to correct the XDG environment variables as Snap + * overrides them to point to snap directories. We will get the correct values, if + * set, from the PAM environment. If the XDG variables are set in profile or in an + * RC file, it will be overridden when the shell initializes. + */
460-460: Use slice-to-map for cleaner initialization.The map initialization can be more concise using a slice-to-map conversion.
- varsToReplace := map[string]string{"XDG_CONFIG_HOME": "", "XDG_DATA_HOME": "", "XDG_CACHE_HOME": "", "XDG_RUNTIME_DIR": "", "XDG_CONFIG_DIRS": "", "XDG_DATA_DIRS": ""} + xdgVars := []string{"XDG_CONFIG_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME", "XDG_RUNTIME_DIR", "XDG_CONFIG_DIRS", "XDG_DATA_DIRS"} + varsToReplace := make(map[string]string, len(xdgVars)) + for _, v := range xdgVars { + varsToReplace[v] = "" + }
541-546: Consider using a block comment for better visibility.The precedence order documentation is important and should be more visible. Consider using a block comment with proper formatting.
-// tryGetPamEnvVars tries to get the environment variables from /etc/environment, /etc/security/pam_env.conf, and ~/.pam_environment. -// It then returns a map of the environment variables, overriding duplicates with the following order of precedence: -// 1. /etc/environment -// 2. /etc/security/pam_env.conf -// 3. ~/.pam_environment +/* + * tryGetPamEnvVars tries to get the environment variables from /etc/environment, + * /etc/security/pam_env.conf, and ~/.pam_environment. + * + * It then returns a map of the environment variables, overriding duplicates with + * the following order of precedence: + * 1. /etc/environment + * 2. /etc/security/pam_env.conf + * 3. ~/.pam_environment + */
546-566: Consider aggregating errors for better debugging.The function logs individual errors but aggregating them could help in understanding if all files failed to parse.
func tryGetPamEnvVars() map[string]string { + var errs []error envVars, err := pamparse.ParseEnvironmentFile(etcEnvironmentPath) if err != nil { log.Printf("error parsing %s: %v", etcEnvironmentPath, err) + errs = append(errs, fmt.Errorf("parsing %s: %w", etcEnvironmentPath, err)) } envVars2, err := pamparse.ParseEnvironmentConfFile(etcSecurityPath) if err != nil { log.Printf("error parsing %s: %v", etcSecurityPath, err) + errs = append(errs, fmt.Errorf("parsing %s: %w", etcSecurityPath, err)) } envVars3, err := pamparse.ParseEnvironmentConfFile(wavebase.ExpandHomeDirSafe(userEnvironmentPath)) if err != nil { log.Printf("error parsing %s: %v", userEnvironmentPath, err) + errs = append(errs, fmt.Errorf("parsing %s: %w", userEnvironmentPath, err)) } + if len(errs) == 3 { + log.Printf("all PAM environment files failed to parse: %v", errs) + } for k, v := range envVars2 { envVars[k] = v }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/shellexec/shellexec.go(3 hunks)pkg/util/pamparse/pamparse.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-app (windows, windows-latest)
- GitHub Check: build-app (linux, ubuntu-24.04-arm64-16core)
- GitHub Check: build-app (linux, ubuntu-latest)
- GitHub Check: build-app (darwin, macos-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
pkg/util/pamparse/pamparse.go (3)
Line range hint
457-474: Document the XDG variable precedence and handling.The current implementation merges variables from multiple sources, but the precedence order should be clearly documented. Consider adding comments explaining:
- Which XDG variables take precedence when defined in multiple sources
- How conflicts between Snap and PAM environment variables are resolved
Line range hint
537-539: Fix tilde expansion in userEnvironmentPath.The
userEnvironmentPathuses tilde notation which won't be automatically expanded.
36-62: Add validation for home and shell values in ParseEnvironmentConfFile.The function should validate that both home and shell values are non-empty before using them for replacements.
func ParseEnvironmentConfFile(path string) (map[string]string, error) { rtn := make(map[string]string) - file, err := os.OpenFile(path, os.O_RDONLY, 0) + file, err := os.Open(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to open environment conf file %s: %w", path, err) } defer file.Close() home, shell, err := parsePasswd() if err != nil { return nil, err } + if home == "" || shell == "" { + return nil, fmt.Errorf("home directory or shell not found in passwd file") + } scanner := bufio.NewScanner(file) for scanner.Scan() { line := scanner.Text() key, val := parseEnvironmentConfLine(line) if key == "" { key, val = parseEnvironmentLine(line) if key == "" { continue } } rtn[key] = replaceHomeAndShell(val, home, shell) } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading environment conf file %s: %w", path, err) + } return rtn, nil }✅ Verification successful
The suggested validation for home and shell values is necessary and correct.
The validation is important because:
parsePasswd()can return empty values in several scenarios (user not found, malformed passwd entry)- Empty values in
replaceHomeAndShell()could lead to incorrect environment variable expansions- Current implementation lacks proper error handling for these cases
- No test coverage exists for these edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find parsePasswd implementation ast-grep --pattern 'func parsePasswd() (string, string, error) { $$$ }' # Find replaceHomeAndShell implementation ast-grep --pattern 'func replaceHomeAndShell(val string, home string, shell string) string { $$$ }' # Find test files fd -e go -E vendor testLength of output: 1815
Script:
#!/bin/bash # Check test file content cat pkg/util/pamparse/pamparse_test.goLength of output: 2489
pkg/shellexec/shellexec.go (1)
25-25: LGTM!The import for the
pamparsepackage is correctly placed and necessary for the new PAM environment parsing functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This fix will overwrite the XDG variables when creating a shell session and only if the installation is Snap (as determined by the presence of the `$SNAP` environment variable). It also will parse the user's PAM files, if present, to ensure that if the XDG variables were set there, they're restored. Otherwise, if the variables are set in an RC or profile file, we'll already pick it up when the shell starts. closes wavetermdev#1696 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
This fix will overwrite the XDG variables when creating a shell session and only if the installation is Snap (as determined by the presence of the
$SNAPenvironment variable). It also will parse the user's PAM files, if present, to ensure that if the XDG variables were set there, they're restored. Otherwise, if the variables are set in an RC or profile file, we'll already pick it up when the shell starts.closes #1696