From bdd6a23e92bec5ccd4201cf1d14f5443560ae702 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 7 Oct 2026 15:19:19 -0400 Subject: [PATCH 1/3] fix: make new file creation atomic and non-destructive Signed-off-by: Baylem --- agent/internal/files/create_linux_test.go | 210 ++++++++++++++++++ agent/internal/files/files.go | 24 +- agent/internal/files/rooted_linux.go | 66 +++++- agent/specs.md | 1 + api/internal/gatewayprotocol/protocol.go | 2 +- api/internal/gatewayprotocol/protocol_test.go | 3 + api/internal/rbac/rbac_test.go | 2 + api/internal/ws/create_route_test.go | 111 +++++++++ api/internal/ws/dialer.go | 1 + api/internal/ws/dialer_proxy_test.go | 1 + web/specs.md | 3 +- web/src/lib/endpoints.test.ts | 33 ++- web/src/lib/endpoints.ts | 12 + web/src/routes/tabs/Files.test.tsx | 30 ++- web/src/routes/tabs/Files.tsx | 2 +- 15 files changed, 480 insertions(+), 21 deletions(-) create mode 100644 agent/internal/files/create_linux_test.go create mode 100644 api/internal/ws/create_route_test.go diff --git a/agent/internal/files/create_linux_test.go b/agent/internal/files/create_linux_test.go new file mode 100644 index 000000000..5456a9531 --- /dev/null +++ b/agent/internal/files/create_linux_test.go @@ -0,0 +1,210 @@ +package files + +import ( + "bytes" + "errors" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +func assertNoCreateTemp(t *testing.T, dir string) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".create-") { + t.Fatalf("temporary file remains: %s", entry.Name()) + } + } +} + +func TestCreateHTTPRefusesExistingEntries(t *testing.T) { + for _, kind := range []string{"file", "directory", "symlink"} { + t.Run(kind, func(t *testing.T) { + url, root := newServer(t) + target := filepath.Join(root, "existing") + switch kind { + case "file": + put(t, target, "keep this content") + case "directory": + put(t, filepath.Join(target, "child"), "keep child") + case "symlink": + // A dotfile target must still report a collision, without inspecting it. + put(t, filepath.Join(root, ".secret"), "keep secret") + if err := os.Symlink(".secret", target); err != nil { + t.Fatal(err) + } + } + var before unix.Stat_t + if err := unix.Lstat(target, &before); err != nil { + t.Fatal(err) + } + resp, err := testPost(t, url+"/files/create?path=/existing", "application/octet-stream", strings.NewReader("")) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status = %d, want 409", resp.StatusCode) + } + var after unix.Stat_t + if err := unix.Lstat(target, &after); err != nil || before != after { + t.Fatalf("existing entry metadata changed: before=%+v after=%+v err=%v", before, after, err) + } + contentPath, want := target, "keep this content" + if kind == "directory" { + contentPath, want = filepath.Join(target, "child"), "keep child" + } else if kind == "symlink" { + contentPath, want = filepath.Join(root, ".secret"), "keep secret" + if link, err := os.Readlink(target); err != nil || link != ".secret" { + t.Fatalf("symlink changed: %q %v", link, err) + } + } + got, err := os.ReadFile(contentPath) + if err != nil || string(got) != want { + t.Fatalf("content = %q, error = %v", got, err) + } + assertNoCreateTemp(t, root) + }) + } +} + +func TestCreateHTTPAndWriteOverwrite(t *testing.T) { + url, root := newServer(t) + for _, operation := range []struct{ path, body string }{{"create", ""}, {"write", "edited"}} { + resp, err := testPost(t, url+"/files/"+operation.path+"?path=/sub/new.txt", "application/octet-stream", strings.NewReader(operation.body)) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("%s status = %d", operation.path, resp.StatusCode) + } + got, err := os.ReadFile(filepath.Join(root, "sub", "new.txt")) + if err != nil || string(got) != operation.body { + t.Fatalf("content = %q, error = %v", got, err) + } + info, err := os.Stat(filepath.Join(root, "sub", "new.txt")) + if err != nil || info.Mode().Perm() != 0o644 { + t.Fatalf("new-file permissions = %v, error = %v", info, err) + } + assertNoCreateTemp(t, filepath.Join(root, "sub")) + } +} + +func TestCreateCommitCollision(t *testing.T) { + for _, kind := range []string{"file", "directory", "symlink"} { + t.Run(kind, func(t *testing.T) { + h, root, outside := raceHandler(t) + target := filepath.Join(root, "new.txt") + raceAt(t, h, "commit", "", "new.txt", 1, func() { + switch kind { + case "file": + put(t, target, "concurrent creator") + case "directory": + if err := os.Mkdir(target, 0o700); err != nil { + t.Fatal(err) + } + case "symlink": + if err := os.Symlink(filepath.Join(outside, "secret.txt"), target); err != nil { + t.Fatal(err) + } + } + }) + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", strings.NewReader("new body")) + if rr.Code != http.StatusConflict || !strings.Contains(rr.Body.String(), "already exists") { + t.Fatalf("status=%d body=%q", rr.Code, rr.Body.String()) + } + info, err := os.Lstat(target) + if err != nil { + t.Fatal(err) + } + if kind == "file" { + got, err := os.ReadFile(target) + if err != nil || string(got) != "concurrent creator" { + t.Fatalf("concurrent content=%q err=%v", got, err) + } + } else if (kind == "directory" && !info.IsDir()) || (kind == "symlink" && info.Mode()&os.ModeSymlink == 0) { + t.Fatalf("concurrent entry replaced: %v", info) + } + assertNoCreateTemp(t, root) + assertOutsideUntouched(t, outside) + }) + } +} + +func TestCreateFailureDoesNotPublish(t *testing.T) { + for _, failure := range []string{"body", "metadata", "body limit"} { + t.Run(failure, func(t *testing.T) { + h, root, _ := raceHandler(t) + body := &errAfterReader{n: 4} + if failure == "metadata" { + h.preserveAccess = func(*os.File, fileAccess) error { return errors.New("metadata failure") } + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", strings.NewReader("payload")) + if rr.Code != http.StatusConflict { + t.Fatalf("status=%d", rr.Code) + } + } else if failure == "body limit" { + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", bytes.NewReader(bytes.Repeat([]byte("a"), maxWriteBytes+1))) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("oversized body status=%d", rr.Code) + } + } else { + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", body) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("failed body status=%d", rr.Code) + } + } + if _, err := os.Lstat(filepath.Join(root, "new.txt")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("target published: %v", err) + } + assertNoCreateTemp(t, root) + }) + } +} + +func TestCreateRemovesInheritedAccessACL(t *testing.T) { + h, root, _ := raceHandler(t) + acl := encodeACL([]aclEntry{{aclUserObj, 7, aclUndefined}, {aclUser, 4, uint32(os.Getuid() + 1)}, {aclGroupObj, 0, aclUndefined}, {aclMask, 4, aclUndefined}, {aclOther, 0, aclUndefined}}) + if err := unix.Setxattr(root, "system.posix_acl_default", acl, 0); err != nil { + if errors.Is(err, unix.EOPNOTSUPP) { + t.Skip("test filesystem lacks POSIX ACL support") + } + t.Fatal(err) + } + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", strings.NewReader("")) + if rr.Code != http.StatusNoContent { + t.Fatalf("status=%d body=%q", rr.Code, rr.Body.String()) + } + if _, err := unix.Getxattr(filepath.Join(root, "new.txt"), accessACLName, nil); !errors.Is(err, unix.ENODATA) { + t.Fatalf("inherited access ACL remains: %v", err) + } +} + +func TestCreateConfinedPaths(t *testing.T) { + h, root, outside := raceHandler(t) + for _, tc := range []struct { + path string + status int + }{{"/", http.StatusConflict}, {"/.hidden", http.StatusBadRequest}, {"/sub/.hidden/new", http.StatusBadRequest}} { + rr := serve(t, h.create, http.MethodPost, "/files/create?path="+tc.path, strings.NewReader("")) + if rr.Code != tc.status { + t.Fatalf("path=%s status=%d body=%q", tc.path, rr.Code, rr.Body.String()) + } + } + if err := os.Mkdir(filepath.Join(root, "sub"), 0o755); err != nil { + t.Fatal(err) + } + raceAt(t, h, "open", "", "sub", 1, func() { swapForSymlink(t, filepath.Join(root, "sub"), outside) }) + rr := serve(t, h.create, http.MethodPost, "/files/create?path=/sub/new.txt", strings.NewReader("")) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) + assertNoCreateTemp(t, root) +} diff --git a/agent/internal/files/files.go b/agent/internal/files/files.go index 5bfa121a3..2bb47a011 100644 --- a/agent/internal/files/files.go +++ b/agent/internal/files/files.go @@ -35,6 +35,9 @@ var errPathOutOfRoot = errors.New("path escapes root") // part of the file-browser surface. Like errPathOutOfRoot it is safe to echo. var errDotfile = errors.New("dotfile access denied") +// errFileExists is a client-safe conflict for create-only operations. +var errFileExists = errors.New("file already exists") + // hasDotComponent reports whether the slash-separated relative path rel // (already stripped of its leading slash) has a dot-prefixed component. func hasDotComponent(rel string) bool { @@ -67,6 +70,7 @@ func Mount(r chi.Router, root string) { bounded.Get("/list", h.list) bounded.Get("/read", h.read) bounded.Post("/write", h.write) + bounded.Post("/create", h.create) bounded.Post("/mkdir", h.mkdir) bounded.Delete("/delete", h.del) }) @@ -207,7 +211,7 @@ func (h *handler) download(w http.ResponseWriter, req *http.Request) { http.ServeContent(w, req, fi.Name(), fi.ModTime(), f) } -// maxWriteBytes caps a single /files/write body. The API-side ws proxy +// maxWriteBytes caps a single /files/write or /files/create body. The API-side ws proxy // already enforces 64 MiB, so this is defense-in-depth against direct // agent access with a valid mTLS cert. const maxWriteBytes = 64 << 20 @@ -224,6 +228,14 @@ const maxUploadFileBytes = 64 << 20 const maxUploadFiles = 64 func (h *handler) write(w http.ResponseWriter, req *http.Request) { + h.storeRequest(w, req, false) +} + +func (h *handler) create(w http.ResponseWriter, req *http.Request) { + h.storeRequest(w, req, true) +} + +func (h *handler) storeRequest(w http.ResponseWriter, req *http.Request, createOnly bool) { comps, err := h.components(req.URL.Query().Get("path")) if err != nil { h.fail(w, err) @@ -235,7 +247,11 @@ func (h *handler) write(w http.ResponseWriter, req *http.Request) { // only on success, so any error after the copy starts (ENOSPC, a body that // ends early, an agent restart mid-copy) leaves the previous file intact // (F-102). The whole sequence runs through one parent directory descriptor. - err = h.writeFile(comps, func(dst io.Writer) error { + store := h.writeFile + if createOnly { + store = h.createFile + } + err = store(comps, func(dst io.Writer) error { _, copyErr := io.Copy(dst, body) return copyErr }) @@ -344,7 +360,7 @@ func (h *handler) storePart(comps []string, filename string, src io.Reader, limi if strings.HasPrefix(name, ".") { return errDotfile } - return h.storeIn(comps, name, ".upload-", false, func(dst io.Writer) error { + return h.storeIn(comps, name, ".upload-", false, storeReplace, func(dst io.Writer) error { // Read one byte past the cap: if that byte materializes the part is // over the limit, whatever its multipart headers claimed. n, err := io.Copy(dst, io.LimitReader(src, limit+1)) @@ -393,6 +409,8 @@ func (h *handler) del(w http.ResponseWriter, req *http.Request) { func httpErr(w http.ResponseWriter, err error) { switch { + case errors.Is(err, errFileExists): + http.Error(w, errFileExists.Error(), http.StatusConflict) case errors.Is(err, errPreserveAccess): http.Error(w, "cannot preserve existing file access; the file changed or the filesystem does not support its group/POSIX ACL permissions", http.StatusConflict) case errors.Is(err, os.ErrNotExist): diff --git a/agent/internal/files/rooted_linux.go b/agent/internal/files/rooted_linux.go index 05f0848a1..fbc0eb93c 100644 --- a/agent/internal/files/rooted_linux.go +++ b/agent/internal/files/rooted_linux.go @@ -355,15 +355,40 @@ func createTemp(dirFD int, prefix string) (*os.File, string, error) { return nil, "", fmt.Errorf("create temp file: %w", unix.EEXIST) } -// storeFile writes name inside dirFD atomically: fill streams the content -// into a temp file created in dirFD, which is chmodded, fsynced and renamed -// over name with renameat in the same directory. On any failure only the temp -// file is removed; an existing name is left untouched. renameat replaces a -// symlink that appears at name in the meantime instead of following it. -func (h *handler) storeFile(dirFD int, parents []string, name, prefix string, fill func(io.Writer) error) error { - access, err := h.destAccess(dirFD, parents, name) - if err != nil { - return err +type storeMode uint8 + +const ( + storeReplace storeMode = iota + storeCreate +) + +// storeFile fills, applies access metadata, syncs and closes a temporary file +// in dirFD before committing it. Create uses RENAME_NOREPLACE to atomically +// refuse any existing entry, including a symlink or concurrent creator. +// Replace keeps the existing access-preserving renameat behavior. Failures +// remove only the temporary entry; no unsupported no-replace fallback exists. +func (h *handler) storeFile(dirFD int, parents []string, name, prefix string, mode storeMode, fill func(io.Writer) error) error { + var access fileAccess + var err error + if mode == storeCreate { + // This probe avoids copying a body for an existing name. It never + // opens the entry or inspects a symlink target; the commit syscall, + // rather than this probe, enforces absence against concurrent writes. + h.fire("stat", parents, name) + var st unix.Stat_t + err = unix.Fstatat(dirFD, name, &st, unix.AT_SYMLINK_NOFOLLOW) + if err == nil { + return errFileExists + } + if !errors.Is(err, unix.ENOENT) { + return err + } + // The empty access policy applies 0644 and removes inherited access ACLs. + } else { + access, err = h.destAccess(dirFD, parents, name) + if err != nil { + return err + } } tmp, tmpName, err := createTemp(dirFD, prefix) if err != nil { @@ -392,6 +417,15 @@ func (h *handler) storeFile(dirFD int, parents []string, name, prefix string, fi return abort(fmt.Errorf("close %q: %w", name, err)) } h.fire("commit", parents, name) + if mode == storeCreate { + if err = unix.Renameat2(dirFD, tmpName, dirFD, name, unix.RENAME_NOREPLACE); err != nil { + if errors.Is(err, unix.EEXIST) { + return abort(errFileExists) + } + return abort(fmt.Errorf("create %q: %w", name, err)) + } + return nil + } if !access.unchanged(dirFD, name) { return abort(fmt.Errorf("%w: destination changed during upload", errPreserveAccess)) } @@ -403,7 +437,7 @@ func (h *handler) storeFile(dirFD int, parents []string, name, prefix string, fi // storeIn opens the directory comps (creating missing ancestors when create is // set) and stores name in it atomically via storeFile. -func (h *handler) storeIn(comps []string, name, prefix string, create bool, fill func(io.Writer) error) error { +func (h *handler) storeIn(comps []string, name, prefix string, create bool, mode storeMode, fill func(io.Writer) error) error { rootFD, err := openRoot(h.root) if err != nil { return err @@ -414,7 +448,7 @@ func (h *handler) storeIn(comps []string, name, prefix string, create bool, fill return err } defer closeFD(dirFD) - return h.storeFile(dirFD, comps, name, prefix, fill) + return h.storeFile(dirFD, comps, name, prefix, mode, fill) } // writeFile atomically writes the file comps, creating missing ancestors. @@ -422,7 +456,15 @@ func (h *handler) writeFile(comps []string, fill func(io.Writer) error) error { if len(comps) == 0 { return fmt.Errorf("write root: %w", unix.EISDIR) } - return h.storeIn(parentsOf(comps), comps[len(comps)-1], ".write-", true, fill) + return h.storeIn(parentsOf(comps), comps[len(comps)-1], ".write-", true, storeReplace, fill) +} + +// createFile atomically creates the file comps, refusing any existing entry. +func (h *handler) createFile(comps []string, fill func(io.Writer) error) error { + if len(comps) == 0 { + return errFileExists + } + return h.storeIn(parentsOf(comps), comps[len(comps)-1], ".create-", true, storeCreate, fill) } // removeEntry removes the single entry name in dirFD like os.Remove: unlink, diff --git a/agent/specs.md b/agent/specs.md index 764cd1552..b3af673df 100644 --- a/agent/specs.md +++ b/agent/specs.md @@ -164,6 +164,7 @@ Resource usage env vars (set by the operator): | `/files/read` | GET | Read file inline (small files only); query param `path` | | `/files/download` | GET | Download file as attachment; query param `path` | | `/files/write` | POST | Overwrite or create file; query param `path`; body is raw file content | +| `/files/create` | POST | Create only; query param `path`; raw body capped at 64 MiB. Atomically refuses any existing final entry with 409 `file already exists`, including directories, symlinks and concurrent creators. Shares rooted traversal and 0644/no inherited access-ACL new-file policy; cleans temporary files on failure and fails closed if atomic no-replace is unsupported. Older agents return 404; clients must never fall back to `/files/write`. | | `/files/upload` | POST | Upload one or more files to a directory; query param `path`; body is `multipart/form-data` with `files[]`; returns HTTP 400 if the destination directory or a destination file name is, or is reached through, a symlink | | `/files/mkdir` | POST | Create directory (recursive); query param `path` | | `/files/delete` | DELETE | Delete file or directory; query param `path`, optional `recursive` (boolean) | diff --git a/api/internal/gatewayprotocol/protocol.go b/api/internal/gatewayprotocol/protocol.go index 1d092bc88..584f2d41d 100644 --- a/api/internal/gatewayprotocol/protocol.go +++ b/api/internal/gatewayprotocol/protocol.go @@ -62,7 +62,7 @@ func Allowed(method, path string) (int64, bool) { return 16 << 10, true case "/mods/upload": return 512 << 20, true - case "/files/write", "/files/upload", "/files/mkdir", "/players/kick", "/players/ban", "/players/unban", "/players/whitelist/add", "/players/whitelist/remove", "/mods/install": + case "/files/write", "/files/create", "/files/upload", "/files/mkdir", "/players/kick", "/players/ban", "/players/unban", "/players/whitelist/add", "/players/whitelist/remove", "/mods/install": return 64 << 20, true } case http.MethodDelete: diff --git a/api/internal/gatewayprotocol/protocol_test.go b/api/internal/gatewayprotocol/protocol_test.go index 8889f0553..bdaf77e83 100644 --- a/api/internal/gatewayprotocol/protocol_test.go +++ b/api/internal/gatewayprotocol/protocol_test.go @@ -36,6 +36,9 @@ func TestOperationAllowlist(t *testing.T) { }{ {http.MethodGet, "/console", 0, true}, {http.MethodPost, "/files/write", 64 << 20, true}, + {http.MethodPost, "/files/create", 64 << 20, true}, + {http.MethodGet, "/files/create", 0, false}, + {http.MethodPut, "/files/create", 0, false}, {http.MethodPost, "/mods/upload", 512 << 20, true}, {http.MethodPost, "/actions/run", 16 << 10, true}, {http.MethodGet, "/files/write", 0, false}, diff --git a/api/internal/rbac/rbac_test.go b/api/internal/rbac/rbac_test.go index 5bcb20e0f..42a8abe53 100644 --- a/api/internal/rbac/rbac_test.go +++ b/api/internal/rbac/rbac_test.go @@ -70,6 +70,8 @@ func TestAllow(t *testing.T) { }{ {RoleViewer, "GET", "/servers", scope.DefaultCluster, true}, {RoleViewer, "POST", "/servers", scope.DefaultCluster, false}, + {RoleViewer, "POST", "/servers/foo/files/create", scope.DefaultCluster, false}, + {RoleOperator, "POST", "/servers/foo/files/create", scope.DefaultCluster, true}, {RoleOperator, "POST", "/servers", scope.DefaultCluster, true}, {RoleOperator, "DELETE", "/users/1", scope.DefaultCluster, false}, {RoleAdmin, "DELETE", "/users/1", scope.DefaultCluster, true}, diff --git a/api/internal/ws/create_route_test.go b/api/internal/ws/create_route_test.go new file mode 100644 index 000000000..781a72498 --- /dev/null +++ b/api/internal/ws/create_route_test.go @@ -0,0 +1,111 @@ +package ws + +import ( + "encoding/pem" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/ValgulNecron/gameplane/api/internal/gatewayprotocol" + "github.com/ValgulNecron/gameplane/api/internal/kube" +) + +func TestLocalCreateProxyPreservesResponseWithoutWriteFallback(t *testing.T) { + for _, status := range []int{http.StatusNoContent, http.StatusConflict, http.StatusNotFound} { + t.Run(http.StatusText(status), func(t *testing.T) { + calls := 0 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + calls++ + if req.Method != http.MethodPost || req.URL.Path != "/v1/targets/gs-uid/files/create" { + t.Errorf("unsafe upstream operation: %s %s", req.Method, req.URL.Path) + } + if req.URL.Query().Get("path") != "/new #?.txt" { + t.Errorf("path query = %s", req.URL.RawQuery) + } + body, err := io.ReadAll(req.Body) + if err != nil || string(body) != "payload" { + t.Errorf("body=%q err=%v", body, err) + } + w.WriteHeader(status) + if status != http.StatusNoContent { + _, _ = w.Write([]byte("create refused")) + } + })) + defer srv.Close() + k := &kube.Client{} + _ = streamTestRegistry(k) + p := &proxy{k: k, transport: testDirectTransport(srv)} + r := chi.NewRouter() + r.Post("/servers/{name}/files/create", p.agentHTTP("/files/create")) + rr := httptest.NewRecorder() + r.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodPost, + "/servers/alpha/files/create?path=%2Fnew%20%23%3F.txt", strings.NewReader("payload"))) + if rr.Code != status || calls != 1 { + t.Fatalf("status=%d calls=%d", rr.Code, calls) + } + if status != http.StatusNoContent && rr.Body.String() != "create refused" { + t.Fatalf("body=%q", rr.Body.String()) + } + }) + } +} + +func TestRemoteCreateProxyPreservesScopeAndRefusalWithoutFallback(t *testing.T) { + for _, status := range []int{http.StatusNoContent, http.StatusConflict, http.StatusNotFound} { + t.Run(http.StatusText(status), func(t *testing.T) { + calls := 0 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + calls++ + target, operation, err := gatewayprotocol.ParsePath(req.URL.Path) + if err != nil || target != (gatewayprotocol.Target{Cluster: "remote", Namespace: "gameplane-games", Name: "alpha", UID: "remote-uid"}) || operation != "/files/create" || req.Method != http.MethodPost { + t.Errorf("target=%+v operation=%s method=%s err=%v", target, operation, req.Method, err) + } + if req.URL.RawQuery != "path=%2Fnew+%23%3F.txt" { + t.Errorf("query=%s", req.URL.RawQuery) + } + if req.Header.Get("Cookie") != "" || req.Header.Get("X-Gameplane-CSRF") != "" { + t.Error("session credentials forwarded") + } + body, err := io.ReadAll(req.Body) + if err != nil || string(body) != "payload" { + t.Errorf("body=%q err=%v", body, err) + } + w.WriteHeader(status) + if status != http.StatusNoContent { + _, _ = w.Write([]byte("create refused")) + } + })) + defer srv.Close() + resolver, home, _ := gatewayFixture(t, srv.URL) + secret, err := home.Typed.CoreV1().Secrets("gameplane-system").Get(t.Context(), "gateway-client", metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + secret.Data["ca.crt"] = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) + if _, err := home.Typed.CoreV1().Secrets("gameplane-system").Update(t.Context(), secret, metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + p := &proxy{k: home, gateway: resolver} + r := chi.NewRouter() + r.Post("/servers/{name}/files/create", p.agentHTTP("/files/create")) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, + "/servers/alpha/files/create?cluster=remote&namespace=gameplane-games&path=%2Fnew%20%23%3F.txt", strings.NewReader("payload")) + req.Header.Set("Content-Type", "application/octet-stream") + req.Header.Set("Cookie", "session=private") + req.Header.Set("X-Gameplane-CSRF", "private") + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + if rr.Code != status || calls != 1 { + t.Fatalf("status=%d calls=%d body=%s", rr.Code, calls, rr.Body) + } + if status != http.StatusNoContent && rr.Body.String() != "create refused" { + t.Fatalf("body=%q", rr.Body.String()) + } + }) + } +} diff --git a/api/internal/ws/dialer.go b/api/internal/ws/dialer.go index a7ecdc066..3fefcea7a 100644 --- a/api/internal/ws/dialer.go +++ b/api/internal/ws/dialer.go @@ -60,6 +60,7 @@ func Mount(r chi.Router, reg *kube.Registry, caBundle, clientCert, clientKey str r.Get("/read", p.agentHTTP("/files/read")) r.Get("/download", p.agentHTTP("/files/download")) r.Post("/write", p.agentHTTP("/files/write")) + r.Post("/create", p.agentHTTP("/files/create")) r.Post("/upload", p.agentHTTP("/files/upload")) r.Post("/mkdir", p.agentHTTP("/files/mkdir")) r.Delete("/delete", p.agentHTTP("/files/delete")) diff --git a/api/internal/ws/dialer_proxy_test.go b/api/internal/ws/dialer_proxy_test.go index b2788668b..e83df4665 100644 --- a/api/internal/ws/dialer_proxy_test.go +++ b/api/internal/ws/dialer_proxy_test.go @@ -89,6 +89,7 @@ func TestMount_ActionsAndStatusRouted(t *testing.T) { method, path string }{ {"POST", "/servers/alpha/actions/run"}, + {"POST", "/servers/alpha/files/create"}, {"GET", "/servers/alpha/status"}, {"GET", "/servers/alpha/mods"}, {"POST", "/servers/alpha/mods/install"}, diff --git a/web/specs.md b/web/specs.md index 2515e9c8c..464b8625b 100644 --- a/web/specs.md +++ b/web/specs.md @@ -289,6 +289,7 @@ Six of the nine ServerDetail tabs are rebuilt in this slice: - The new file/folder name prompt explains a rejected name inline ("Names can't contain "/" or be "." or "..".") and marks the input `aria-invalid`; Create stays disabled - Monaco editor unchanged - File drafts bind to the selected path, resource target and selection generation. Selection clears content immediately and Save stays disabled until that file has loaded. Late reads and save completions cannot replace another selection's content or baseline, including reopening the same path. Saves capture their target, path, body and directory at click time; edits typed during a save remain dirty against the saved body. Changing server/namespace/cluster/UID starts a fresh file browser. + - New file uses POST `/files/create` with the encoded path, namespace/cluster scope, credentials and CSRF headers. The agent atomically refuses existing entries with 409; a conflict or unsupported older agent/gateway keeps the prompt open, shows the error and does not select a file or invalidate the list. Never fall back to `/files/write`; editor saves and uploads retain overwrite behavior. 6. **Players** (`web/src/routes/tabs/Players.tsx`, 386 lines) - Online player snapshot, ban list, whitelist management @@ -1035,7 +1036,7 @@ Each namespace is an object of typed functions building and fetching URLs: - **ModRegistries** — `putSecret(provider, apiKey)`, `deleteSecret(provider)` -- **Files** — `list(server, path, ns?)`, `read(server, path, ns?)`, `write(server, path, content, ns?)`, `mkdir(server, path, ns?)`, `remove(server, path, recursive?, ns?)`, `upload(server, dir, files, ns?)`, `downloadURL(server, path, ns?)` +- **Files** — `list(server, path, ns?)`, `read(server, path, ns?)`, `write(server, path, content, ns?)`, `create(server, path, content, ns?)`, `mkdir(server, path, ns?)`, `remove(server, path, recursive?, ns?)`, `upload(server, dir, files, ns?)`, `downloadURL(server, path, ns?)` - **Logs** — `downloadURL(server, ns?)`, `fileStreamPath(server, ns?)` (WebSocket), `podStreamPath(server, ns?)` (WebSocket) diff --git a/web/src/lib/endpoints.test.ts b/web/src/lib/endpoints.test.ts index ceb8394ef..077e51c0d 100644 --- a/web/src/lib/endpoints.test.ts +++ b/web/src/lib/endpoints.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { Audit, Auth, Clusters, Files, Logs, Players, Servers, Templates, Users } from "./endpoints"; +import { Audit, Auth, Clusters, Files, Logs, Players, Servers, Templates, Users, createResourceClient } from "./endpoints"; // Mock the cluster module so we don't depend on actual localStorage in tests vi.mock("./cluster", () => ({ @@ -148,6 +148,37 @@ describe("endpoints", () => { expect(c.init.body).toBe("k=v\n"); }); + it("create POSTs encoded octet-stream content with namespace and credentials", async () => { + fetchMock.mockImplementation(async () => new Response(null, { status: 204 })); + await Files.create("mc-survival", "/new #?.txt", "", "team-c"); + const c = called(); + expect(c.url).toBe("/servers/mc-survival/files/create?path=%2Fnew%20%23%3F.txt&namespace=team-c"); + expect(c.init.method).toBe("POST"); + expect(c.init.headers?.["Content-Type"]).toBe("application/octet-stream"); + expect(c.init.body).toBe(""); + expect(fetchMock.mock.calls.at(-1)?.[1]?.credentials).toBe("include"); + }); + + it.each([409, 404])("create fails closed on %s without retrying write", async (status) => { + fetchMock.mockImplementation(async () => new Response("create failed", { status })); + await expect(Files.create("mc-survival", "/existing.txt", "")).rejects.toThrow("create failed"); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(called().url).toBe("/servers/mc-survival/files/create?path=%2Fexisting.txt"); + }); + + it("create preserves the captured remote cluster and sends the CSRF token", async () => { + fetchMock.mockImplementation(async () => new Response(null, { status: 204 })); + document.cookie = "gameplane_csrf=create-token; path=/"; + try { + const client = createResourceClient({ cluster: "remote", namespace: "team-c" }); + await client.Files.create("mc-survival", "/new.txt", ""); + expect(called().url).toBe("/servers/mc-survival/files/create?path=%2Fnew.txt&namespace=team-c&cluster=remote"); + expect(called().init.headers?.["X-Gameplane-CSRF"]).toBe("create-token"); + } finally { + document.cookie = "gameplane_csrf=; Max-Age=0; path=/"; + } + }); + it("mkdir POSTs to /files/mkdir with empty body", async () => { fetchMock.mockImplementation(async () => new Response(null, { status: 204 })); await Files.mkdir("mc-survival", "/mods-disabled"); diff --git a/web/src/lib/endpoints.ts b/web/src/lib/endpoints.ts index 14069f1af..869039cc0 100644 --- a/web/src/lib/endpoints.ts +++ b/web/src/lib/endpoints.ts @@ -946,6 +946,18 @@ function makeResourceEndpoints(request: RequestClient) { body: content, }); }, + create: async ( + server: string, + path: string, + content: string | Blob, + ns?: string, + ): Promise => { + await filesFetch(withNS(`${filesBase(server)}/create?path=${encodeURIComponent(path)}`, ns), { + method: "POST", + headers: { "Content-Type": "application/octet-stream", ...csrfHeaders() }, + body: content, + }); + }, mkdir: async (server: string, path: string, ns?: string): Promise => { await filesFetch(withNS(`${filesBase(server)}/mkdir?path=${encodeURIComponent(path)}`, ns), { method: "POST", diff --git a/web/src/routes/tabs/Files.test.tsx b/web/src/routes/tabs/Files.test.tsx index 6a8d6bcc9..03a03cc86 100644 --- a/web/src/routes/tabs/Files.test.tsx +++ b/web/src/routes/tabs/Files.test.tsx @@ -603,12 +603,13 @@ describe("FilesTab", () => { fetchMock.mockImplementation(async (url: string, init?: FetchInit) => { if (url.startsWith("/servers/mc-survival/files/list")) return jsonRes(ROOT_ENTRIES); if ( - url === "/servers/mc-survival/files/write?path=%2Fnew-file.txt" && + url === "/servers/mc-survival/files/create?path=%2Fnew-file.txt" && init?.method === "POST" ) { createCalled = true; return new Response(null, { status: 204 }); } + if (url === "/servers/mc-survival/files/read?path=%2Fnew-file.txt") return textRes(""); throw new Error(`unexpected fetch: ${url}`); }); renderWithQuery(); @@ -620,6 +621,31 @@ describe("FilesTab", () => { fireEvent.click(screen.getByRole("button", { name: /^Create$/ })); }); await waitFor(() => expect(createCalled).toBe(true)); + expect(await screen.findByTestId("monaco")).toHaveValue(""); + expect(screen.queryByPlaceholderText("config.yaml")).not.toBeInTheDocument(); + }); + + it.each([409, 404])("keeps the new-file dialog open after create fails with %s", async (status) => { + const error = status === 409 ? "file already exists" : "not found"; + fetchMock.mockImplementation(async (url: string) => { + if (url.startsWith("/servers/mc-survival/files/list")) return jsonRes(ROOT_ENTRIES); + if (url === "/servers/mc-survival/files/create?path=%2Fserver.properties") return textRes(error, status); + throw new Error(`unexpected fetch: ${url}`); + }); + renderWithQuery(); + await screen.findByText("server.properties"); + const initialLists = fetchMock.mock.calls.filter(([url]) => String(url).includes("/files/list")).length; + fireEvent.click(screen.getByRole("button", { name: /New file/ })); + const input = await screen.findByPlaceholderText("config.yaml"); + fireEvent.change(input, { target: { value: "server.properties" } }); + await act(async () => { fireEvent.click(screen.getByRole("button", { name: /^Create$/ })); }); + await screen.findByText(`${status}: ${error}`); + expect(input).toBeInTheDocument(); + expect(input).toHaveValue("server.properties"); + expect(screen.getByRole("button", { name: /^Create$/ })).toBeEnabled(); + expect(screen.queryByTestId("monaco")).not.toBeInTheDocument(); + expect(fetchMock.mock.calls.some(([url]) => String(url).includes("/files/write") || String(url).includes("/files/read"))).toBe(false); + expect(fetchMock.mock.calls.filter(([url]) => String(url).includes("/files/list"))).toHaveLength(initialLists); }); it("deletes a folder from its row button without opening it", async () => { @@ -1100,7 +1126,7 @@ describe("FilesTab", () => { return jsonRes(ROOT_ENTRIES); } if ( - url === "/servers/mc-survival/files/write?path=%2Fnew.txt" && + url === "/servers/mc-survival/files/create?path=%2Fnew.txt" && init?.method === "POST" ) { return new Response(null, { status: 204 }); diff --git a/web/src/routes/tabs/Files.tsx b/web/src/routes/tabs/Files.tsx index 92c86c2d6..9a45325bd 100644 --- a/web/src/routes/tabs/Files.tsx +++ b/web/src/routes/tabs/Files.tsx @@ -221,7 +221,7 @@ function FilesBrowser({ resourceTarget }: { resourceTarget: ResourceTarget }) { const newFileMutation = useMutation({ mutationFn: async (fileName: string) => { const path = joinPath(cwd, fileName); - await Files.write(name, path, "", ns); + await Files.create(name, path, "", ns); return path; }, onSuccess: async (path) => { From ce41a14a9bb241ec6208f2bc03354d74d960e704 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 7 Oct 2026 15:26:16 -0400 Subject: [PATCH 2/3] test: use tagged switches in create-only regressions Signed-off-by: Baylem --- agent/internal/files/create_linux_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/agent/internal/files/create_linux_test.go b/agent/internal/files/create_linux_test.go index 5456a9531..3944ba32b 100644 --- a/agent/internal/files/create_linux_test.go +++ b/agent/internal/files/create_linux_test.go @@ -59,9 +59,10 @@ func TestCreateHTTPRefusesExistingEntries(t *testing.T) { t.Fatalf("existing entry metadata changed: before=%+v after=%+v err=%v", before, after, err) } contentPath, want := target, "keep this content" - if kind == "directory" { + switch kind { + case "directory": contentPath, want = filepath.Join(target, "child"), "keep child" - } else if kind == "symlink" { + case "symlink": contentPath, want = filepath.Join(root, ".secret"), "keep secret" if link, err := os.Readlink(target); err != nil || link != ".secret" { t.Fatalf("symlink changed: %q %v", link, err) @@ -145,18 +146,19 @@ func TestCreateFailureDoesNotPublish(t *testing.T) { t.Run(failure, func(t *testing.T) { h, root, _ := raceHandler(t) body := &errAfterReader{n: 4} - if failure == "metadata" { + switch failure { + case "metadata": h.preserveAccess = func(*os.File, fileAccess) error { return errors.New("metadata failure") } rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", strings.NewReader("payload")) if rr.Code != http.StatusConflict { t.Fatalf("status=%d", rr.Code) } - } else if failure == "body limit" { + case "body limit": rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", bytes.NewReader(bytes.Repeat([]byte("a"), maxWriteBytes+1))) if rr.Code != http.StatusInternalServerError { t.Fatalf("oversized body status=%d", rr.Code) } - } else { + default: rr := serve(t, h.create, http.MethodPost, "/files/create?path=/new.txt", body) if rr.Code != http.StatusInternalServerError { t.Fatalf("failed body status=%d", rr.Code) From 4562c542ecc7998093ba7f868528ebd21a816f82 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 14 Oct 2026 19:58:05 -0400 Subject: [PATCH 3/3] test(api): use private gateway fixture for file creation Signed-off-by: Baylem --- api/internal/ws/create_route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/internal/ws/create_route_test.go b/api/internal/ws/create_route_test.go index 19bc1b8c4..8be25bdcd 100644 --- a/api/internal/ws/create_route_test.go +++ b/api/internal/ws/create_route_test.go @@ -59,7 +59,7 @@ func TestRemoteCreateProxyPreservesScopeAndRefusalWithoutFallback(t *testing.T) for _, status := range []int{http.StatusNoContent, http.StatusConflict, http.StatusNotFound} { t.Run(http.StatusText(status), func(t *testing.T) { calls := 0 - srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + srv := newGatewayTLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { calls++ target, operation, err := gatewayprotocol.ParsePath(req.URL.Path) if err != nil || target != (gatewayprotocol.Target{Cluster: "remote", Namespace: "gameplane-games", Name: "alpha", UID: "remote-uid"}) || operation != "/files/create" || req.Method != http.MethodPost {