fix backend test workflow #491
Conversation
This comment has been minimized.
This comment has been minimized.
nsingla
force-pushed
the
fix_backend_tests_workflow
branch
from
864d49c to
2611d07
Compare
Claude Code ReviewSummaryThis PR makes two changes:
The workflow fix is correct and necessary. However, the test changes reveal a critical security issue in the path handling logic that needs immediate attention. Issues by Severity🚫 Blocker Issues1. Security vulnerability: Path normalization creates unsafe behavior (content.go:348, content.go:386, content.go:415) The current implementation has a dangerous edge case: // In ContentWrite, ContentRead, ContentList:
path := filepath.Clean("/" + strings.TrimSpace(req.Path))
abs := filepath.Join(StateBaseDir, path)Problem: Inputs like
Security Impact: An attacker could discover all files in the workspace by accessing Test Evidence: The updated test expectations confirm this: {"../", "parent directory", http.StatusInternalServerError, http.StatusInternalServerError, http.StatusOK}
// ^^^^^^^^^^^^
// Lists base dir!Required Fix: Reject these inputs explicitly BEFORE normalization: // Add this check BEFORE filepath.Clean in all three handlers
inputPath := strings.TrimSpace(req.Path) // or c.Query("path")
if strings.Contains(inputPath, "..") {
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path"})
return
}
path := filepath.Clean("/" + inputPath)Why the test changes are problematic: The PR updates tests to accept this behavior as "correct", which normalizes a security issue instead of fixing it. 🔴 Critical Issues2. Test comment is misleading (content_test.go:991-998) The comment states:
Problem: This reads as documenting intended behavior rather than documenting a bug to be fixed. Fix: Add a TODO or FIXME: // FIXME: Security issue - these paths normalize to base directory and allow
// listing workspace contents. Should reject paths containing ".." before normalization.
// See: https://github.com/ambient-code/platform/issues/XXX🟡 Major Issues3. Inconsistent error handling between operations (content_test.go:996-997) The test expects different behaviors for the same invalid input:
Problem: This is inconsistent API design. The same invalid input should produce the same error code across all operations. Expected behavior: All three should return 🔵 Minor Issues4. GitHub Actions path pattern fix is correct (backend-unit-tests.yml:28) Change from Explanation: GitHub Actions path filters don't use leading Reference: GitHub Actions workflow syntax 5. Test isolation is good (content_test.go:1014-1018) Good practice: Tests recreate temp directories between iterations for isolation. This prevents test pollution. Positive Highlights✅ Workflow trigger fix: Correctly removes invalid RecommendationsPriority 1 (Block Merge)
Priority 2 (Before Next Release)
Priority 3 (Follow-up)
Suggested Fixdiff --git a/components/backend/handlers/content.go b/components/backend/handlers/content.go
index xxx..yyy 100644
--- a/components/backend/handlers/content.go
+++ b/components/backend/handlers/content.go
@@ -345,6 +345,11 @@ func ContentWrite(c *gin.Context) {
}
log.Printf("ContentWrite: path=%q contentLen=%d encoding=%q StateBaseDir=%q", req.Path, len(req.Content), req.Encoding, StateBaseDir)
+ // Reject paths with .. BEFORE normalization to prevent base directory access
+ if strings.Contains(req.Path, "..") {
+ c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path"})
+ return
+ }
path := filepath.Clean("/" + strings.TrimSpace(req.Path))
abs := filepath.Join(StateBaseDir, path)
// Verify abs is within StateBaseDir to prevent path traversalApply the same fix to Compliance Check Against Project StandardsSecurity Standards (.claude/context/security-standards.md):
Backend Development (.claude/context/backend-development.md):
Error Handling Patterns (.claude/patterns/error-handling.md):
Final Recommendation: 🚫 Request changes - Security issue must be addressed before merge. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
nsingla
force-pushed
the
fix_backend_tests_workflow
branch
from
2611d07 to
d1dcc7c
Compare
Claude Code ReviewSummaryThis PR fixes two issues:
The changes correctly address test failures, but there are some important security and architectural concerns that should be addressed. Issues by Severity🟡 Major Issues1. Security: Path Traversal Handling Inconsistency (content.go:351-354, content.go:392-395, content.go:422-425) The current implementation has an edge case where certain path traversal attempts (like
Why this matters:
Example behavior: Recommendation: // After IsPathWithinBase check, add:
if cleanAbs == StateBaseDir {
log.Printf("ContentWrite: attempt to operate on base directory rejected: path=%q", path)
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path: cannot operate on base directory"})
return
}This should be added to 2. Test Documentation Could Be Clearer (content_test.go:991-997) The NOTE comment explains the behavior but doesn't explain why this is acceptable or if it's intentional: // NOTE: The current handler implementation normalizes these inputs to the
// base directory itself. That passes the "within base" check and then the
// underlying file operations behave as:
// - write/read: fails (directory write/read) -> 500
// - list: lists the base directory -> 200 (empty items in these tests)Recommendation:
🔵 Minor Issues3. Missing Error Context in Logs (content.go:352, 393, 423) When path traversal is detected, logs don't include the resolved absolute path, which would help debugging: // Current
log.Printf("ContentWrite: path traversal attempt rejected: path=%q abs=%q", path, abs)
// Better (already matches pattern)
log.Printf("ContentWrite: path traversal attempt rejected: path=%q abs=%q base=%q", path, abs, StateBaseDir)Actually, looking at the code more closely, the current logging is adequate - this is already done correctly. 4. Workflow Path Pattern (.github/workflows/backend-unit-tests.yml:28) The fix from ✅ This change is correct and follows GitHub Actions path filter conventions. Positive Highlights
RecommendationsPriority 1: Security Hardening (Before Merge)Add explicit base directory check to // In ContentWrite, after IsPathWithinBase check (line 351)
if filepath.Clean(abs) == filepath.Clean(StateBaseDir) {
log.Printf("ContentWrite: attempt to write to base directory rejected: path=%q", path)
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path: cannot write to base directory"})
return
}
// In ContentRead, after IsPathWithinBase check (line 392)
if filepath.Clean(abs) == filepath.Clean(StateBaseDir) {
log.Printf("ContentRead: attempt to read base directory rejected: path=%q", path)
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path: cannot read directory as file"})
return
}Then update tests to expect Priority 2: Documentation (Before Merge)Update the NOTE comment in tests to clarify intent: // NOTE: Paths like "../" and "test/../../.." normalize to the base directory.
// The IsPathWithinBase check passes (base is technically within base), but file
// operations fail because you can't read/write a directory as a file.
// TODO: Consider adding explicit validation to return 400 instead of 500 for better UXPriority 3: Future Improvement (Nice to Have)Consider extracting path validation into a shared helper: // pathutil/pathutil.go
func ValidateFilePath(path, baseDir string) (string, error) {
abs := filepath.Join(baseDir, filepath.Clean("/" + path))
if !IsPathWithinBase(abs, baseDir) {
return "", fmt.Errorf("path traversal detected")
}
if filepath.Clean(abs) == filepath.Clean(baseDir) {
return "", fmt.Errorf("cannot operate on base directory")
}
return abs, nil
}This would centralize validation logic and ensure consistency across all handlers. ConclusionThe PR fixes the immediate issues (workflow path and test failures), but exposes a security-adjacent edge case where path traversal attempts return 500 instead of 400. While no data leak occurs (the traversal is blocked), the error response doesn't follow best practices. Recommendation: Fix the base directory check issue before merging to align with error handling patterns and improve security posture. Review completed with reference to:
🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code ReviewSummaryThis PR fixes a GitHub Actions workflow path issue and updates backend test expectations to match current implementation behavior. The changes are minimal but address two distinct issues:
Issues by Severity🚫 Blocker IssuesNone - No blockers identified. 🔴 Critical Issues1. Test Changes Mask Potential Security Regression
Analysis of Current Implementation ( When input is
Missing Validation: Need to explicitly reject attempts to operate on the base directory itself 🟡 Major Issues2. Disk Cleanup Action from External Repository
3. Inconsistent Syntax in Workflow Conditionals
🔵 Minor Issues4. Missing PR Description Details
5. Test Comment Accuracy
6. Workflow Path Pattern Fix
Positive Highlights✅ Correct Workflow Path Fix: The change from ✅ Test Documentation: Added helpful comments explaining the current behavior (lines 991-998) ✅ Comprehensive Test Coverage: The test suite covers multiple path traversal scenarios including edge cases ✅ Disk Space Management: Adding disk cleanup step is proactive for preventing CI failures due to space constraints RecommendationsPriority 1 (Must Fix Before Merge):
Priority 2 (Should Fix):
Priority 3 (Nice to Have):
Security Assessment
Recommendation: This should be treated as a security fix, not just a test adjustment. The handler should explicitly reject attempts to operate on the base directory with appropriate 400 status codes. Testing Validation
Final AssessmentStatus: While the workflow fix is good, the test changes suggest a deeper issue that should be investigated and properly resolved rather than just updating test expectations. The security implications of path handling changes require careful review. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code ReviewSummaryThis PR fixes a workflow path configuration issue and updates test expectations to match current behavior after a regression in the content.go path handling logic. The changes include workflow path fixes, test expectation updates, and image name corrections for minikube deployments. Issues by Severity🚫 Blocker Issues1. Security Regression in Path Traversal Handling (content_test.go) Location: Issue: The test expectation changes indicate a security regression in path traversal validation. The current implementation now returns HTTP 500 (Internal Server Error) or 200 (OK for list) instead of HTTP 400 (Bad Request) for path traversal attempts like According to the test comment: // NOTE: The current handler implementation normalizes these inputs to the
// base directory itself. That passes the "within base" check and then the
// underlying file operations behave as:
// - write/read: fails (directory write/read) -> 500
// - list: lists the base directory -> 200 (empty items in these tests)Why this is a blocker:
Root cause: The Required fix:
Pattern to follow (from error-handling.md): // Validate early, return 400 Bad Request
if strings.Contains(path, "..") {
c.JSON(http.StatusBadRequest, gin.H{"error": "Path traversal not allowed"})
return
}🔴 Critical Issues2. Missing Test Updates for Root Cause Location: Issue: The PR updates test expectations to match the regression but doesn't fix the underlying bug in Why critical:
Recommendation:
🟡 Major Issues3. Workflow Path Configuration Inconsistency Location: Issue: Changed from Why this matters:
Verification needed: # Check all workflows for similar patterns
grep -r "'./" .github/workflows/4. Disk Cleanup in CI Workflows Location: Issue: Added disk cleanup step using external action Concerns:
Recommendations:
Pattern to follow: - name: Cleanup Diskspace
uses: kubeflow/pipelines/.github/actions/github-disk-cleanup@v1.2.3 # Pin to version
if: (\!cancelled())5. Image Name Changes Without Documentation Location: Issue: Changed image names from hyphen-separated ( Concerns:
Verification needed: # Check if build scripts produce underscore names
grep -r "vteam_backend\|vteam-backend" Makefile components/*/DockerfileRecommendation:
🔵 Minor Issues6. Test Comment Clarity Location: Issue: The added comment explains the current behavior but doesn't indicate it's incorrect. Suggestion: // FIXME: The current handler implementation has a security bug where it normalizes
// path traversal attempts to the base directory instead of rejecting them.
// These test cases are temporarily updated to match the buggy behavior but should
// be reverted once the bug is fixed. See issue #XXX7. Inconsistent Debug Logging Location: Issue: Added backend pod debug output but formatting could be improved. Minor improvement: echo "⚠️ Backend pod events:"
kubectl get events -n ambient-code \
--field-selector involvedObject.kind=Pod \
--sort-by='.lastTimestamp' | grep backend-api || echo "(No backend-api events)"Positive Highlights
RecommendationsPriority 1: Fix Security Regression (Before Merge)
Priority 2: Secure External Dependencies
Priority 3: Verify Image Naming
Priority 4: Code Quality
Pre-Commit Checklist (from CLAUDE.md)
Final Verdict⛔ BLOCK MERGE until security regression is fixed. The workflow path fix is correct and needed, but the test expectation changes accept a security regression in path traversal handling. This violates security standards and error handling patterns documented in CLAUDE.md. Next steps:
Generated with Claude Code Review following standards from CLAUDE.md, security-standards.md, and error-handling.md 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Signed-off-by: Nelesh Singla <117123879+nsingla@users.noreply.github.com> fixing image paths for minikube deployment Signed-off-by: Nelesh Singla <117123879+nsingla@users.noreply.github.com>
nsingla
force-pushed
the
fix_backend_tests_workflow
branch
from
d13fd73 to
b51c1cb
Compare
Claude Code ReviewSummaryThis PR fixes two issues:
The changes are mostly configuration fixes, but there's a critical security concern with the test expectation changes that masks a potential path traversal regression. Issues by Severity🚫 Blocker Issues1. Path Traversal Regression Masked by Test Changes (components/backend/handlers/content_test.go:991-1069) The test expectation changes reveal that the current implementation has a security regression in path traversal protection: // Before (Expected):
{"test/../../..", "relative parent dirs", http.StatusBadRequest, http.StatusBadRequest, http.StatusBadRequest},
{"../", "parent directory", http.StatusBadRequest, http.StatusBadRequest, http.StatusBadRequest},
// After (Current behavior):
{"test/../../..", "relative parent dirs", http.StatusInternalServerError, http.StatusInternalServerError, http.StatusOK},
{"../", "parent directory", http.StatusInternalServerError, http.StatusInternalServerError, http.StatusOK},Problem: The current implementation normalizes
Security Risk: This behavior change means:
Root Cause: The logic in path := filepath.Clean("/" + strings.TrimSpace(c.Query("path")))
abs := filepath.Join(StateBaseDir, path)
if \!pathutil.IsPathWithinBase(abs, StateBaseDir) {
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path"})
return
}When Recommendation:
🔴 Critical Issues2. Missing Test Coverage for the Actual Fix (components/backend/handlers/content_test.go) The workflow path pattern was broken, which means backend tests weren't running on PRs. However, this PR doesn't add any regression tests to prevent this from happening again. Recommendation:
🟡 Major Issues3. Inconsistent Error Handling Pattern (components/backend/handlers/content.go:351-355, 392-395, 422-425) The path traversal validation returns // Current:
if \!pathutil.IsPathWithinBase(abs, StateBaseDir) {
log.Printf("ContentWrite: path traversal attempt rejected: path=%q abs=%q", path, abs)
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid path"})
return
}
// Should be (per error-handling.md pattern):
if \!pathutil.IsPathWithinBase(abs, StateBaseDir) {
log.Printf("Security: path traversal attempt rejected: path=%q abs=%q baseDir=%q",
path, abs, StateBaseDir)
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid path: must be within workspace"})
return
}Issues:
4. Workflow Disk Cleanup May Not Be Sufficient (.github/workflows/e2e.yml:65-67, test-local-dev.yml:15-17) Added disk cleanup step, but:
5. Image Naming Inconsistency (components/manifests/minikube/*.yaml) The image names changed from
Recommendation:
🔵 Minor Issues6. Verbose Debugging Code in Workflow (.github/workflows/test-local-dev.yml:58-61) Added extensive debugging output for backend deployment failures: echo "⚠️ Backend pod events:"
kubectl get events -n ambient-code --field-selector involvedObject.kind=Pod,involvedObject.name\!='' --sort-by='.lastTimestamp' | grep backend-api || echo "(No backend-api pod events found)"
echo "⚠️ Backend pod details:"
kubectl get pods -n ambient-code -l app=backend-api -o yaml | grep -A 20 -B 5 "conditions:\|events:\|status:" || trueIssues:
Recommendation:
7. Missing PR Description Details The PR description mentions:
But doesn't explain:
Positive Highlights✅ Workflow Path Pattern Fix - Correctly removed leading RecommendationsImmediate (Before Merge):
Short-term (Follow-up PR):
Long-term:
Overall Assessment: This PR fixes important infrastructure issues but masks a security regression by changing test expectations instead of fixing the underlying path traversal bug. The path traversal issue is a blocker that must be resolved before merge. Recommended Action: Request changes to fix the path traversal regression before merging. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
2 participants
Fix the path to the backend files in the github workflow
Also, at some point there was a regression introduced in the content.go file which we did not catch in a PR due to the 1st issue, but for now I am just changing the test expectations in this PR so that the tests start to pass again