mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-15 22:15:00 +00:00
fix(storage): hide tenant roots from master storage API
Master tenant's base dir is the global data dir, which contains the tenants/ cross-tenant isolation root. Without this fix, the Storage page exposes tenant slugs, file names, and sizes. - Extract topLevelPath() helper, remove dead code - Add isHiddenPath() scoped to master tenant only - Guard handleList, handleRead, handleSize with hidden path checks - handleSize uses filepath.SkipDir to skip entire tenant subtree - Add regression tests for list, read, size, subpath, and non-master tenant isolation
This commit is contained in:
1 parent
484d434f6c
commit
99720892a7
2 files changed
+228
-9
No files matched your search
@@ -71,15 +71,16 @@ func (h *StorageHandler) tenantBaseDir(r *http.Request) string {
|
||||
// tenants (tenant isolation root — each tenant's data is scoped internally).
|
||||
var protectedDirs = []string{"skills", "skills-store", "media", "tenants"}
|
||||
|
||||
func isProtectedPath(rel string) bool {
|
||||
top := rel
|
||||
// topLevelPath returns the first path component of rel.
|
||||
func topLevelPath(rel string) string {
|
||||
if before, _, ok := strings.Cut(rel, "/"); ok {
|
||||
top = before
|
||||
}
|
||||
// Also handle forward slash on all platforms
|
||||
if i := strings.IndexByte(top, '/'); i >= 0 {
|
||||
top = top[:i]
|
||||
return before
|
||||
}
|
||||
return rel
|
||||
}
|
||||
|
||||
func isProtectedPath(rel string) bool {
|
||||
top := topLevelPath(rel)
|
||||
for _, d := range protectedDirs {
|
||||
if strings.EqualFold(top, d) {
|
||||
return true
|
||||
@@ -88,6 +89,19 @@ func isProtectedPath(rel string) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// isHiddenPath reports paths that should not be surfaced in the Storage UI/API.
|
||||
// Master tenant keeps its legacy base dir for backward compatibility, but must
|
||||
// not expose the cross-tenant isolation root.
|
||||
func (h *StorageHandler) isHiddenPath(r *http.Request, rel string) bool {
|
||||
if rel == "" {
|
||||
return false
|
||||
}
|
||||
if store.TenantIDFromContext(r.Context()) != store.MasterTenantID {
|
||||
return false
|
||||
}
|
||||
return strings.EqualFold(topLevelPath(rel), "tenants")
|
||||
}
|
||||
|
||||
// handleList lists files and directories under ~/.goclaw/ with depth limiting.
|
||||
// Query params:
|
||||
// - ?path= scopes the listing to a subtree
|
||||
@@ -111,6 +125,10 @@ func (h *StorageHandler) handleList(w http.ResponseWriter, r *http.Request) {
|
||||
base := h.tenantBaseDir(r)
|
||||
rootDir := base
|
||||
if subPath != "" {
|
||||
if h.isHiddenPath(r, subPath) {
|
||||
writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "path", subPath)})
|
||||
return
|
||||
}
|
||||
rootDir = filepath.Join(base, filepath.Clean(subPath))
|
||||
if !strings.HasPrefix(rootDir, base) {
|
||||
slog.Warn("security.storage_escape", "resolved", rootDir, "root", base)
|
||||
@@ -139,6 +157,14 @@ func (h *StorageHandler) handleList(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
rel, _ := filepath.Rel(base, path)
|
||||
|
||||
// Hide tenant isolation root from master storage listing.
|
||||
if h.isHiddenPath(r, rel) {
|
||||
if d.IsDir() {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// Skip symlinks
|
||||
if d.Type()&os.ModeSymlink != 0 {
|
||||
return nil
|
||||
@@ -240,7 +266,18 @@ func (h *StorageHandler) handleSize(w http.ResponseWriter, r *http.Request) {
|
||||
lastFlush := time.Now()
|
||||
|
||||
filepath.WalkDir(sizeBase, func(path string, d os.DirEntry, err error) error {
|
||||
if err != nil || d.IsDir() {
|
||||
if err != nil {
|
||||
return nil
|
||||
}
|
||||
rel, _ := filepath.Rel(sizeBase, path)
|
||||
// Skip hidden tenant root before d.IsDir() so we can SkipDir.
|
||||
if h.isHiddenPath(r, rel) {
|
||||
if d.IsDir() {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if d.IsDir() {
|
||||
return nil
|
||||
}
|
||||
if r.Context().Err() != nil {
|
||||
@@ -249,7 +286,6 @@ func (h *StorageHandler) handleSize(w http.ResponseWriter, r *http.Request) {
|
||||
if d.Type()&os.ModeSymlink != 0 {
|
||||
return nil
|
||||
}
|
||||
rel, _ := filepath.Rel(sizeBase, path)
|
||||
if skills.IsSystemArtifact(rel) {
|
||||
return nil
|
||||
}
|
||||
@@ -290,6 +326,10 @@ func (h *StorageHandler) handleRead(w http.ResponseWriter, r *http.Request) {
|
||||
writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)})
|
||||
return
|
||||
}
|
||||
if h.isHiddenPath(r, relPath) {
|
||||
writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgFileNotFound)})
|
||||
return
|
||||
}
|
||||
|
||||
readBase := h.tenantBaseDir(r)
|
||||
absPath := filepath.Join(readBase, filepath.Clean(relPath))
|
||||
|
||||
@@ -0,0 +1,179 @@
|
||||
package http
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||
)
|
||||
|
||||
// writeStorageTestFile creates a file with the given content for testing.
|
||||
func writeStorageTestFile(t *testing.T, path, content string) {
|
||||
t.Helper()
|
||||
if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
|
||||
t.Fatalf("MkdirAll(%s): %v", path, err)
|
||||
}
|
||||
if err := os.WriteFile(path, []byte(content), 0644); err != nil {
|
||||
t.Fatalf("WriteFile(%s): %v", path, err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStorageListHidesTenantRootForMaster(t *testing.T) {
|
||||
baseDir := t.TempDir()
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "master.txt"), "master")
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt"), "tenant-secret")
|
||||
|
||||
handler := NewStorageHandler(baseDir)
|
||||
req := httptest.NewRequest("GET", "/v1/storage/files", nil)
|
||||
req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID))
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
handler.handleList(w, req)
|
||||
if w.Code != 200 {
|
||||
t.Fatalf("status = %d, want 200", w.Code)
|
||||
}
|
||||
|
||||
var resp struct {
|
||||
Files []struct {
|
||||
Path string `json:"path"`
|
||||
Name string `json:"name"`
|
||||
} `json:"files"`
|
||||
}
|
||||
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
|
||||
t.Fatalf("unmarshal response: %v", err)
|
||||
}
|
||||
|
||||
for _, f := range resp.Files {
|
||||
if f.Path == "tenants" || strings.HasPrefix(f.Path, "tenants/") {
|
||||
t.Fatalf("master storage unexpectedly exposed tenant path %q", f.Path)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestStorageListSubpathTenantReturnsNotFound(t *testing.T) {
|
||||
baseDir := t.TempDir()
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt"), "tenant-secret")
|
||||
|
||||
handler := NewStorageHandler(baseDir)
|
||||
req := httptest.NewRequest("GET", "/v1/storage/files?path=tenants", nil)
|
||||
req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID))
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
handler.handleList(w, req)
|
||||
if w.Code != 404 {
|
||||
t.Fatalf("status = %d, want 404", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStorageReadTenantRootReturnsNotFoundForMaster(t *testing.T) {
|
||||
baseDir := t.TempDir()
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt"), "tenant-secret")
|
||||
|
||||
handler := NewStorageHandler(baseDir)
|
||||
req := httptest.NewRequest("GET", "/v1/storage/files/tenants/tenant-a/secret.txt", nil)
|
||||
req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID))
|
||||
req.SetPathValue("path", "tenants/tenant-a/secret.txt")
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
handler.handleRead(w, req)
|
||||
if w.Code != 404 {
|
||||
t.Fatalf("status = %d, want 404", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStorageSizeExcludesTenantRootForMaster(t *testing.T) {
|
||||
baseDir := t.TempDir()
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "master.txt"), "12345")
|
||||
writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt"), "1234567890")
|
||||
|
||||
handler := NewStorageHandler(baseDir)
|
||||
req := httptest.NewRequest("GET", "/v1/storage/size", nil)
|
||||
req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID))
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
handler.handleSize(w, req)
|
||||
if w.Code != 200 {
|
||||
t.Fatalf("status = %d, want 200", w.Code)
|
||||
}
|
||||
|
||||
body := w.Body.String()
|
||||
lines := strings.Split(strings.TrimSpace(body), "\n")
|
||||
if len(lines) == 0 {
|
||||
t.Fatal("expected SSE response body")
|
||||
}
|
||||
|
||||
// Find the final "done" event.
|
||||
last := lines[len(lines)-1]
|
||||
if !strings.HasPrefix(last, "data: ") {
|
||||
t.Fatalf("unexpected SSE line %q", last)
|
||||
}
|
||||
|
||||
var payload struct {
|
||||
Total int64 `json:"total"`
|
||||
Files int `json:"files"`
|
||||
Done bool `json:"done"`
|
||||
}
|
||||
if err := json.Unmarshal([]byte(strings.TrimPrefix(last, "data: ")), &payload); err != nil {
|
||||
t.Fatalf("unmarshal SSE payload: %v", err)
|
||||
}
|
||||
if !payload.Done {
|
||||
t.Fatalf("expected final SSE payload, got %#v", payload)
|
||||
}
|
||||
if payload.Total != 5 {
|
||||
t.Fatalf("total = %d, want 5 (tenant files should be excluded)", payload.Total)
|
||||
}
|
||||
if payload.Files != 1 {
|
||||
t.Fatalf("files = %d, want 1 (tenant files should be excluded)", payload.Files)
|
||||
}
|
||||
}
|
||||
|
||||
// TestIsHiddenPathOnlyAffectsMaster verifies that isHiddenPath only blocks
|
||||
// the master tenant and leaves non-master tenants unaffected.
|
||||
func TestIsHiddenPathOnlyAffectsMaster(t *testing.T) {
|
||||
handler := NewStorageHandler(t.TempDir())
|
||||
nonMasterID := uuid.MustParse("0193a5b0-7000-7000-8000-000000000099")
|
||||
|
||||
masterReq := httptest.NewRequest("GET", "/", nil)
|
||||
masterReq = masterReq.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID))
|
||||
|
||||
otherReq := httptest.NewRequest("GET", "/", nil)
|
||||
otherReq = otherReq.WithContext(store.WithTenantID(context.Background(), nonMasterID))
|
||||
|
||||
// Master tenant: tenants paths are hidden.
|
||||
if !handler.isHiddenPath(masterReq, "tenants") {
|
||||
t.Fatal("expected 'tenants' to be hidden for master")
|
||||
}
|
||||
if !handler.isHiddenPath(masterReq, "tenants/foo/bar") {
|
||||
t.Fatal("expected 'tenants/foo/bar' to be hidden for master")
|
||||
}
|
||||
if !handler.isHiddenPath(masterReq, "Tenants") {
|
||||
t.Fatal("expected case-insensitive match for master")
|
||||
}
|
||||
|
||||
// Non-master tenant: tenants paths are NOT hidden.
|
||||
if handler.isHiddenPath(otherReq, "tenants") {
|
||||
t.Fatal("tenants should not be hidden for non-master tenant")
|
||||
}
|
||||
if handler.isHiddenPath(otherReq, "tenants/foo") {
|
||||
t.Fatal("tenants/foo should not be hidden for non-master tenant")
|
||||
}
|
||||
|
||||
// Empty path is never hidden.
|
||||
if handler.isHiddenPath(masterReq, "") {
|
||||
t.Fatal("empty path should never be hidden")
|
||||
}
|
||||
|
||||
// Non-tenants paths are never hidden.
|
||||
if handler.isHiddenPath(masterReq, "skills") {
|
||||
t.Fatal("non-tenants path should not be hidden")
|
||||
}
|
||||
if handler.isHiddenPath(masterReq, "my-tenants") {
|
||||
t.Fatal("partial match should not be hidden")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user