diff --git a/agent/internal/files/access_linux_test.go b/agent/internal/files/access_linux_test.go index d9a2aad8a..fc999a892 100644 --- a/agent/internal/files/access_linux_test.go +++ b/agent/internal/files/access_linux_test.go @@ -13,6 +13,15 @@ import ( "golang.org/x/sys/unix" ) +// A named ACL user need not exist in passwd. Keep the fixture different from +// the current UID without overflow or producing the reserved ACL ID sentinel. +func otherACLUserID() uint32 { + if os.Getuid() == 1 { + return 2 + } + return 1 +} + func TestReplacementACLRetainsOriginalOwnerWithoutBroadeningOthers(t *testing.T) { for _, mode := range []uint32{0o600, 0o644} { st := unix.Stat_t{Uid: 1000, Mode: mode} @@ -66,7 +75,7 @@ func TestApplyReplacementACLOnRealFile(t *testing.T) { defer func() { _ = f.Close() }() // A real unprivileged file owner may install an exact-UID ACL for a // different UID without CAP_CHOWN. No privileged test setup is needed. - access := fileAccess{exists: true, stat: unix.Stat_t{Uid: uint32(os.Getuid() + 1), Gid: uint32(os.Getgid()), Mode: 0o600}} + access := fileAccess{exists: true, stat: unix.Stat_t{Uid: otherACLUserID(), Gid: uint32(os.Getgid()), Mode: 0o600}} if err := access.apply(f); err != nil { if errors.Is(err, unix.EOPNOTSUPP) { t.Skip("test filesystem lacks POSIX ACL support") @@ -82,7 +91,7 @@ func TestApplyReplacementACLOnRealFile(t *testing.T) { if err != nil { t.Fatal(err) } - want := []aclEntry{{aclUserObj, 6, aclUndefined}, {aclUser, 6, uint32(os.Getuid() + 1)}, {aclGroupObj, 0, aclUndefined}, {aclMask, 6, aclUndefined}, {aclOther, 0, aclUndefined}} + want := []aclEntry{{aclUserObj, 6, aclUndefined}, {aclUser, 6, otherACLUserID()}, {aclGroupObj, 0, aclUndefined}, {aclMask, 6, aclUndefined}, {aclOther, 0, aclUndefined}} if !reflect.DeepEqual(entries, want) { t.Fatalf("on-disk ACL=%v", entries) } diff --git a/agent/internal/files/create_linux_test.go b/agent/internal/files/create_linux_test.go new file mode 100644 index 000000000..c781e54f0 --- /dev/null +++ b/agent/internal/files/create_linux_test.go @@ -0,0 +1,212 @@ +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" + switch kind { + case "directory": + contentPath, want = filepath.Join(target, "child"), "keep child" + 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) + } + } + 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} + 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) + } + 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.StatusRequestEntityTooLarge { + t.Fatalf("oversized body status=%d", rr.Code) + } + 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) + } + } + 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, otherACLUserID()}, {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 8fd3e1890..6fd39fe0b 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) }) @@ -129,7 +133,7 @@ func (h *handler) badRequest(w http.ResponseWriter, err error) { // fail maps an error from path validation or a descriptor walk to a response: // bad-path classes are 400s via badRequest, everything else goes through -// httpErr (404/403/500). +// httpErr (404/403/409/413/500). func (h *handler) fail(w http.ResponseWriter, err error) { if errors.Is(err, errPathOutOfRoot) || errors.Is(err, errDotfile) || errors.Is(err, errBadPath) { h.badRequest(w, err) @@ -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,18 @@ const maxUploadFileBytes = 64 << 20 const maxUploadFiles = 64 func (h *handler) write(w http.ResponseWriter, req *http.Request) { + h.storeRequest(w, req, false) +} + +// create publishes a new file only if the destination remains absent; an +// existing entry returns 409, and an oversized body returns 413 without publishing. +func (h *handler) create(w http.ResponseWriter, req *http.Request) { + h.storeRequest(w, req, true) +} + +// storeRequest shares the bounded, atomic write path for create and replace. +// Copy failures leave the destination untouched; body-limit failures are 413. +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 +251,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 +364,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)) @@ -391,8 +411,15 @@ func (h *handler) del(w http.ResponseWriter, req *http.Request) { w.WriteHeader(http.StatusNoContent) } +// httpErr maps storage errors to safe HTTP responses, including wrapped body +// limits as 413. Internal error details are logged rather than returned. func httpErr(w http.ResponseWriter, err error) { + var tooLarge *http.MaxBytesError switch { + case errors.As(err, &tooLarge): + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) + 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/files_gaps_test.go b/agent/internal/files/files_gaps_test.go index 3e6974f00..6428caeac 100644 --- a/agent/internal/files/files_gaps_test.go +++ b/agent/internal/files/files_gaps_test.go @@ -34,8 +34,8 @@ func TestWrite_BodyTooLarge(t *testing.T) { t.Fatalf("post: %v", err) } defer resp.Body.Close() - if resp.StatusCode == http.StatusNoContent { - t.Fatal("oversize write should not succeed") + if resp.StatusCode != http.StatusRequestEntityTooLarge { + t.Fatalf("oversized write status=%d, want 413", resp.StatusCode) } } diff --git a/agent/internal/files/files_test.go b/agent/internal/files/files_test.go index 2057e08ba..e9514f4c7 100644 --- a/agent/internal/files/files_test.go +++ b/agent/internal/files/files_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "io" "mime/multipart" "net/http" @@ -927,6 +928,19 @@ func TestHttpErr_PermissionMappedTo403(t *testing.T) { } } +func TestHttpErr_BodyLimitMappedTo413(t *testing.T) { + for _, err := range []error{ + &http.MaxBytesError{Limit: maxWriteBytes}, + fmt.Errorf("write /private/data/file: %w", &http.MaxBytesError{Limit: maxWriteBytes}), + } { + rr := httptest.NewRecorder() + httpErr(rr, err) + if rr.Code != http.StatusRequestEntityTooLarge || rr.Body.String() != "request body too large\n" { + t.Fatalf("body limit response: status=%d body=%q", rr.Code, rr.Body.String()) + } + } +} + func TestHttpErr_GenericMappedTo500(t *testing.T) { rr := httptest.NewRecorder() httpErr(rr, errors.New("disk on fire")) 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 24bd7d56a..af1f57529 100644 --- a/agent/specs.md +++ b/agent/specs.md @@ -163,7 +163,8 @@ Resource usage env vars (set by the operator): | `/files/list` | GET | List directory entries; query param `path` (default `/`) | | `/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/write` | POST | Overwrite or create file; query param `path`; raw body capped at 64 MiB. An oversized request returns 413 without changing an existing file or leaving a new file. | +| `/files/create` | POST | Create only; query param `path`; raw body capped at 64 MiB. An oversized request returns 413 without publishing the file. 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 d3b3aa724..ba99ca06d 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..8be25bdcd --- /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/GameplanePanel/gameplane/api/internal/gatewayprotocol" + "github.com/GameplanePanel/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 := 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 { + 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 4212231e6..5a066f09f 100644 --- a/api/internal/ws/dialer.go +++ b/api/internal/ws/dialer.go @@ -66,6 +66,7 @@ func MountWithPlayerNames(r chi.Router, reg *kube.Registry, caBundle, clientCert 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 1c27d253e..d05e32f66 100644 --- a/web/specs.md +++ b/web/specs.md @@ -305,6 +305,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 @@ -1055,7 +1056,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 393dc7e04..a5edd458d 100644 --- a/web/src/lib/endpoints.ts +++ b/web/src/lib/endpoints.ts @@ -950,6 +950,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) => {