Open
Conversation
- Add tests for ACL file detection, path manipulation, and symlink security - Add tests for access control levels and bit flag operations - Add tests for rule pattern matching and access validation - Add tests for ruleset loading and validation - Add tests for rate limiting and resource constraints - Add tests for ACL cache operations including concurrency - Add tests for server ACL access levels
4 tasks
There was a problem hiding this comment.
Pull Request Overview
This PR enhances security by rejecting symlinked ACL files, fixes rule management in the tree and node structures, and increases test coverage across ACL and permissions components.
- Prevents symlink-based ACL loading in both
ExistsandLoadFromFile - Improves cache tests to cover basic ops, prefix deletion, version behavior, concurrency, and memory usage
- Adds comprehensive tests for rule set loading, saving, and default injection
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/acl/level_test.go | Adds unit tests for AccessLevel.String() and bitwise operations |
| internal/server/acl/cache_test.go | Expands tests for ACLCache (Get/Set/Delete, DeletePrefix, concurrency, memory) |
| internal/aclspec/ruleset.go | Enforces symlink rejection in LoadFromFile |
| internal/aclspec/aclspec.go | Switches to Lstat and rejects symlinks in Exists |
| internal/aclspec/ruleset_test.go | New tests for YAML load/save, default rule injection, and error cases |
| internal/aclspec/aclspec_test.go | New tests for ACL file detection, path helpers, and symlink security |
Comments suppressed due to low confidence (1)
internal/aclspec/ruleset.go:44
- You use
fmt.Errorfon the next line, but I don’t seefmtimported; addimport "fmt"so the code compiles.
if stat.Mode()&os.ModeSymlink != 0 {
| aclPath := AsACLPath(path) | ||
| stat, err := os.Stat(aclPath) | ||
| stat, err := os.Lstat(aclPath) // Use Lstat to not follow symlinks | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
Errors other than IsNotExist are currently ignored, potentially masking permission or I/O errors. Consider checking if err != nil { return false } immediately after Lstat before continuing.
Suggested change
| if os.IsNotExist(err) { | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return false | |
| } | |
| // Handle other errors (e.g., permission or I/O errors) |
| // Test concurrent Get operations | ||
| for i := 0; i < numGoroutines; i++ { | ||
| wg.Add(1) | ||
| go func(id int) { |
There was a problem hiding this comment.
[nitpick] Calling assert (or using t) directly inside goroutines can lead to data races on testing.T. Collect errors or use channels and perform assertions after all goroutines complete.
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.
Core Changes:
Terminal node enforcement: Prevent child rulesets from being added under terminal nodes for proper security boundaries
Symlink security: Reject symlinked ACL files in Exists() and LoadFromFile() to prevent security vulnerabilities
Rule removal fix: RemoveRuleSet now only clears rules from target node while preserving child nodes, preventing accidental data loss
Rule clearing: SetRules properly clears existing rules when passed nil/empty ruleset
Technical Details:
tree.go: Add ErrTerminalNodeExists, validate terminal nodes in AddRuleSet, rewrite RemoveRuleSet to preserve children
node.go: Fix SetRules to clear rules when nil/empty input provided
aclspec.go: Replace os.Stat with os.Lstat and add symlink rejection in Exists()
ruleset.go: Add symlink validation in LoadFromFile() with descriptive error messages
Tests added to internal/server/acl and internal/aclspec packages to increase coverage and validate new security behaviors.