Conversation
Koopa0
commented
Feb 15, 2026
Koopa0
commented
- Add http.MaxBytesReader (1MB) and content length cap (32KB) to API chat handlers
- Add ErrPathDenied sentinel and deniedPrefixes to security.Path for blocking prompt directory access
- Add MaxKnowledgeContentSize (50KB) validation to knowledge_store tool
- Update NewPath signature across all callers to support denied prefixes
- Add unit tests for all new validation paths
- Add http.MaxBytesReader (1MB) and content length cap (32KB) to API
chat handlers
- Add ErrPathDenied sentinel and deniedPrefixes to security.Path for
blocking prompt directory access
- Add MaxKnowledgeContentSize (50KB) validation to knowledge_store
tool
- Update NewPath signature across all callers to support denied
prefixes
- Add unit tests for all new validation paths
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| func TestChatSend_BodyTooLarge(t *testing.T) { | ||
| // Create a valid JSON body larger than maxRequestBodySize (1 MB). | ||
| // The content field must be large enough so the whole JSON exceeds the limit. | ||
| largeContent := strings.Repeat("x", maxRequestBodySize) | ||
| body, _ := json.Marshal(map[string]string{ | ||
| "content": largeContent, | ||
| "sessionId": uuid.New().String(), | ||
| }) | ||
|
|
||
| w := httptest.NewRecorder() | ||
| r := httptest.NewRequest(http.MethodPost, "/api/v1/chat", bytes.NewReader(body)) | ||
|
|
||
| newTestChatHandler().send(w, r) | ||
|
|
||
| if w.Code != http.StatusRequestEntityTooLarge { | ||
| t.Fatalf("send(>1MB body) status = %d, want %d\nbody: %s", w.Code, http.StatusRequestEntityTooLarge, w.Body.String()) | ||
| } | ||
|
|
||
| errResp := decodeErrorEnvelope(t, w) | ||
| if errResp.Code != "body_too_large" { | ||
| t.Errorf("send(>1MB body) code = %q, want %q", errResp.Code, "body_too_large") | ||
| } | ||
| } |
There was a problem hiding this comment.
Security/Robustness:
The test TestChatSend_BodyTooLarge verifies the status code and error code, but does not assert that the response body is free from sensitive or unexpected information. To strengthen the test, add assertions to ensure the response body does not contain any partial or leaked data.
Recommended solution:
if strings.Contains(w.Body.String(), "hello") {
t.Error("send(>1MB body) response should not contain partial content")
}| handler = corsMiddleware(cfg.CORSOrigins)(handler) | ||
| handler = loggingMiddleware(logger)(handler) |
There was a problem hiding this comment.
Middleware Order: CORS and Logging
The CORS middleware is applied before the logging middleware. This means that CORS preflight requests (OPTIONS) may not be logged, potentially omitting important request traces for debugging or auditing. Consider placing the logging middleware as the outermost layer (before CORS) to ensure all requests, including preflight, are logged:
handler = loggingMiddleware(logger)(handler)
handler = corsMiddleware(cfg.CORSOrigins)(handler)This change will improve observability without affecting CORS behavior.
| if subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 { | ||
| return ErrCSRFInvalid |
There was a problem hiding this comment.
Potential ambiguity in signature length handling:
The comparison subtle.ConstantTimeCompare(actualSig, expectedSig) will return 0 if the lengths differ, but this is implicit. For clarity and maintainability, consider explicitly checking that len(actualSig) == len(expectedSig) before performing the comparison. This makes the logic clearer and avoids confusion for future maintainers.
Recommended solution:
if len(actualSig) != len(expectedSig) || subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
return ErrCSRFInvalid
}| if subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 { | ||
| return ErrCSRFInvalid |
There was a problem hiding this comment.
Potential ambiguity in signature length handling:
As in the user-bound CSRF check, subtle.ConstantTimeCompare(actualSig, expectedSig) will return 0 if the lengths differ, but this is implicit. For clarity and maintainability, explicitly check len(actualSig) == len(expectedSig) before comparison.
Recommended solution:
if len(actualSig) != len(expectedSig) || subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
return ErrCSRFInvalid
}| pathValidator, err := security.NewPath([]string{os.TempDir()}, nil) | ||
| if err != nil { | ||
| t.Fatalf("creating path validator: %v", err) | ||
| } |
There was a problem hiding this comment.
Error Handling and Test Robustness:
The error handling for security.NewPath is abrupt and does not provide actionable context if the path validator fails. If the failure is due to environmental issues (e.g., os.TempDir() is not writable or accessible), the test will simply fail without guidance. Consider enhancing the error message to include the value of os.TempDir() and possible remediation steps, such as checking directory permissions or environment configuration.
Recommended Solution:
if err != nil {
t.Fatalf("creating path validator for temp dir '%s': %v. Ensure the directory is writable and accessible.", os.TempDir(), err)
}| absAllowedDirs = append(absAllowedDirs, absDir) | ||
| } | ||
|
|
||
| // Convert denied prefixes to absolute paths | ||
| absDenied := make([]string, 0, len(deniedPrefixes)) | ||
| for _, prefix := range deniedPrefixes { | ||
| absPrefix, err := filepath.Abs(prefix) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to resolve denied prefix %s: %w", prefix, err) | ||
| } | ||
| absDenied = append(absDenied, absPrefix) | ||
| } | ||
|
|
||
| return &Path{ | ||
| allowedDirs: absAllowedDirs, | ||
| workDir: workDir, | ||
| allowedDirs: absAllowedDirs, | ||
| deniedPrefixes: absDenied, | ||
| workDir: workDir, | ||
| }, nil | ||
| } | ||
|
|
||
| // isPathDenied checks if a path matches any denied prefix. | ||
| // Uses case-insensitive comparison to handle case-insensitive filesystems (macOS HFS+). | ||
| func (v *Path) isPathDenied(absPath string) bool { | ||
| for _, denied := range v.deniedPrefixes { | ||
| deniedWithSep := filepath.Clean(denied) + string(filepath.Separator) | ||
| if strings.EqualFold(absPath, denied) || strings.HasPrefix(strings.ToLower(absPath+string(filepath.Separator)), strings.ToLower(deniedWithSep)) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // isPathInAllowedDirs checks if a path is within allowed directories | ||
| // Returns true if path is in working directory or any allowed directory | ||
| func (v *Path) isPathInAllowedDirs(absPath string) bool { |
There was a problem hiding this comment.
Potential directory containment flaw in isPathInAllowedDirs
The use of strings.HasPrefix to check if a path is within an allowed directory can be unsafe if directory names are prefixes of others (e.g., /tmp/foo and /tmp/foobar). This could allow unintended access:
if strings.HasPrefix(absPathWithSep, dirNorm) || absPath == dir {
return true
}Recommendation: Use filepath.Rel(dir, absPath) and check that the result does not start with .. or is not equal to ... This provides a more robust containment check and prevents directory traversal attacks.
| return "", fmt.Errorf("%w: access denied", ErrPathOutsideAllowed) | ||
| } | ||
|
|
||
| // 3b. Check if path matches a denied prefix (e.g. prompts/) | ||
| if v.isPathDenied(absPath) { | ||
| slog.Warn("path denied by prefix rule", | ||
| "path", absPath, | ||
| "security_event", "denied_prefix_access_attempt") | ||
| return "", fmt.Errorf("%w: access denied", ErrPathDenied) | ||
| } | ||
|
|
||
| // 4. Resolve symbolic links (prevent bypassing restrictions through symlinks) | ||
| realPath, err := filepath.EvalSymlinks(absPath) | ||
| if err != nil { |
There was a problem hiding this comment.
Denied prefix check missing after symlink resolution
After resolving symlinks with filepath.EvalSymlinks, the code checks if the resolved path is within allowed directories, but does not re-check denied prefixes. This could allow a symlink to bypass denied prefix restrictions:
if realPath != absPath {
if !v.isPathInAllowedDirs(realPath) {
// ...
return "", fmt.Errorf("%w: access denied", ErrSymlinkOutsideAllowed)
}
absPath = realPath
}Recommendation: After symlink resolution, also check isPathDenied(realPath) and return an error if the resolved path matches a denied prefix.
| // | ||
| // // Create tools with security validators | ||
| // pathVal, _ := security.NewPath([]string{"/allowed/path"}) | ||
| // pathVal, _ := security.NewPath([]string{"/allowed/path"}, nil) |
There was a problem hiding this comment.
Potential Security and Logic Issue: Error from security.NewPath is Ignored
The example uses a blank identifier for the error returned by security.NewPath:
pathVal, _ := security.NewPath([]string{"/allowed/path"}, nil)If security.NewPath fails (e.g., due to invalid input), this error will be silently ignored, potentially resulting in a nil or insecure path validator. This could compromise the security guarantees of the file tools.
Recommended Solution:
Always check and handle the error returned by security.NewPath:
pathVal, err := security.NewPath([]string{"/allowed/path"}, nil)
if err != nil {
return err
}This ensures that the path validator is correctly initialized and that any initialization errors are surfaced early.
|
|
||
| func TestFile_Constructor(t *testing.T) { | ||
| t.Run("valid inputs", func(t *testing.T) { | ||
| pathVal, err := security.NewPath([]string{"/tmp"}) | ||
| pathVal, err := security.NewPath([]string{"/tmp"}, nil) | ||
| if err != nil { | ||
| t.Fatalf("creating path validator: %v", err) | ||
| } |
There was a problem hiding this comment.
Insufficient validation of constructed object:
The test only checks that ft is non-nil and that no error occurred, but does not verify the properties or state of the constructed File object. This could allow subtle initialization bugs to go undetected.
Recommendation:
Add assertions to verify the expected properties and state of the File object after construction, for example:
if ft.PathValidator != pathVal {
t.Errorf("PathValidator not set correctly")
}| if err != nil { | ||
| t.Skip("could not create validator") |
There was a problem hiding this comment.
Skipping the test when the validator cannot be created (t.Skip("could not create validator")) may mask persistent setup or configuration issues, resulting in incomplete test coverage. Instead, consider failing the test with a clear error message to ensure that validator initialization problems are detected early:
t.Fatalf("could not create validator: %v", err)This approach provides immediate feedback and aids in debugging.
1 participant
