Fix group name validation

[eleftherias]

• Move lowercase validation to primary validator
• Remove validation from manager since it's already used in CLI and API

Fix #2355

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.42%. Comparing base (f5cef87) to head (9d3ca29).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2402   +/-   ##
=======================================
  Coverage   54.41%   54.42%           
=======================================
  Files         262      262           
  Lines       25185    25177    -8     
=======================================
- Hits        13704    13702    -2     
+ Misses      10176    10174    -2     
+ Partials     1305     1301    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

@claude please review this with emphasis on security and input validation.

[claude]

Claude finished @JAORMX's task —— View job

---

Security and Input Validation Review

Tasks

• Read changed files and understand modifications
• Analyze validation logic changes
• Review security implications
• Check for validation bypasses or edge cases
• Verify test coverage
• Provide comprehensive review feedback

---

Review Summary

This PR successfully consolidates group name validation into a single location (pkg/validation/validation.go) and adds the lowercase enforcement to the primary validator. The changes improve code maintainability and reduce duplication. Overall, this is a positive security change with solid test coverage.

Security Analysis

✅ Strengths

1. Centralized Validation: Moving validation to a single location (pkg/validation) reduces the risk of inconsistent validation logic across different code paths.

2. Comprehensive Character Restrictions: The validation enforces:

   • Lowercase alphanumeric only (a-z, 0-9)
   • Allows underscores, dashes, and spaces
   • Rejects uppercase letters explicitly
   • Prevents null bytes (\x00)
   • Prevents leading/trailing whitespace
   • Prevents consecutive spaces
   • Prevents empty/whitespace-only names

3. Defense in Depth: Validation is applied at multiple layers:

   • CLI layer: cmd/thv/app/common.go:168 and cmd/thv/app/group.go:76
   • API layer: pkg/api/v1/groups.go:102, 145, 183
   • Workload service: pkg/api/v1/workload_service.go:284
   • Manager layer: pkg/groups/manager.go:39

4. Test Coverage: The test suite (pkg/validation/validation_test.go) includes good edge cases:

   • Valid inputs with various allowed characters
   • Empty/whitespace-only inputs
   • Invalid special characters and unicode
   • Null byte injection attempts
   • Whitespace boundary conditions
   • Uppercase rejection

🔍 Security Considerations

1. Validation Removal from Manager Methods ⚠️

   The PR removes validation from Get(), Delete(), and Exists() methods in pkg/groups/manager.go:

   // Before: validation on Get/Delete/Exists
   // After: NO validation on Get/Delete/Exists

   Impact: This is acceptable IF (and only if) all callers perform validation before calling these methods. Based on my analysis:

   • Get() callers: API endpoints validate (✅), internal manager calls during RegisterClients/UnregisterClients do not validate (⚠️)
   • Delete() callers: API endpoint validates (✅)
   • Exists() callers: Mixed - some validate, some don't

   Recommendation: The removal is generally safe because invalid names would fail to match existing groups (fail-safe), but it creates a subtle security assumption that all entry points validate. Consider one of:

   a. Document this assumption clearly in the Manager interface documentation
   b. Keep defensive validation in Get() at minimum, since it's used internally by RegisterClients/UnregisterClients
   c. Add validation to internal callers like RegisterClients/UnregisterClients before calling Get()

2. Path Traversal Risk ⚠️

   Group names are used as filesystem paths (via state.Store). The current regex (^[a-z0-9_\-\s]+$) prevents:

   • Dot segments (. and ..)
   • Slashes (/)
   • Backslashes (\)

   This is secure - no path traversal is possible with the allowed character set.

3. Null Byte Check Ordering ✅

   The null byte check happens BEFORE the regex check, which is correct:

   if strings.Contains(name, "\x00") {
       return fmt.Errorf("group name cannot contain null bytes")
   }

   This prevents null byte injection attacks in contexts where null bytes could truncate strings.

4. Regex Security ✅

   The regex ^[a-z0-9_\-\s]+$ is:

   • Anchored (prevents partial matches)
   • Simple character class (not vulnerable to ReDoS)
   • Compiled once at package init (performance)

5. Lowercase Normalization vs Rejection ✅

   The code rejects uppercase rather than normalizing:

   if name != strings.ToLower(name) {
       return fmt.Errorf("group name must be lowercase")
   }

   This is the correct security approach - forcing users to provide correct input rather than silently modifying it prevents confusion and potential security issues where case matters.

Code Quality

✅ Improvements

1. Better Error Messages: Error messages now consistently mention "lowercase" requirement
2. Reduced Duplication: Removed 20 lines of duplicate validation code
3. Test Updates: Tests correctly updated to reflect new error messages

📝 Minor Issues

1. Error Wrapping Inconsistency:

   • In manager.Create(): Returns thverrors.NewInvalidArgumentError(err.Error(), err) ✅
   • In API handlers: Returns plain errors ⚠️

   Consider wrapping with appropriate error types consistently across all layers.

2. Validation Performance: The validation performs multiple string operations. For high-frequency operations, consider short-circuiting:

   // Check lowercase first (fast)
   if name != strings.ToLower(name) {
       return fmt.Errorf("group name must be lowercase")
   }
   // Then regex (slower)

   Current order is fine, but lowercase check could be moved earlier for performance.

Test Coverage Assessment

The test coverage is excellent:

• ✅ Valid cases with various character combinations
• ✅ Empty and whitespace-only inputs
• ✅ Invalid characters (special chars, unicode, emojis)
• ✅ Null byte injection
• ✅ Whitespace boundary conditions
• ✅ Uppercase rejection (new in this PR)
• ✅ Edge cases (single char, long names)

Missing test case: Consider adding test for names with mix of allowed characters: "test-group_123 name" (already covered in "max_typical")

Recommendations

High Priority

None - the changes are secure.

Medium Priority

1. Document validation assumptions: Add godoc comments to Manager interface methods explaining that callers must validate group names before calling Get(), Delete(), Exists().

Low Priority

1. Consider adding defensive validation back to Get() since it's called by internal methods
2. Ensure consistent error wrapping across all layers (CLI, API, Manager)

Conclusion

✅ APPROVED from a security perspective

This PR improves security by centralizing validation logic and maintaining comprehensive input validation. The removal of validation from some manager methods is acceptable given the defense-in-depth approach at higher layers, though documenting this design decision would be beneficial.

The changes properly address the security concerns around input validation:

• No injection vulnerabilities
• No path traversal risks
• Proper null byte handling
• Appropriate uppercase rejection (not normalization)
• Good test coverage

---