From ab7c43b88791554731ef9eecc0e84b2f2d7e73a0 Mon Sep 17 00:00:00 2001 From: user Date: Fri, 20 Feb 2026 05:33:07 -0800 Subject: [PATCH] fix: disable API v1 write methods (closes #112) Remove POST /apps, DELETE /apps/{id}, and POST /apps/{id}/deploy from the API v1 route group. These endpoints used cookie-based session auth without CSRF protection, creating a CSRF vulnerability. Read-only endpoints (GET /apps, GET /apps/{id}, GET /apps/{id}/deployments), login, and whoami are retained. Removed handlers: HandleAPICreateApp, HandleAPIDeleteApp, HandleAPITriggerDeploy, along with apiCreateRequest struct and validateCreateRequest function. Updated tests to use service layer directly for app creation in remaining read-only endpoint tests. --- internal/handlers/api.go | 150 ----------------------------- internal/handlers/api_test.go | 111 +++++---------------- internal/handlers/handlers_test.go | 28 ------ internal/server/routes.go | 3 - 4 files changed, 24 insertions(+), 268 deletions(-) diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 19101eb..2dae196 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -1,7 +1,6 @@ package handlers import ( - "context" "encoding/json" "net/http" "strconv" @@ -9,7 +8,6 @@ import ( "github.com/go-chi/chi/v5" "git.eeqj.de/sneak/upaas/internal/models" - "git.eeqj.de/sneak/upaas/internal/service/app" ) // apiAppResponse is the JSON representation of an app. @@ -176,121 +174,6 @@ func (h *Handlers) HandleAPIGetApp() http.HandlerFunc { } } -// apiCreateRequest is the JSON body for creating an app via the API. -type apiCreateRequest struct { - Name string `json:"name"` - RepoURL string `json:"repoUrl"` - Branch string `json:"branch"` - DockerfilePath string `json:"dockerfilePath"` - DockerNetwork string `json:"dockerNetwork"` - NtfyTopic string `json:"ntfyTopic"` - SlackWebhook string `json:"slackWebhook"` -} - -// validateCreateRequest validates the fields of an API create app request. -// Returns an error message string or empty string if valid. -func validateCreateRequest(req *apiCreateRequest) string { - if req.Name == "" || req.RepoURL == "" { - return "name and repo_url are required" - } - - nameErr := validateAppName(req.Name) - if nameErr != nil { - return "invalid app name: " + nameErr.Error() - } - - repoURLErr := validateRepoURL(req.RepoURL) - if repoURLErr != nil { - return "invalid repository URL: " + repoURLErr.Error() - } - - return "" -} - -// HandleAPICreateApp returns a handler that creates a new app. -func (h *Handlers) HandleAPICreateApp() http.HandlerFunc { - return func(writer http.ResponseWriter, request *http.Request) { - var req apiCreateRequest - - decodeErr := json.NewDecoder(request.Body).Decode(&req) - if decodeErr != nil { - h.respondJSON(writer, request, - map[string]string{"error": "invalid JSON body"}, - http.StatusBadRequest) - - return - } - - if errMsg := validateCreateRequest(&req); errMsg != "" { - h.respondJSON(writer, request, - map[string]string{"error": errMsg}, - http.StatusBadRequest) - - return - } - - createdApp, createErr := h.appService.CreateApp(request.Context(), app.CreateAppInput{ - Name: req.Name, - RepoURL: req.RepoURL, - Branch: req.Branch, - DockerfilePath: req.DockerfilePath, - DockerNetwork: req.DockerNetwork, - NtfyTopic: req.NtfyTopic, - SlackWebhook: req.SlackWebhook, - }) - if createErr != nil { - h.log.Error("api: failed to create app", "error", createErr) - h.respondJSON(writer, request, - map[string]string{"error": "failed to create app"}, - http.StatusInternalServerError) - - return - } - - h.respondJSON(writer, request, appToAPI(createdApp), http.StatusCreated) - } -} - -// HandleAPIDeleteApp returns a handler that deletes an app. -func (h *Handlers) HandleAPIDeleteApp() http.HandlerFunc { - return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - - application, err := h.appService.GetApp(request.Context(), appID) - if err != nil { - h.respondJSON(writer, request, - map[string]string{"error": "internal server error"}, - http.StatusInternalServerError) - - return - } - - if application == nil { - h.respondJSON(writer, request, - map[string]string{"error": "app not found"}, - http.StatusNotFound) - - return - } - - // Stop and remove the Docker container before deleting the DB record - h.cleanupContainer(request.Context(), appID, application.Name) - - deleteErr := h.appService.DeleteApp(request.Context(), application) - if deleteErr != nil { - h.log.Error("api: failed to delete app", "error", deleteErr) - h.respondJSON(writer, request, - map[string]string{"error": "failed to delete app"}, - http.StatusInternalServerError) - - return - } - - h.respondJSON(writer, request, - map[string]string{"status": "deleted"}, http.StatusOK) - } -} - // deploymentsPageLimit is the default number of deployments per page. const deploymentsPageLimit = 20 @@ -337,39 +220,6 @@ func (h *Handlers) HandleAPIListDeployments() http.HandlerFunc { } } -// HandleAPITriggerDeploy returns a handler that triggers a deployment for an app. -func (h *Handlers) HandleAPITriggerDeploy() http.HandlerFunc { - return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - - application, err := h.appService.GetApp(request.Context(), appID) - if err != nil || application == nil { - h.respondJSON(writer, request, - map[string]string{"error": "app not found"}, - http.StatusNotFound) - - return - } - - // Use a detached context so the deployment continues even if the - // HTTP client disconnects. - deployCtx := context.WithoutCancel(request.Context()) - - deployErr := h.deploy.Deploy(deployCtx, application, nil, true) - if deployErr != nil { - h.log.Error("api: failed to trigger deploy", "error", deployErr) - h.respondJSON(writer, request, - map[string]string{"error": deployErr.Error()}, - http.StatusConflict) - - return - } - - h.respondJSON(writer, request, - map[string]string{"status": "deploying"}, http.StatusAccepted) - } -} - // HandleAPIWhoAmI returns a handler that shows the current authenticated user. func (h *Handlers) HandleAPIWhoAmI() http.HandlerFunc { type whoAmIResponse struct { diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 6d7d899..f8f9cc0 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -10,6 +10,8 @@ import ( "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "git.eeqj.de/sneak/upaas/internal/service/app" ) // apiRouter builds a chi router with the API routes using session auth middleware. @@ -23,10 +25,7 @@ func apiRouter(tc *testContext) http.Handler { apiR.Use(tc.middleware.APISessionAuth()) apiR.Get("/whoami", tc.handlers.HandleAPIWhoAmI()) apiR.Get("/apps", tc.handlers.HandleAPIListApps()) - apiR.Post("/apps", tc.handlers.HandleAPICreateApp()) apiR.Get("/apps/{id}", tc.handlers.HandleAPIGetApp()) - apiR.Delete("/apps/{id}", tc.handlers.HandleAPIDeleteApp()) - apiR.Post("/apps/{id}/deploy", tc.handlers.HandleAPITriggerDeploy()) apiR.Get("/apps/{id}/deployments", tc.handlers.HandleAPIListDeployments()) }) }) @@ -62,23 +61,16 @@ func setupAPITest(t *testing.T) (*testContext, []*http.Cookie) { return tc, cookies } -// apiRequest makes an authenticated API request using session cookies. -func apiRequest( +// apiGet makes an authenticated GET request using session cookies. +func apiGet( t *testing.T, tc *testContext, cookies []*http.Cookie, - method, path string, - body string, + path string, ) *httptest.ResponseRecorder { t.Helper() - var req *http.Request - if body != "" { - req = httptest.NewRequest(method, path, strings.NewReader(body)) - req.Header.Set("Content-Type", "application/json") - } else { - req = httptest.NewRequest(method, path, nil) - } + req := httptest.NewRequest(http.MethodGet, path, nil) for _, c := range cookies { req.AddCookie(c) @@ -175,7 +167,7 @@ func TestAPIWhoAmI(t *testing.T) { tc, cookies := setupAPITest(t) - rr := apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/whoami", "") + rr := apiGet(t, tc, cookies, "/api/v1/whoami") assert.Equal(t, http.StatusOK, rr.Code) var resp map[string]any @@ -188,7 +180,7 @@ func TestAPIListAppsEmpty(t *testing.T) { tc, cookies := setupAPITest(t) - rr := apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/apps", "") + rr := apiGet(t, tc, cookies, "/api/v1/apps") assert.Equal(t, http.StatusOK, rr.Code) var apps []any @@ -196,52 +188,23 @@ func TestAPIListAppsEmpty(t *testing.T) { assert.Empty(t, apps) } -func TestAPICreateApp(t *testing.T) { - t.Parallel() - - tc, cookies := setupAPITest(t) - - body := `{"name":"test-app","repoUrl":"https://github.com/example/repo"}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - assert.Equal(t, http.StatusCreated, rr.Code) - - var app map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &app)) - assert.Equal(t, "test-app", app["name"]) - assert.Equal(t, "pending", app["status"]) -} - -func TestAPICreateAppValidation(t *testing.T) { - t.Parallel() - - tc, cookies := setupAPITest(t) - - body := `{"name":"","repoUrl":""}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - assert.Equal(t, http.StatusBadRequest, rr.Code) -} - func TestAPIGetApp(t *testing.T) { t.Parallel() tc, cookies := setupAPITest(t) - body := `{"name":"my-app","repoUrl":"https://github.com/example/repo"}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - require.Equal(t, http.StatusCreated, rr.Code) + created, err := tc.appSvc.CreateApp(t.Context(), app.CreateAppInput{ + Name: "my-app", + RepoURL: "https://github.com/example/repo", + }) + require.NoError(t, err) - var created map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &created)) - - appID, ok := created["id"].(string) - require.True(t, ok) - - rr = apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/apps/"+appID, "") + rr := apiGet(t, tc, cookies, "/api/v1/apps/"+created.ID) assert.Equal(t, http.StatusOK, rr.Code) - var app map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &app)) - assert.Equal(t, "my-app", app["name"]) + var resp map[string]any + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &resp)) + assert.Equal(t, "my-app", resp["name"]) } func TestAPIGetAppNotFound(t *testing.T) { @@ -249,29 +212,7 @@ func TestAPIGetAppNotFound(t *testing.T) { tc, cookies := setupAPITest(t) - rr := apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/apps/nonexistent", "") - assert.Equal(t, http.StatusNotFound, rr.Code) -} - -func TestAPIDeleteApp(t *testing.T) { - t.Parallel() - - tc, cookies := setupAPITest(t) - - body := `{"name":"delete-me","repoUrl":"https://github.com/example/repo"}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - require.Equal(t, http.StatusCreated, rr.Code) - - var created map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &created)) - - appID, ok := created["id"].(string) - require.True(t, ok) - - rr = apiRequest(t, tc, cookies, http.MethodDelete, "/api/v1/apps/"+appID, "") - assert.Equal(t, http.StatusOK, rr.Code) - - rr = apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/apps/"+appID, "") + rr := apiGet(t, tc, cookies, "/api/v1/apps/nonexistent") assert.Equal(t, http.StatusNotFound, rr.Code) } @@ -280,17 +221,13 @@ func TestAPIListDeployments(t *testing.T) { tc, cookies := setupAPITest(t) - body := `{"name":"deploy-app","repoUrl":"https://github.com/example/repo"}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - require.Equal(t, http.StatusCreated, rr.Code) + created, err := tc.appSvc.CreateApp(t.Context(), app.CreateAppInput{ + Name: "deploy-app", + RepoURL: "https://github.com/example/repo", + }) + require.NoError(t, err) - var created map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &created)) - - appID, ok := created["id"].(string) - require.True(t, ok) - - rr = apiRequest(t, tc, cookies, http.MethodGet, "/api/v1/apps/"+appID+"/deployments", "") + rr := apiGet(t, tc, cookies, "/api/v1/apps/"+created.ID+"/deployments") assert.Equal(t, http.StatusOK, rr.Code) var deployments []any diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 6a4e66e..49da78a 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -2,7 +2,6 @@ package handlers_test import ( "context" - "encoding/json" "net/http" "net/http/httptest" "net/url" @@ -843,33 +842,6 @@ func TestSetupRequiredExemptsHealthAndStaticAndAPI(t *testing.T) { }) } -// TestAPITriggerDeployUsesDetachedContext verifies that HandleAPITriggerDeploy -// does not pass the request context directly to the deploy operation. -// This is a compile-time/code-level fix verified by the deployment not being -// cancelled when the request context is cancelled. -func TestAPITriggerDeployUsesDetachedContext(t *testing.T) { - t.Parallel() - - // This test verifies the fix exists by checking the handler doesn't - // fail when called — the actual context detachment is verified by code review. - // The deploy will fail (no docker) but shouldn't panic. - tc, cookies := setupAPITest(t) - - body := `{"name":"detach-ctx-app","repoUrl":"https://github.com/example/repo"}` - rr := apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps", body) - require.Equal(t, http.StatusCreated, rr.Code) - - var created map[string]any - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &created)) - - appID, ok := created["id"].(string) - require.True(t, ok) - - rr = apiRequest(t, tc, cookies, http.MethodPost, "/api/v1/apps/"+appID+"/deploy", "") - // Should get conflict (deploy will fail) or accepted, but not panic - assert.Contains(t, []int{http.StatusAccepted, http.StatusConflict}, rr.Code) -} - func TestHandleCancelDeployRedirects(t *testing.T) { t.Parallel() diff --git a/internal/server/routes.go b/internal/server/routes.go index 3339bb8..e7dd7ab 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -114,10 +114,7 @@ func (s *Server) SetupRoutes() { r.Get("/whoami", s.handlers.HandleAPIWhoAmI()) r.Get("/apps", s.handlers.HandleAPIListApps()) - r.Post("/apps", s.handlers.HandleAPICreateApp()) r.Get("/apps/{id}", s.handlers.HandleAPIGetApp()) - r.Delete("/apps/{id}", s.handlers.HandleAPIDeleteApp()) - r.Post("/apps/{id}/deploy", s.handlers.HandleAPITriggerDeploy()) r.Get("/apps/{id}/deployments", s.handlers.HandleAPIListDeployments()) }) }) -- 2.45.2