From 647538928018e332afab5f2dae7b7b9ea6107412 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 15 Feb 2026 20:52:19 -0800 Subject: [PATCH 1/2] test: add IDOR tests for resource deletion ownership verification Tests demonstrate that env vars, labels, volumes, and ports can be deleted via another app's URL path without ownership checks. All 4 tests fail, confirming the vulnerability described in #19. --- internal/handlers/handlers_test.go | 183 ++++++++++++++++++++++++++++- internal/server/routes.go | 70 +++++------ 2 files changed, 217 insertions(+), 36 deletions(-) diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 58266fb..7fff691 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strconv" "strings" "testing" "time" @@ -14,6 +15,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/fx" + "git.eeqj.de/sneak/upaas/internal/models" + "git.eeqj.de/sneak/upaas/internal/config" "git.eeqj.de/sneak/upaas/internal/database" "git.eeqj.de/sneak/upaas/internal/docker" @@ -426,7 +429,30 @@ func addChiURLParams( ) } -func TestHandleWebhookRejectsOversizedBody(t *testing.T) { +// createTestApp creates an app using the app service and returns it. +func createTestApp( + t *testing.T, + tc *testContext, + name string, +) *models.App { + t.Helper() + + createdApp, err := tc.appSvc.CreateApp( + context.Background(), + app.CreateAppInput{ + Name: name, + RepoURL: "git@example.com:user/" + name + ".git", + Branch: "main", + }, + ) + require.NoError(t, err) + + return createdApp +} + +// TestDeleteEnvVarOwnershipVerification tests that deleting an env var +// via another app's URL path returns 404 (IDOR prevention). +func TestDeleteEnvVarOwnershipVerification(t *testing.T) { t.Parallel() testCtx := setupTestHandlers(t) @@ -465,6 +491,161 @@ func TestHandleWebhookRejectsOversizedBody(t *testing.T) { // Should still return OK (payload is truncated and fails JSON parse, // but webhook service handles invalid JSON gracefully) assert.Equal(t, http.StatusOK, recorder.Code) + + app1 := createTestApp(t, testCtx, "envvar-owner-app") + app2 := createTestApp(t, testCtx, "envvar-other-app") + + // Create env var belonging to app1 + envVar := models.NewEnvVar(testCtx.database) + envVar.AppID = app1.ID + envVar.Key = "SECRET" + envVar.Value = "hunter2" + require.NoError(t, envVar.Save(context.Background())) + + // Try to delete app1's env var using app2's URL path + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+app2.ID+"/env/"+strconv.FormatInt(envVar.ID, 10)+"/delete", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": app2.ID, + "envID": strconv.FormatInt(envVar.ID, 10), + }) + recorder := httptest.NewRecorder() + + handler := testCtx.handlers.HandleEnvVarDelete() + handler.ServeHTTP(recorder, request) + + // Should return 404 because the env var doesn't belong to app2 + assert.Equal(t, http.StatusNotFound, recorder.Code) + + // Verify the env var was NOT deleted + found, err := models.FindEnvVar(context.Background(), testCtx.database, envVar.ID) + require.NoError(t, err) + assert.NotNil(t, found, "env var should still exist after IDOR attempt") +} + +// TestDeleteLabelOwnershipVerification tests that deleting a label +// via another app's URL path returns 404 (IDOR prevention). +func TestDeleteLabelOwnershipVerification(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + + app1 := createTestApp(t, testCtx, "label-owner-app") + app2 := createTestApp(t, testCtx, "label-other-app") + + // Create label belonging to app1 + label := models.NewLabel(testCtx.database) + label.AppID = app1.ID + label.Key = "traefik.enable" + label.Value = "true" + require.NoError(t, label.Save(context.Background())) + + // Try to delete app1's label using app2's URL path + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+app2.ID+"/labels/"+strconv.FormatInt(label.ID, 10)+"/delete", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": app2.ID, + "labelID": strconv.FormatInt(label.ID, 10), + }) + recorder := httptest.NewRecorder() + + handler := testCtx.handlers.HandleLabelDelete() + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusNotFound, recorder.Code) + + // Verify the label was NOT deleted + found, err := models.FindLabel(context.Background(), testCtx.database, label.ID) + require.NoError(t, err) + assert.NotNil(t, found, "label should still exist after IDOR attempt") +} + +// TestDeleteVolumeOwnershipVerification tests that deleting a volume +// via another app's URL path returns 404 (IDOR prevention). +func TestDeleteVolumeOwnershipVerification(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + + app1 := createTestApp(t, testCtx, "volume-owner-app") + app2 := createTestApp(t, testCtx, "volume-other-app") + + // Create volume belonging to app1 + volume := models.NewVolume(testCtx.database) + volume.AppID = app1.ID + volume.HostPath = "/data/app1" + volume.ContainerPath = "/app/data" + volume.ReadOnly = false + require.NoError(t, volume.Save(context.Background())) + + // Try to delete app1's volume using app2's URL path + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+app2.ID+"/volumes/"+strconv.FormatInt(volume.ID, 10)+"/delete", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": app2.ID, + "volumeID": strconv.FormatInt(volume.ID, 10), + }) + recorder := httptest.NewRecorder() + + handler := testCtx.handlers.HandleVolumeDelete() + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusNotFound, recorder.Code) + + // Verify the volume was NOT deleted + found, err := models.FindVolume(context.Background(), testCtx.database, volume.ID) + require.NoError(t, err) + assert.NotNil(t, found, "volume should still exist after IDOR attempt") +} + +// TestDeletePortOwnershipVerification tests that deleting a port +// via another app's URL path returns 404 (IDOR prevention). +func TestDeletePortOwnershipVerification(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + + app1 := createTestApp(t, testCtx, "port-owner-app") + app2 := createTestApp(t, testCtx, "port-other-app") + + // Create port belonging to app1 + port := models.NewPort(testCtx.database) + port.AppID = app1.ID + port.HostPort = 8080 + port.ContainerPort = 80 + port.Protocol = models.PortProtocolTCP + require.NoError(t, port.Save(context.Background())) + + // Try to delete app1's port using app2's URL path + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+app2.ID+"/ports/"+strconv.FormatInt(port.ID, 10)+"/delete", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": app2.ID, + "portID": strconv.FormatInt(port.ID, 10), + }) + recorder := httptest.NewRecorder() + + handler := testCtx.handlers.HandlePortDelete() + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusNotFound, recorder.Code) + + // Verify the port was NOT deleted + found, err := models.FindPort(context.Background(), testCtx.database, port.ID) + require.NoError(t, err) + assert.NotNil(t, found, "port should still exist after IDOR attempt") } func TestHandleWebhookReturns404ForUnknownSecret(t *testing.T) { diff --git a/internal/server/routes.go b/internal/server/routes.go index 860d0b9..f36339b 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -46,7 +46,7 @@ func (s *Server) SetupRoutes() { // Public routes r.Get("/login", s.handlers.HandleLoginGET()) - r.Post("/login", s.handlers.HandleLoginPOST()) + r.With(s.mw.LoginRateLimit()).Post("/login", s.handlers.HandleLoginPOST()) r.Get("/setup", s.handlers.HandleSetupGET()) r.Post("/setup", s.handlers.HandleSetupPOST()) @@ -54,46 +54,46 @@ func (s *Server) SetupRoutes() { r.Group(func(r chi.Router) { r.Use(s.mw.SessionAuth()) - // Dashboard - r.Get("/", s.handlers.HandleDashboard()) + // Dashboard + r.Get("/", s.handlers.HandleDashboard()) - // Logout - r.Post("/logout", s.handlers.HandleLogout()) + // Logout + r.Post("/logout", s.handlers.HandleLogout()) - // App routes - r.Get("/apps/new", s.handlers.HandleAppNew()) - r.Post("/apps", s.handlers.HandleAppCreate()) - r.Get("/apps/{id}", s.handlers.HandleAppDetail()) - r.Get("/apps/{id}/edit", s.handlers.HandleAppEdit()) - r.Post("/apps/{id}", s.handlers.HandleAppUpdate()) - r.Post("/apps/{id}/delete", s.handlers.HandleAppDelete()) - r.Post("/apps/{id}/deploy", s.handlers.HandleAppDeploy()) - r.Get("/apps/{id}/deployments", s.handlers.HandleAppDeployments()) - r.Get("/apps/{id}/deployments/{deploymentID}/logs", s.handlers.HandleDeploymentLogsAPI()) - r.Get("/apps/{id}/deployments/{deploymentID}/download", s.handlers.HandleDeploymentLogDownload()) - r.Get("/apps/{id}/logs", s.handlers.HandleAppLogs()) - r.Get("/apps/{id}/container-logs", s.handlers.HandleContainerLogsAPI()) - r.Get("/apps/{id}/status", s.handlers.HandleAppStatusAPI()) - r.Get("/apps/{id}/recent-deployments", s.handlers.HandleRecentDeploymentsAPI()) - r.Post("/apps/{id}/restart", s.handlers.HandleAppRestart()) - r.Post("/apps/{id}/stop", s.handlers.HandleAppStop()) - r.Post("/apps/{id}/start", s.handlers.HandleAppStart()) + // App routes + r.Get("/apps/new", s.handlers.HandleAppNew()) + r.Post("/apps", s.handlers.HandleAppCreate()) + r.Get("/apps/{id}", s.handlers.HandleAppDetail()) + r.Get("/apps/{id}/edit", s.handlers.HandleAppEdit()) + r.Post("/apps/{id}", s.handlers.HandleAppUpdate()) + r.Post("/apps/{id}/delete", s.handlers.HandleAppDelete()) + r.Post("/apps/{id}/deploy", s.handlers.HandleAppDeploy()) + r.Get("/apps/{id}/deployments", s.handlers.HandleAppDeployments()) + r.Get("/apps/{id}/deployments/{deploymentID}/logs", s.handlers.HandleDeploymentLogsAPI()) + r.Get("/apps/{id}/deployments/{deploymentID}/download", s.handlers.HandleDeploymentLogDownload()) + r.Get("/apps/{id}/logs", s.handlers.HandleAppLogs()) + r.Get("/apps/{id}/container-logs", s.handlers.HandleContainerLogsAPI()) + r.Get("/apps/{id}/status", s.handlers.HandleAppStatusAPI()) + r.Get("/apps/{id}/recent-deployments", s.handlers.HandleRecentDeploymentsAPI()) + r.Post("/apps/{id}/restart", s.handlers.HandleAppRestart()) + r.Post("/apps/{id}/stop", s.handlers.HandleAppStop()) + r.Post("/apps/{id}/start", s.handlers.HandleAppStart()) - // Environment variables - r.Post("/apps/{id}/env-vars", s.handlers.HandleEnvVarAdd()) - r.Post("/apps/{id}/env-vars/{varID}/delete", s.handlers.HandleEnvVarDelete()) + // Environment variables + r.Post("/apps/{id}/env-vars", s.handlers.HandleEnvVarAdd()) + r.Post("/apps/{id}/env-vars/{varID}/delete", s.handlers.HandleEnvVarDelete()) - // Labels - r.Post("/apps/{id}/labels", s.handlers.HandleLabelAdd()) - r.Post("/apps/{id}/labels/{labelID}/delete", s.handlers.HandleLabelDelete()) + // Labels + r.Post("/apps/{id}/labels", s.handlers.HandleLabelAdd()) + r.Post("/apps/{id}/labels/{labelID}/delete", s.handlers.HandleLabelDelete()) - // Volumes - r.Post("/apps/{id}/volumes", s.handlers.HandleVolumeAdd()) - r.Post("/apps/{id}/volumes/{volumeID}/delete", s.handlers.HandleVolumeDelete()) + // Volumes + r.Post("/apps/{id}/volumes", s.handlers.HandleVolumeAdd()) + r.Post("/apps/{id}/volumes/{volumeID}/delete", s.handlers.HandleVolumeDelete()) - // Ports - r.Post("/apps/{id}/ports", s.handlers.HandlePortAdd()) - r.Post("/apps/{id}/ports/{portID}/delete", s.handlers.HandlePortDelete()) + // Ports + r.Post("/apps/{id}/ports", s.handlers.HandlePortAdd()) + r.Post("/apps/{id}/ports/{portID}/delete", s.handlers.HandlePortDelete()) }) }) -- 2.45.2 From 867cdf01abaa0fc7e53bb3269352a818d4fc278a Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 15 Feb 2026 20:52:59 -0800 Subject: [PATCH 2/2] fix: add ownership verification on env var, label, volume, and port deletion Verify that the resource's AppID matches the URL path app ID before allowing deletion. Without this check, any authenticated user could delete resources belonging to any app by providing the target resource's ID in the URL regardless of the app ID in the path (IDOR vulnerability). Closes #19 --- internal/handlers/app.go | 8 ++-- internal/handlers/handlers_test.go | 10 ++++- internal/server/routes.go | 70 +++++++++++++++--------------- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/internal/handlers/app.go b/internal/handlers/app.go index a1fe09e..8a94b42 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -824,7 +824,7 @@ func (h *Handlers) HandleEnvVarDelete() http.HandlerFunc { } envVar, findErr := models.FindEnvVar(request.Context(), h.db, envVarID) - if findErr != nil || envVar == nil { + if findErr != nil || envVar == nil || envVar.AppID != appID { http.NotFound(writer, request) return @@ -871,7 +871,7 @@ func (h *Handlers) HandleLabelDelete() http.HandlerFunc { } label, findErr := models.FindLabel(request.Context(), h.db, labelID) - if findErr != nil || label == nil { + if findErr != nil || label == nil || label.AppID != appID { http.NotFound(writer, request) return @@ -949,7 +949,7 @@ func (h *Handlers) HandleVolumeDelete() http.HandlerFunc { } volume, findErr := models.FindVolume(request.Context(), h.db, volumeID) - if findErr != nil || volume == nil { + if findErr != nil || volume == nil || volume.AppID != appID { http.NotFound(writer, request) return @@ -1039,7 +1039,7 @@ func (h *Handlers) HandlePortDelete() http.HandlerFunc { } port, findErr := models.FindPort(request.Context(), h.db, portID) - if findErr != nil || port == nil { + if findErr != nil || port == nil || port.AppID != appID { http.NotFound(writer, request) return diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 7fff691..03de641 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -452,7 +452,7 @@ func createTestApp( // TestDeleteEnvVarOwnershipVerification tests that deleting an env var // via another app's URL path returns 404 (IDOR prevention). -func TestDeleteEnvVarOwnershipVerification(t *testing.T) { +func TestHandleWebhookRejectsOversizedBody(t *testing.T) { t.Parallel() testCtx := setupTestHandlers(t) @@ -491,6 +491,14 @@ func TestDeleteEnvVarOwnershipVerification(t *testing.T) { // Should still return OK (payload is truncated and fails JSON parse, // but webhook service handles invalid JSON gracefully) assert.Equal(t, http.StatusOK, recorder.Code) +} + +// TestDeleteEnvVarOwnershipVerification tests that deleting an env var +// via another app's URL path returns 404 (IDOR prevention). +func TestDeleteEnvVarOwnershipVerification(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) app1 := createTestApp(t, testCtx, "envvar-owner-app") app2 := createTestApp(t, testCtx, "envvar-other-app") diff --git a/internal/server/routes.go b/internal/server/routes.go index f36339b..860d0b9 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -46,7 +46,7 @@ func (s *Server) SetupRoutes() { // Public routes r.Get("/login", s.handlers.HandleLoginGET()) - r.With(s.mw.LoginRateLimit()).Post("/login", s.handlers.HandleLoginPOST()) + r.Post("/login", s.handlers.HandleLoginPOST()) r.Get("/setup", s.handlers.HandleSetupGET()) r.Post("/setup", s.handlers.HandleSetupPOST()) @@ -54,46 +54,46 @@ func (s *Server) SetupRoutes() { r.Group(func(r chi.Router) { r.Use(s.mw.SessionAuth()) - // Dashboard - r.Get("/", s.handlers.HandleDashboard()) + // Dashboard + r.Get("/", s.handlers.HandleDashboard()) - // Logout - r.Post("/logout", s.handlers.HandleLogout()) + // Logout + r.Post("/logout", s.handlers.HandleLogout()) - // App routes - r.Get("/apps/new", s.handlers.HandleAppNew()) - r.Post("/apps", s.handlers.HandleAppCreate()) - r.Get("/apps/{id}", s.handlers.HandleAppDetail()) - r.Get("/apps/{id}/edit", s.handlers.HandleAppEdit()) - r.Post("/apps/{id}", s.handlers.HandleAppUpdate()) - r.Post("/apps/{id}/delete", s.handlers.HandleAppDelete()) - r.Post("/apps/{id}/deploy", s.handlers.HandleAppDeploy()) - r.Get("/apps/{id}/deployments", s.handlers.HandleAppDeployments()) - r.Get("/apps/{id}/deployments/{deploymentID}/logs", s.handlers.HandleDeploymentLogsAPI()) - r.Get("/apps/{id}/deployments/{deploymentID}/download", s.handlers.HandleDeploymentLogDownload()) - r.Get("/apps/{id}/logs", s.handlers.HandleAppLogs()) - r.Get("/apps/{id}/container-logs", s.handlers.HandleContainerLogsAPI()) - r.Get("/apps/{id}/status", s.handlers.HandleAppStatusAPI()) - r.Get("/apps/{id}/recent-deployments", s.handlers.HandleRecentDeploymentsAPI()) - r.Post("/apps/{id}/restart", s.handlers.HandleAppRestart()) - r.Post("/apps/{id}/stop", s.handlers.HandleAppStop()) - r.Post("/apps/{id}/start", s.handlers.HandleAppStart()) + // App routes + r.Get("/apps/new", s.handlers.HandleAppNew()) + r.Post("/apps", s.handlers.HandleAppCreate()) + r.Get("/apps/{id}", s.handlers.HandleAppDetail()) + r.Get("/apps/{id}/edit", s.handlers.HandleAppEdit()) + r.Post("/apps/{id}", s.handlers.HandleAppUpdate()) + r.Post("/apps/{id}/delete", s.handlers.HandleAppDelete()) + r.Post("/apps/{id}/deploy", s.handlers.HandleAppDeploy()) + r.Get("/apps/{id}/deployments", s.handlers.HandleAppDeployments()) + r.Get("/apps/{id}/deployments/{deploymentID}/logs", s.handlers.HandleDeploymentLogsAPI()) + r.Get("/apps/{id}/deployments/{deploymentID}/download", s.handlers.HandleDeploymentLogDownload()) + r.Get("/apps/{id}/logs", s.handlers.HandleAppLogs()) + r.Get("/apps/{id}/container-logs", s.handlers.HandleContainerLogsAPI()) + r.Get("/apps/{id}/status", s.handlers.HandleAppStatusAPI()) + r.Get("/apps/{id}/recent-deployments", s.handlers.HandleRecentDeploymentsAPI()) + r.Post("/apps/{id}/restart", s.handlers.HandleAppRestart()) + r.Post("/apps/{id}/stop", s.handlers.HandleAppStop()) + r.Post("/apps/{id}/start", s.handlers.HandleAppStart()) - // Environment variables - r.Post("/apps/{id}/env-vars", s.handlers.HandleEnvVarAdd()) - r.Post("/apps/{id}/env-vars/{varID}/delete", s.handlers.HandleEnvVarDelete()) + // Environment variables + r.Post("/apps/{id}/env-vars", s.handlers.HandleEnvVarAdd()) + r.Post("/apps/{id}/env-vars/{varID}/delete", s.handlers.HandleEnvVarDelete()) - // Labels - r.Post("/apps/{id}/labels", s.handlers.HandleLabelAdd()) - r.Post("/apps/{id}/labels/{labelID}/delete", s.handlers.HandleLabelDelete()) + // Labels + r.Post("/apps/{id}/labels", s.handlers.HandleLabelAdd()) + r.Post("/apps/{id}/labels/{labelID}/delete", s.handlers.HandleLabelDelete()) - // Volumes - r.Post("/apps/{id}/volumes", s.handlers.HandleVolumeAdd()) - r.Post("/apps/{id}/volumes/{volumeID}/delete", s.handlers.HandleVolumeDelete()) + // Volumes + r.Post("/apps/{id}/volumes", s.handlers.HandleVolumeAdd()) + r.Post("/apps/{id}/volumes/{volumeID}/delete", s.handlers.HandleVolumeDelete()) - // Ports - r.Post("/apps/{id}/ports", s.handlers.HandlePortAdd()) - r.Post("/apps/{id}/ports/{portID}/delete", s.handlers.HandlePortDelete()) + // Ports + r.Post("/apps/{id}/ports", s.handlers.HandlePortAdd()) + r.Post("/apps/{id}/ports/{portID}/delete", s.handlers.HandlePortDelete()) }) }) -- 2.45.2