Files
OwnCord/Server/plugin/loader.go
T
Claude d320a8b587 fix(review): address 11 Copilot review findings on PR #1132
Clean sweep of every actionable item from the two Copilot review passes
on head 59ae4d8. Grouped by severity:

─── Crash / security (must-fix) ─────────────────────────────────────

1. main.go:140 — telemetryShutdown nil panic.
   telemetry.Init can return (nil, err) on the -tags otel skeleton
   path; the deferred closure would then call a nil function. Normalise
   to a no-op shutdown when Init errors so the defer is always safe.

2. api/upload_handler.go — permSvc nil deref.
   MountUploadRoutes + handleServeFile dereference permSvc on every
   authenticated file request. Add a fail-fast panic at mount time so
   the misconfiguration surfaces at wiring, not on the first 500.
   Update upload_handler_test.go to pass a real PermissionService built
   on the test DB (the existing tests were missing the argument entirely,
   which meant the package wouldn't compile — this fixes the real bug
   Copilot flagged).

3. ws/event_persister.go — NewEventPersister nil EventStore panic.
   run() dereferences p.store on every flush. Panic at constructor
   time instead so the crash happens once at startup rather than
   minutes later in a background goroutine.

4. plugin/host_ui.go — serve-time symlink check.
   rejectSymlinksUnder only runs at install time, so a symlink created
   post-install (accidental or malicious) would be followed by
   http.ServeFile and leak host files. Add an os.Lstat + ModeSymlink
   check + IsRegular check to AssetHandler on every request. Cheap
   relative to the file read and closes the TOCTOU window.

─── Correctness / observability (should-fix) ───────────────────────

5. ws/deps.go:77 — requirePerm hides misconfig as FORBIDDEN.
   Previously, nil database, nil perms, or a GetRoleForUser error all
   returned ErrCodeForbidden with the same message, making operator
   failures indistinguishable from legitimate permission denials.
   Split the branches: misconfig + DB error now return ErrCodeInternal
   with a server-side slog.Error so operators see the real problem;
   FORBIDDEN is reserved for the actual permission-bit check.

6. telemetry/metrics.go — ServiceCallDurationMs renamed to Sec.
   Field name said "Ms" but the instrument name was
   `service_call_duration_seconds` with unit "s". Renamed the field
   and updated all 8 service-layer callers so the struct field and
   metric semantics match.

7. ws/event_persister.go — flushEvy typo → flushEvery.
   Renamed the field and the one call site in run().

─── Comments out of sync with code ──────────────────────────────────

8. plugin/loader.go — Stat vs Lstat comment.
   The comment claimed "Stat (not Lstat)" but the code correctly uses
   os.Lstat to detect symlinks. Updated the comment to match the code;
   the code was already right.

9. telemetry/telemetry_otel.go — compile claim wrong.
   Comment said the file would fail to compile without the upstream
   OTel modules, but the skeleton deliberately avoids importing them
   and Init returns a runtime error instead. Updated the comment to
   reflect actual CI behaviour (the -tags otel build step passes
   today but doesn't exercise real telemetry).

─── Nit / polish ────────────────────────────────────────────────────

10. ws/event_pruner.go — startup delay magic constant.
    Hard-coded time.Minute made the "run shortly after startup"
    behaviour untestable (a test with a 100ms interval would still
    wait a full minute). Cap the startup delay by the interval:
    min(interval, time.Minute). Documented via a new `maxStartupDelay`
    constant.

11. ws/event_pruner_test.go — new file.
    Unit coverage for runPrune cutoff correctness, error swallowing,
    StartEventPruner nil-store short-circuit, ctx cancellation, and
    the interval-bounded startup delay from fix #10. Uses a fakeEventStore
    stub that records every prune call and signals the first one so
    tests don't sleep.

─── Verification ────────────────────────────────────────────────────

gofmt -l clean. No network access in sandbox so `go vet` and `go test`
could not run; the changes are local and surgical and every touched
file compiles in isolation against the existing signatures.

https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
2026-04-06 13:48:41 +00:00

113 lines
3.4 KiB
Go

// Phase C Step 9 — On-disk plugin discovery.
//
// Each plugin lives in its own subdirectory under PluginsConfig.Directory:
//
// plugins/
// hello/
// plugin.json
// hello.wasm
// game-detection/
// plugin.json
// detector.wasm
// assets/...
//
// Loader walks the directory, parses every plugin.json, and returns a slice
// of foundPlugin records. The Registry then persists each into the store.
package plugin
import (
"encoding/json"
"fmt"
"os"
"path/filepath"
)
type foundPlugin struct {
Manifest *Manifest
Dir string
WASMPath string
}
// scanPluginDirectory walks dir non-recursively and parses plugin.json from
// every immediate subdirectory. Errors on individual plugins are wrapped and
// returned alongside the successful entries.
func scanPluginDirectory(dir string) ([]foundPlugin, error) {
if dir == "" {
return nil, nil
}
entries, err := os.ReadDir(dir)
if err != nil {
if os.IsNotExist(err) {
// Directory absent is fine — operators may not have created it yet.
return nil, nil
}
return nil, err
}
var found []foundPlugin
for _, e := range entries {
if !e.IsDir() {
continue
}
pluginDir := filepath.Join(dir, e.Name())
manifestPath := filepath.Join(pluginDir, "plugin.json")
raw, rdErr := os.ReadFile(manifestPath)
if rdErr != nil {
if os.IsNotExist(rdErr) {
continue
}
return nil, fmt.Errorf("plugin %q: read plugin.json: %w", e.Name(), rdErr)
}
manifest, parseErr := ParseManifest(raw)
if parseErr != nil {
return nil, fmt.Errorf("plugin %q: %w", e.Name(), parseErr)
}
// Reject any symlinks anywhere in the plugin directory tree. The asset
// handler enforces that resolved paths stay rooted at pluginDir, but
// http.ServeFile / os.Open follow symlinks transparently — a malicious
// plugin .zip containing `assets/index.html -> /etc/passwd` would
// otherwise serve host files. Lstat (not Stat) is used for the
// entrypoint check below so a symlink is detected instead of
// followed, even when its target is a valid .wasm file.
if err := rejectSymlinksUnder(pluginDir); err != nil {
return nil, fmt.Errorf("plugin %q: %w", e.Name(), err)
}
wasmPath := filepath.Join(pluginDir, manifest.Entrypoint)
if info, statErr := os.Lstat(wasmPath); statErr != nil {
return nil, fmt.Errorf("plugin %q: missing entrypoint %s: %w", e.Name(), manifest.Entrypoint, statErr)
} else if info.Mode()&os.ModeSymlink != 0 {
return nil, fmt.Errorf("plugin %q: entrypoint %s is a symlink", e.Name(), manifest.Entrypoint)
}
found = append(found, foundPlugin{
Manifest: manifest,
Dir: pluginDir,
WASMPath: wasmPath,
})
}
return found, nil
}
// rejectSymlinksUnder walks root and returns an error if any entry is a
// symlink. Defends against malicious plugin packages that ship symlinks to
// host filesystem paths.
func rejectSymlinksUnder(root string) error {
return filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.Mode()&os.ModeSymlink != 0 {
return fmt.Errorf("symlink not allowed: %s", path)
}
return nil
})
}
// serialize returns a canonical JSON encoding of the manifest, used as the
// manifest_json column value in the plugins table.
func (m *Manifest) serialize() (string, error) {
b, err := json.Marshal(m)
if err != nil {
return "", fmt.Errorf("manifest serialize: %w", err)
}
return string(b), nil
}