security: Reject path traversal in repo IDs, sanitize error responses

- handlers_upload.go: reject repo IDs containing ".." in both
  isValidRepoID and extractRepoIDFromPath (defense in depth,
  storage layer already sanitizes but validation should catch first)
- errors.go: WriteError no longer exposes raw error messages for
  5xx responses — returns "Internal server error" instead of
  internal paths, SQL errors, or stack traces. CkbError messages
  (user-facing) and 4xx errors are still included.

Addresses concerns from PRs #140 and #141 (mrwind-up-bird).

Author: SimplyLiz

internal/api/errors.go

@@ -16,22 +16,29 @@ type ErrorResponse struct {
 	Drilldowns     []errors.Drilldown `json:"drilldowns,omitempty"`
 }
 
-// WriteError writes an error response to the HTTP response writer
+// WriteError writes an error response to the HTTP response writer.
+// For internal server errors (5xx), the raw error message is sanitized
+// to avoid leaking file paths, SQL errors, or internal details.
 func WriteError(w http.ResponseWriter, err error, status int) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(status)
 
-	resp := ErrorResponse{
-		Error: err.Error(),
-	}
+	resp := ErrorResponse{}
 
-	// If it's a CkbError, include additional information
+	// If it's a CkbError, include structured information (safe to expose)
 	if ckbErr, ok := err.(*errors.CkbError); ok {
+		resp.Error = ckbErr.Message
 		resp.Code = string(ckbErr.Code)
 		resp.Details = ckbErr.Details
 		resp.SuggestedFixes = ckbErr.SuggestedFixes
 		resp.Drilldowns = ckbErr.Drilldowns
+	} else if status >= 500 {
+		// For internal errors, don't expose raw error details
+		resp.Error = "Internal server error"
+		resp.Code = "INTERNAL_ERROR"
 	} else {
+		// Client errors (4xx) — safe to include the message
+		resp.Error = err.Error()
 		resp.Code = "INTERNAL_ERROR"
 	}
 
@@ -101,8 +108,9 @@ func NotFound(w http.ResponseWriter, message string) {
 	}, http.StatusNotFound)
 }
 
-// InternalError writes a 500 Internal Server Error
-func InternalError(w http.ResponseWriter, message string, err error) {
+// InternalError writes a 500 Internal Server Error.
+// The message is returned to the client; the original err is not exposed.
+func InternalError(w http.ResponseWriter, message string, _ error) {
 	WriteError(w, &errors.CkbError{
 		Code:    errors.InternalError,
 		Message: message,

internal/api/errors_test.go

@@ -62,8 +62,9 @@ func TestWriteError(t *testing.T) {
 			t.Fatalf("failed to parse response: %v", err)
 		}
 
-		if resp.Error != "something went wrong" {
-			t.Errorf("resp.Error = %q, want 'something went wrong'", resp.Error)
+		// Internal errors (5xx) should NOT expose raw error messages
+		if resp.Error != "Internal server error" {
+			t.Errorf("resp.Error = %q, want 'Internal server error' (raw message should be sanitized)", resp.Error)
 		}
 		if resp.Code != "INTERNAL_ERROR" {
 			t.Errorf("resp.Code = %q, want INTERNAL_ERROR", resp.Code)

internal/api/handlers_upload.go

@@ -448,7 +448,10 @@ func isValidRepoID(id string) bool {
 			return false
 		}
 	}
-	// Don't allow consecutive slashes or starting/ending with special chars
+	// Don't allow path traversal components or leading/trailing slashes
+	if strings.Contains(id, "..") {
+		return false
+	}
 	if strings.Contains(id, "//") || strings.HasPrefix(id, "/") || strings.HasSuffix(id, "/") {
 		return false
 	}
@@ -462,7 +465,8 @@ func isValidRepoIDChar(c rune) bool {
 		c == '/' || c == '-' || c == '_' || c == '.'
 }
 
-// extractRepoIDFromPath extracts repo ID from URL path
+// extractRepoIDFromPath extracts repo ID from URL path.
+// Returns empty string if the extracted ID contains path traversal components.
 func extractRepoIDFromPath(path, prefix, suffix string) string {
 	if !strings.HasPrefix(path, prefix) {
 		return ""
@@ -471,5 +475,9 @@ func extractRepoIDFromPath(path, prefix, suffix string) string {
 	if suffix != "" && strings.HasSuffix(id, suffix) {
 		id = strings.TrimSuffix(id, suffix)
 	}
+	// Reject path traversal
+	if strings.Contains(id, "..") {
+		return ""
+	}
 	return id
 }