Fix audit findings: lock architecture, security, deployment hardening
Close all 14 findings from the codebase audit (1 high, 5 medium, 8 low): - Locking: replace the global API and workspace mutexes with per-instance and per-user keyed locks (new internal/api/locks.go) so a slow restore, upgrade or transfer no longer blocks unrelated users or instances; move Docker I/O out of the build queue and invalidate concurrent image-index observations with a generation counter; serialize instance deletion behind in-flight creation. - Security: equalize login timing for unknown usernames, periodically purge expired sessions/refresh tokens/lock rows, URL-encode orphan IDs in the frontend, upgrade vitest 3.x->5.x to clear 3 npm audit vulnerabilities (2 critical RCE). - Deployment: add a -healthcheck mode probing /healthz (used by Dockerfile HEALTHCHECK and compose), make ADMIN_PASSWORD optional in compose with removal guidance after first start, exclude test scripts from the image, and cap panel resources (cpus: 2, pids_limit: 512). - Reliability: check rows.Err() on admin listing loops, surface intent polling errors without sticking, disable close during in-flight create. Verified: go build/vet, full go test -race -count=1 ./..., frontend tsc build + 32 tests, npm audit 0, compose config, real-container healthcheck.
This commit is contained in:
@@ -55,11 +55,12 @@ func TestSlowDownloadDoesNotHoldLifecycleLock(t *testing.T) {
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("download not started")
|
||||
}
|
||||
if !a.mu.TryLock() {
|
||||
t.Fatal("slow network holds global lifecycle lock")
|
||||
if a.instanceLocks.held() {
|
||||
t.Fatal("slow network holds instance lifecycle lock")
|
||||
}
|
||||
a.readersMu.Lock()
|
||||
active := a.workspaceReaders["one"]
|
||||
a.mu.Unlock()
|
||||
a.readersMu.Unlock()
|
||||
if active != 1 {
|
||||
t.Fatalf("lease count %d", active)
|
||||
}
|
||||
@@ -174,14 +175,12 @@ func TestSlowUploadDoesNotHoldLifecycleLock(t *testing.T) {
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("upload not reading body")
|
||||
}
|
||||
if !a.mu.TryLock() {
|
||||
t.Fatal("slow upload holds global lock")
|
||||
if a.instanceLocks.held() {
|
||||
t.Fatal("slow upload holds instance lock")
|
||||
}
|
||||
a.mu.Unlock()
|
||||
if !workspaceMu.TryLock() {
|
||||
t.Fatal("slow upload holds workspace write lock")
|
||||
if a.userLocks.held() {
|
||||
t.Fatal("slow upload holds user write lock")
|
||||
}
|
||||
workspaceMu.Unlock()
|
||||
}
|
||||
|
||||
func TestWorkspaceErrorResponseReleasesLocks(t *testing.T) {
|
||||
@@ -204,16 +203,96 @@ func TestWorkspaceErrorResponseReleasesLocks(t *testing.T) {
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("response not started")
|
||||
}
|
||||
if !workspaceMu.TryLock() {
|
||||
t.Fatal("error response holds workspace lock")
|
||||
if a.userLocks.held() {
|
||||
t.Fatal("error response holds user write lock")
|
||||
}
|
||||
workspaceMu.Unlock()
|
||||
if !a.mu.TryLock() {
|
||||
t.Fatal("error response holds lifecycle lock")
|
||||
if a.instanceLocks.held() {
|
||||
t.Fatal("error response holds instance lock")
|
||||
}
|
||||
a.readersMu.Lock()
|
||||
leases := len(a.workspaceReaders)
|
||||
a.mu.Unlock()
|
||||
a.readersMu.Unlock()
|
||||
if leases != 0 {
|
||||
t.Fatal("response retains lease")
|
||||
}
|
||||
}
|
||||
|
||||
// A restore or upgrade holds its instance and user locks for minutes; that
|
||||
// must never block file operations or lifecycle actions of other users.
|
||||
func TestLongMutationDoesNotBlockOtherUsers(t *testing.T) {
|
||||
a, _ := runtimeTestAPI(t, func(w http.ResponseWriter, r *http.Request) {
|
||||
switch {
|
||||
case r.Method == "POST" && strings.HasSuffix(r.URL.Path, "/stop"):
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
case r.Method == "GET" && strings.HasSuffix(r.URL.Path, "/json"):
|
||||
writeJSON(w, map[string]any{"Id": "cid-two", "State": map[string]any{"Status": "exited", "Running": false}})
|
||||
default:
|
||||
t.Errorf("unexpected Docker call %s %s", r.Method, r.URL.Path)
|
||||
}
|
||||
})
|
||||
if err := a.s.CreateUser(store.User{ID: "other", Username: "other"}, "unused"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
otherSid, _, err := a.s.Session("other")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
registerTestAccess(t, a, otherSid)
|
||||
if err := a.s.AddInstance(store.Instance{ID: "one", UID: "owner"}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := a.s.AddInstance(store.Instance{ID: "two", UID: "other", ContainerID: "cid-two", Status: "running", Created: time.Now()}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
for _, dir := range []string{"workspaces/owner/instances/one/home", "workspaces/other/instances/two/home"} {
|
||||
if err := os.MkdirAll(filepath.Join(a.root, dir), 0750); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
// Simulate a mutation paused mid-flight (restore/upgrade state).
|
||||
inst := a.instanceLocks.Acquire("one")
|
||||
defer inst.Release()
|
||||
user := a.userLocks.Acquire("owner")
|
||||
defer user.Release()
|
||||
|
||||
treeDone := make(chan *httptest.ResponseRecorder, 1)
|
||||
go func() {
|
||||
w := httptest.NewRecorder()
|
||||
a.workspace(w, runtimeRequest("GET", "/api/ws/tree?instance=two", "", otherSid))
|
||||
treeDone <- w
|
||||
}()
|
||||
select {
|
||||
case w := <-treeDone:
|
||||
if w.Code != 200 {
|
||||
t.Fatalf("other user tree: %d %s", w.Code, w.Body.String())
|
||||
}
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("other user file operation blocked by long mutation")
|
||||
}
|
||||
stopDone := make(chan *httptest.ResponseRecorder, 1)
|
||||
go func() {
|
||||
w := httptest.NewRecorder()
|
||||
a.instance(w, runtimeRequest("POST", "/api/instances/two/stop", "", otherSid))
|
||||
stopDone <- w
|
||||
}()
|
||||
select {
|
||||
case w := <-stopDone:
|
||||
if w.Code != 200 {
|
||||
t.Fatalf("other instance stop: %d %s", w.Code, w.Body.String())
|
||||
}
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("other instance lifecycle blocked by long mutation")
|
||||
}
|
||||
// The same user's mutation is still serialized by the user lock.
|
||||
blocked := make(chan struct{})
|
||||
go func() {
|
||||
user := a.userLocks.Acquire("owner")
|
||||
user.Release()
|
||||
close(blocked)
|
||||
}()
|
||||
select {
|
||||
case <-blocked:
|
||||
t.Fatal("owner lock not held during mutation")
|
||||
case <-time.After(300 * time.Millisecond):
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user