Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"username": "vscode"
},
"ghcr.io/devcontainers/features/go:1": {
"version": "1.26"
"version": "1.27.3"
},
"ghcr.io/devcontainers/features/node:1": {
"version": "24"
Expand Down
9 changes: 4 additions & 5 deletions .devcontainer/post-create.sh
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,11 @@ if ! command -v oras >/dev/null 2>&1; then
fi

# ---------- golangci-lint ----------
# v2 release binaries work with the in-container Go version. The
# .golangci.yml has been migrated from v1 to v2 schema (v2 introduces new
# linters and removes deprecated ones).
GOLANGCI_VERSION="v2.12.2"
# v2.14 includes the importer for Go 1.27.3's export-data version 5.
# Keep this exact version aligned with CI and lint-gate-verify.sh.
GOLANGCI_VERSION="v2.14.0"
GOLANGCI_BIN="/go/bin/golangci-lint"
if ! "$GOLANGCI_BIN" --version 2>/dev/null | grep -qE 'version v?2\.'; then
if ! "$GOLANGCI_BIN" --version 2>/dev/null | grep -qF "version ${GOLANGCI_VERSION#v} "; then
Comment thread
Baylem marked this conversation as resolved.
log "installing golangci-lint ${GOLANGCI_VERSION}"
GOBIN=/go/bin go install \
"github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_VERSION}"
Expand Down
4 changes: 2 additions & 2 deletions .github/actions/go-cache/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ inputs:
go-version:
description: Go version to setup
required: false
default: '1.26'
default: '1.27'

runs:
using: composite
Expand All @@ -20,7 +20,7 @@ runs:
id: go
with:
go-version: ${{ inputs.go-version }}
# Resolve '1.26' to the newest 1.26.x patch instead of the runner's
# Resolve '1.27' to the newest 1.27.x patch (at least 1.27.3) instead of the runner's
# preinstalled one, so stdlib fixes (spec 013 G1, govulncheck) land
# without a manual pin bump.
check-latest: true
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -828,21 +828,21 @@ jobs:
if: matrix.module == 'operator' || matrix.module == 'api'
uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
with:
version: v2.12.2
version: v2.14.0
working-directory: ${{ matrix.module }}
args: --build-tags=envtest
- name: lint (test/e2e - e2e build tags)
if: matrix.module == 'test/e2e'
uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
with:
version: v2.12.2
version: v2.14.0
working-directory: ${{ matrix.module }}
args: --build-tags=e2e
- name: lint (other modules - no build tags)
if: matrix.module != 'operator' && matrix.module != 'api' && matrix.module != 'test/e2e'
uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
with:
version: v2.12.2
version: v2.14.0
working-directory: ${{ matrix.module }}

# Merge gate for spec 022: the project default telemetry destination
Expand Down
50 changes: 48 additions & 2 deletions agent/internal/mods/manifest.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package mods

import (
"crypto/rand"
"encoding/json"
"errors"
"io/fs"
Expand All @@ -11,7 +12,8 @@ import (

// manifestName is the per-volume install ledger. The leading dot keeps it
// out of mod listings and out of reach of client-supplied names (safeName
// rejects dotfiles), so it needs no extra guarding. Each per-(version+loader)
// rejects dotfiles). Filesystem confinement still applies to ledger reads and
// replacements. Each per-(version+loader)
// mod volume carries its own manifest, so metadata follows the files across
// game-version switches.
const manifestName = ".gameplane-mods.json"
Expand Down Expand Up @@ -44,11 +46,15 @@ type manifest struct {
// degrades to an empty manifest — mods then list as unmanaged rather than
// breaking the endpoint; the next mutation rewrites a clean file.
func loadManifest(dir string) *manifest {
empty := &manifest{Version: 1, Mods: map[string]*ModMeta{}}
// Validate that dir doesn't escape expected bounds.
dirClean := filepath.Clean(dir)
manifestPath := filepath.Join(dirClean, manifestName)
data, err := os.ReadFile(manifestPath)
return decodeManifest(data, err)
}

func decodeManifest(data []byte, err error) *manifest {
empty := &manifest{Version: 1, Mods: map[string]*ModMeta{}}
if err != nil {
if !errors.Is(err, fs.ErrNotExist) {
slog.Warn("mod manifest read", "err", err)
Expand All @@ -67,6 +73,46 @@ func loadManifest(dir string) *manifest {
return &parsed
}

// updateManifestInRoot keeps DELETE's metadata on the same directory handle as
// the removal. Reopening h.dir here would let a concurrent directory replacement
// redirect this otherwise confined operation. Metadata remains best-effort.
func (h *handler) updateManifestInRoot(root *os.Root, fn func(map[string]*ModMeta)) {
h.mu.Lock()
defer h.mu.Unlock()
m := decodeManifest(root.ReadFile(manifestName))
fn(m.Mods)
for name := range m.Mods {
if _, err := root.Lstat(name); errors.Is(err, fs.ErrNotExist) {
delete(m.Mods, name)
}
}
if err := m.saveInRoot(root); err != nil {
slog.Warn("mod manifest write", "err", err)
}
}

func (m *manifest) saveInRoot(root *os.Root) error {
data, err := json.Marshal(m)
if err != nil {
return err
}
tmpName := ".manifest-" + rand.Text()
tmp, err := root.OpenFile(tmpName, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600)
if err != nil {
return err
}
defer func() { _ = root.Remove(tmpName) }()
_, writeErr := tmp.Write(data)
closeErr := tmp.Close()
if writeErr != nil {
return writeErr
}
if closeErr != nil {
return closeErr
}
return root.Rename(tmpName, manifestName)
}

// save writes the manifest atomically (dot-prefixed temp + rename, the same
// pattern as downloads) so a crash never leaves a torn file behind.
func (m *manifest) save(dir string) error {
Expand Down
43 changes: 14 additions & 29 deletions agent/internal/mods/mods.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const (
)

type handler struct {
dataRoot string // trusted data-volume root; removal opens descendants relative to it
dir string // absolute mods directory, "" when unconfigured
exts []string // lowercased extension allowlist ("" = any)
allowed []string // download host allowlist; nil disables installs
Expand Down Expand Up @@ -96,6 +97,7 @@ func newHandler(dataRoot string, spec *caps.Mods) *handler {
return h
}
h.dir = dir
h.dataRoot = root
h.extract = spec.Extract
for _, e := range spec.Extensions {
h.exts = append(h.exts, strings.ToLower(e))
Expand Down Expand Up @@ -455,23 +457,12 @@ func (h *handler) upload(w http.ResponseWriter, req *http.Request) {
// removeEntry deletes an installed mod by name — the whole folder for
// extract loaders, a single file otherwise.
func (h *handler) removeEntry(name string) error {
// Use ConfinePath to validate and resolve the target within h.dir.
// ConfinePath consolidates validation and symlink resolution in one function,
// making the guard legible to CodeQL taint analysis.
target, err := ConfinePath(h.dir, name)
root, err := h.openModsRoot()
if err != nil {
return fmt.Errorf("path escape attempt: %w", err)
}
// Re-check inline so the guard is visible at the point of use.
rootClean := filepath.Clean(h.dir)
target = filepath.Clean(target)
if target != rootClean && !strings.HasPrefix(target, rootClean+string(os.PathSeparator)) {
return errors.New("path escape attempt")
}
if h.extract {
return os.RemoveAll(target)
return err
}
return os.Remove(target)
defer func() { _ = root.Close() }()
return removeModEntry(root, name, h.extract)
}

// downloadTemp streams rawURL into a temp file under the mods dir, enforcing
Expand Down Expand Up @@ -738,31 +729,25 @@ func (h *handler) remove(w http.ResponseWriter, req *http.Request) {
httpjson.Error(w, http.StatusBadRequest, err.Error())
return
}
// Use ConfinePath to validate and resolve the target within h.dir.
// ConfinePath consolidates validation and symlink resolution in one function,
// making the guard legible to CodeQL taint analysis.
target, err := ConfinePath(h.dir, name)
if err != nil {
httpjson.Error(w, http.StatusBadRequest, err.Error())
return
root, err := h.openModsRoot()
if err == nil {
defer func() { _ = root.Close() }()
err = removeModEntry(root, name, h.extract)
}
// Re-check inline so the guard is visible at the point of use.
rootClean := filepath.Clean(h.dir)
target = filepath.Clean(target)
if target != rootClean && !strings.HasPrefix(target, rootClean+string(os.PathSeparator)) {
if errors.Is(err, ErrEscapesRoot) {
httpjson.Error(w, http.StatusBadRequest, "path escape attempt")
return
}
if _, statErr := os.Stat(target); errors.Is(statErr, os.ErrNotExist) {
if errors.Is(err, fs.ErrNotExist) {
httpjson.Error(w, http.StatusNotFound, "no such mod")
return
}
if err := h.removeEntry(name); err != nil {
if err != nil {
slog.Warn("mod remove", "err", err)
httpjson.Error(w, http.StatusInternalServerError, "could not remove mod")
return
}
h.updateManifest(func(mods map[string]*ModMeta) {
h.updateManifestInRoot(root, func(mods map[string]*ModMeta) {
delete(mods, name)
})
w.WriteHeader(http.StatusNoContent)
Expand Down
10 changes: 5 additions & 5 deletions agent/internal/mods/mods_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -689,8 +689,8 @@ func TestInstall_ExtractBadArchive(t *testing.T) {

// --- Tests for ConfinePath-based refactors (T006) ---

// TestRemoveEntry_ConfinePath verifies that removeEntry uses ConfinePath
// to validate the target and rejects path traversal attempts.
// TestRemoveEntry_ConfinePath preserves the original valid-name and traversal
// contract now enforced by descriptor-relative removal.
func TestRemoveEntry_ConfinePath(t *testing.T) {
root := t.TempDir()
modsDir := filepath.Join(root, "mods")
Expand Down Expand Up @@ -786,8 +786,8 @@ func TestSwapInArchive_ConfinePath(t *testing.T) {
}
}

// TestRemoveWithConfinePath verifies that the remove handler uses ConfinePath
// to validate the name parameter and rejects traversal attempts.
// TestRemoveWithConfinePath preserves the HTTP name-validation contract when
// deletion is performed through the opened mods root.
func TestRemoveWithConfinePath(t *testing.T) {
root := t.TempDir()
if err := os.MkdirAll(filepath.Join(root, "mods"), 0o755); err != nil {
Expand All @@ -808,7 +808,7 @@ func TestRemoveWithConfinePath(t *testing.T) {
t.Fatal("file should be gone")
}

// remove should reject traversal attempts via ConfinePath validation.
// Rooted removal must still reject the original traversal cases.
for _, bad := range []string{"../escape", "a/b", ".."} {
status, _ := do(t, srv, http.MethodDelete, "/mods?name="+bad, nil)
if status != http.StatusBadRequest {
Expand Down
75 changes: 75 additions & 0 deletions agent/internal/mods/removal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package mods

import (
"fmt"
"os"
"path/filepath"
"strings"
)

// openModsRoot anchors removal to the configured data volume. Open each mods
// directory component without accepting symlink aliases, then retain its handle
// so a later rename cannot redirect removal or its manifest update.
func (h *handler) openModsRoot() (*os.Root, error) {
rel, err := filepath.Rel(h.dataRoot, h.dir)
if err != nil || !filepath.IsLocal(rel) {
return nil, ErrEscapesRoot
}
root, err := os.OpenRoot(h.dataRoot)
if err != nil {
return nil, fmt.Errorf("open mod data root: %w", err)
}
if rel == "." {
return root, nil
}
for _, component := range strings.Split(rel, string(os.PathSeparator)) {
next, err := openModDirectory(root, component)
_ = root.Close()
if err != nil {
return nil, err
}
root = next
}
return root, nil
}

func openModDirectory(parent *os.Root, name string) (*os.Root, error) {
info, err := parent.Lstat(name)
if err != nil {
return nil, fmt.Errorf("inspect mod directory: %w", err)
}
if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
return nil, ErrEscapesRoot
}
root, err := parent.OpenRoot(name)
if err != nil {
return nil, fmt.Errorf("open mod directory: %w", err)
}
opened, err := root.Stat(".")
if err != nil || !os.SameFile(info, opened) {
_ = root.Close()
return nil, ErrEscapesRoot
}
return root, nil
}

// removeModEntry never resolves the final name to a symlink's referent. Existing
// links are rejected; a link swapped in after Lstat is only unlinked by Remove
// or RemoveAll. Recursive removal likewise never follows child symlinks.
func removeModEntry(root *os.Root, name string, recursive bool) error {
name, err := safeName(name)
if err != nil {
return fmt.Errorf("invalid mod name: %w", err)
}
info, err := root.Lstat(name)
if err != nil {
return fmt.Errorf("inspect mod entry: %w", err)
}
if info.Mode()&os.ModeSymlink != 0 {
return ErrEscapesRoot
}
if recursive {
return root.RemoveAll(name)
}
return root.Remove(name)
}
Loading
Loading