Changeset 0.28.0.10 (#182)

server/handlers/surfaces.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/gin-gonic/gin"
@@ -15,6 +16,10 @@ import (
 	"git.gobha.me/xcaliber/chat-switchboard/store"
 )
 
+// validSurfaceID matches lowercase alphanumeric slugs with optional hyphens.
+// No leading/trailing hyphens, 2-64 chars.
+var validSurfaceID = regexp.MustCompile(`^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`)
+
 // SurfaceHandler manages surface lifecycle (admin-only).
 type SurfaceHandler struct {
 	stores      store.Stores
@@ -197,6 +202,13 @@ func (h *SurfaceHandler) InstallSurface(c *gin.Context) {
 		return
 	}
 
+	// Validate surface ID is a safe slug — prevents path traversal in
+	// filesystem operations (filepath.Join(surfacesDir, surfaceID)).
+	if !validSurfaceID.MatchString(surfaceID) {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "surface id must be a lowercase slug (a-z, 0-9, hyphens, 2-64 chars)"})
+		return
+	}
+
 	// Check for conflicts with core surfaces
 	existing, _ := h.stores.Surfaces.Get(c.Request.Context(), surfaceID)
 	if existing != nil && existing.Source == "core" {
@@ -213,21 +225,15 @@ func (h *SurfaceHandler) InstallSurface(c *gin.Context) {
 			if f.FileInfo().IsDir() {
 				continue
 			}
-			// Only extract js/, css/, assets/ directories
-			name := f.Name
-			// Strip leading directory if archive has one (e.g., "my-surface/js/foo.js" → "js/foo.js")
-			if idx := strings.Index(name, "/"); idx > 0 {
-				rest := name[idx+1:]
-				if strings.HasPrefix(rest, "js/") || strings.HasPrefix(rest, "css/") || strings.HasPrefix(rest, "assets/") {
-					name = rest
-				} else if strings.HasPrefix(name, "js/") || strings.HasPrefix(name, "css/") || strings.HasPrefix(name, "assets/") {
-					// Already relative
-				} else {
-					continue // Skip non-static files (templates, manifest)
-				}
+
+			// Resolve the relative path, stripping an optional single
+			// top-level directory prefix from the archive.
+			relPath := extractableRelPath(f.Name)
+			if relPath == "" {
+				continue // Not an extractable static file
 			}
 
-			destPath := filepath.Join(destDir, name)
+			destPath := filepath.Join(destDir, relPath)
 
 			// Security: prevent path traversal
 			if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(destDir)) {
@@ -267,6 +273,44 @@ func (h *SurfaceHandler) InstallSurface(c *gin.Context) {
 	})
 }
 
+// extractableRelPath returns the relative path for a zip entry if it
+// belongs to an extractable static directory (js/, css/, assets/).
+// Returns "" if the file should be skipped.
+//
+// Handles two archive layouts:
+//
+//	Prefixed:  my-surface/js/app.js  → js/app.js
+//	Flat:      js/app.js             → js/app.js
+//
+// Files not under js/, css/, or assets/ (e.g., manifest.json, script.js
+// at root) are skipped.
+func extractableRelPath(name string) string {
+	// Static directory prefixes we extract
+	staticPrefixes := []string{"js/", "css/", "assets/"}
+
+	// Check if already a relative static path (flat archive)
+	for _, p := range staticPrefixes {
+		if strings.HasPrefix(name, p) {
+			return name
+		}
+	}
+
+	// Check for a single directory prefix to strip
+	idx := strings.Index(name, "/")
+	if idx <= 0 {
+		return "" // No slash, or starts with slash — skip
+	}
+
+	rest := name[idx+1:]
+	for _, p := range staticPrefixes {
+		if strings.HasPrefix(rest, p) {
+			return rest
+		}
+	}
+
+	return "" // Not under a static directory
+}
+
 // ListEnabledSurfaces returns enabled surface IDs (non-admin, for nav).
 // GET /api/v1/surfaces
 func (h *SurfaceHandler) ListEnabledSurfaces(c *gin.Context) {