From ba90176907c87e5e8d1c5d54a599448788e19dc8 Mon Sep 17 00:00:00 2001 From: Yael Date: Thu, 28 Nov 2024 20:11:48 +0200 Subject: [PATCH] feat(backend): reject invalid HTTP path params in backend Signed-off-by: Yael --- workspaces/backend/api/errors.go | 1 - .../backend/api/workspacekinds_handler.go | 6 +- .../api/workspacekinds_handler_test.go | 109 +++++++++++ workspaces/backend/api/workspaces_handler.go | 28 +-- .../backend/api/workspaces_handler_test.go | 169 ++++++++++++++++++ .../backend/internal/helper/validation.go | 145 +++++++++++++++ 6 files changed, 444 insertions(+), 14 deletions(-) create mode 100644 workspaces/backend/internal/helper/validation.go diff --git a/workspaces/backend/api/errors.go b/workspaces/backend/api/errors.go index e8ec75c65..da17379fc 100644 --- a/workspaces/backend/api/errors.go +++ b/workspaces/backend/api/errors.go @@ -46,7 +46,6 @@ func (a *App) LogError(r *http.Request, err error) { a.logger.Error(err.Error(), "method", method, "uri", uri) } -//nolint:unused func (a *App) badRequestResponse(w http.ResponseWriter, r *http.Request, err error) { httpError := &HTTPError{ StatusCode: http.StatusBadRequest, diff --git a/workspaces/backend/api/workspacekinds_handler.go b/workspaces/backend/api/workspacekinds_handler.go index c51194408..064f9ef37 100644 --- a/workspaces/backend/api/workspacekinds_handler.go +++ b/workspaces/backend/api/workspacekinds_handler.go @@ -18,11 +18,11 @@ package api import ( "errors" - "fmt" "net/http" "github.com/julienschmidt/httprouter" + "github.com/kubeflow/notebooks/workspaces/backend/internal/helper" models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspacekinds" repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspacekinds" ) @@ -34,8 +34,8 @@ type WorkspaceKindEnvelope Envelope[models.WorkspaceKind] func (a *App) GetWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { name := ps.ByName("name") - if name == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspace kind name is missing")) + if err := helper.ValidateWorkspaceKind(name); err != nil { + a.badRequestResponse(w, r, err) return } diff --git a/workspaces/backend/api/workspacekinds_handler_test.go b/workspaces/backend/api/workspacekinds_handler_test.go index 8d19f2612..ff24a5077 100644 --- a/workspaces/backend/api/workspacekinds_handler_test.go +++ b/workspaces/backend/api/workspacekinds_handler_test.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand" "net/http" "net/http/httptest" "strings" @@ -267,4 +268,112 @@ var _ = Describe("WorkspaceKinds Handler", func() { Expect(rs.StatusCode).To(Equal(http.StatusNotFound)) }) }) + + Context("with unsupported request parameters", Ordered, func() { + + var ( + a App + validResourceName string + invalidResourceName string + validMaxLengthName string + invalidLengthName string + ) + + // generateResourceName generates a random string of the specified length and allowed chars. + generateResourceName := func(length int) string { + const allowedChars = "abcdefghijklmnopqrstuvwxyz0123456789-" + + var sb strings.Builder + for i := 0; i < length; i++ { + if i == 0 || i == length-1 { + sb.WriteByte(allowedChars[rand.Intn(len(allowedChars)-1)]) + } else { + sb.WriteByte(allowedChars[rand.Intn(len(allowedChars))]) + } + } + return sb.String() + } + + BeforeAll(func() { + validResourceName = "test" + invalidResourceName = validResourceName + string(rune(rand.Intn(0x10FFFF-128)+128)) + validMaxLengthName = generateResourceName(253) + invalidLengthName = generateResourceName(254) + + repos := repositories.NewRepositories(k8sClient) + a = App{ + Config: config.EnvConfig{ + Port: 4000, + }, + repositories: repos, + } + }) + + It("should return 400 status code for a invalid workspacekind name", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspaceKindsByNamePath, ":"+WorkspaceNamePathParam, invalidResourceName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspaceKindHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: WorkspaceNamePathParam, + Value: invalidResourceName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspaceKindHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + }) + + It("should return 400 status code for a workspace longer than 253", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspaceKindsByNamePath, ":"+WorkspaceNamePathParam, invalidLengthName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspaceKindHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: WorkspaceNamePathParam, + Value: invalidLengthName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspaceKindHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + + }) + + It("should return 200 status code for a workspace with a length of 253 characters", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspaceKindsByNamePath, ":"+WorkspaceNamePathParam, validMaxLengthName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspaceKindHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: WorkspaceNamePathParam, + Value: validMaxLengthName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspaceKindHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusNotFound), "Expected HTTP status 404 Not Found") + }) + }) }) diff --git a/workspaces/backend/api/workspaces_handler.go b/workspaces/backend/api/workspaces_handler.go index aa970f774..ad1e3c407 100644 --- a/workspaces/backend/api/workspaces_handler.go +++ b/workspaces/backend/api/workspaces_handler.go @@ -24,6 +24,7 @@ import ( "github.com/julienschmidt/httprouter" + "github.com/kubeflow/notebooks/workspaces/backend/internal/helper" models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspaces" repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces" ) @@ -38,12 +39,9 @@ func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps htt var workspace models.Workspace var err error - if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is nil")) - return - } - if workspaceName == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspaceName is nil")) + + if err := helper.ValidateWorkspace(namespace, workspaceName); err != nil { + a.badRequestResponse(w, r, err) return } @@ -71,6 +69,11 @@ func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps htt func (a *App) GetWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { namespace := ps.ByName(NamespacePathParam) + if err := helper.ValidateNamespace(namespace, false); err != nil { + a.badRequestResponse(w, r, err) + return + } + var workspaces []models.Workspace var err error if namespace == "" { @@ -96,8 +99,8 @@ func (a *App) GetWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { namespace := ps.ByName("namespace") - if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is missing")) + if err := helper.ValidateNamespace(namespace, true); err != nil { + a.badRequestResponse(w, r, err) return } @@ -107,6 +110,11 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps return } + if err := helper.ValidateWorkspace(workspaceModel.Namespace, workspaceModel.Name); err != nil { + a.badRequestResponse(w, r, err) + return + } + workspaceModel.Namespace = namespace createdWorkspace, err := a.repositories.Workspace.CreateWorkspace(r.Context(), workspaceModel) @@ -132,12 +140,12 @@ func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps workspaceName := ps.ByName("name") if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is missing")) + a.badRequestResponse(w, r, fmt.Errorf("namespace is missing")) return } if workspaceName == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspace name is missing")) + a.badRequestResponse(w, r, fmt.Errorf("workspace name is missing")) return } diff --git a/workspaces/backend/api/workspaces_handler_test.go b/workspaces/backend/api/workspaces_handler_test.go index 0f3679883..5848f0ad2 100644 --- a/workspaces/backend/api/workspaces_handler_test.go +++ b/workspaces/backend/api/workspaces_handler_test.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand" "net/http" "net/http/httptest" "strings" @@ -809,4 +810,172 @@ var _ = Describe("Workspaces Handler", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) + + Context("with unsupported request parameters", Ordered, func() { + + var ( + a App + validResourceName string + invalidResourceName string + validMaxLengthName string + invalidLengthName string + ) + + // generateResourceName generates a random string of the specified length and allowed chars. + generateResourceName := func(length int) string { + const allowedChars = "abcdefghijklmnopqrstuvwxyz0123456789-" + + var sb strings.Builder + for i := 0; i < length; i++ { + if i == 0 || i == length-1 { + sb.WriteByte(allowedChars[rand.Intn(len(allowedChars)-1)]) + } else { + sb.WriteByte(allowedChars[rand.Intn(len(allowedChars))]) + } + } + return sb.String() + } + + BeforeAll(func() { + validResourceName = "test" + invalidResourceName = validResourceName + string(rune(rand.Intn(0x10FFFF-128)+128)) + validMaxLengthName = generateResourceName(63) + invalidLengthName = generateResourceName(64) + + repos := repositories.NewRepositories(k8sClient) + a = App{ + Config: config.EnvConfig{ + Port: 4000, + }, + repositories: repos, + } + }) + + It("should return 400 status code for a invalid namespace", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, invalidResourceName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspacesHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: NamespacePathParam, + Value: invalidResourceName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspacesHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + }) + + It("should return 400 status code for a non-ascii payload params", func() { + By("creating the HTTP request") + workspaceModel := models.Workspace{ + Name: validResourceName, + Namespace: invalidResourceName, + } + + workspaceJSON, err := json.Marshal(workspaceModel) + Expect(err).NotTo(HaveOccurred(), "Failed to marshal WorkspaceModel to JSON") + path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, invalidResourceName, 1) + + req, err := http.NewRequest(http.MethodPost, path, strings.NewReader(string(workspaceJSON))) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + req.Header.Set("Content-Type", "application/json") + + rr := httptest.NewRecorder() + ps := httprouter.Params{ + httprouter.Param{ + Key: NamespacePathParam, + Value: "namespace", + }, + } + + a.CreateWorkspaceHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code for creation") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + }) + + It("should return 400 status code for a namespace longer than 63", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, invalidLengthName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspacesHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: NamespacePathParam, + Value: invalidLengthName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspacesHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + + }) + It("should return 200 status code for parameters with a length of 63 characters", func() { + By("creating the HTTP request") + path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, validMaxLengthName, 1) + req, err := http.NewRequest(http.MethodGet, path, http.NoBody) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + + By("executing GetWorkspacesHandler") + ps := httprouter.Params{ + httprouter.Param{ + Key: NamespacePathParam, + Value: validMaxLengthName, + }, + } + rr := httptest.NewRecorder() + a.GetWorkspacesHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code") + Expect(rs.StatusCode).To(Equal(http.StatusOK), "Expected HTTP status 200 OK") // is it supported behavior? returning OK with list of [] workspaces + }) + It("should return 400 status code for a namespace longer than 63", func() { + workspaceModel := models.Workspace{ + Name: validMaxLengthName, + Namespace: invalidLengthName, + } + + workspaceJSON, err := json.Marshal(workspaceModel) + Expect(err).NotTo(HaveOccurred(), "Failed to marshal WorkspaceModel to JSON") + path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, invalidLengthName, 1) + + req, err := http.NewRequest(http.MethodPost, path, strings.NewReader(string(workspaceJSON))) + Expect(err).NotTo(HaveOccurred(), "Failed to create HTTP request") + req.Header.Set("Content-Type", "application/json") + + rr := httptest.NewRecorder() + ps := httprouter.Params{ + httprouter.Param{ + Key: NamespacePathParam, + Value: "namespace", + }, + } + + a.CreateWorkspaceHandler(rr, req, ps) + rs := rr.Result() + defer rs.Body.Close() + + By("verifying the HTTP response status code for creation") + Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), "Expected HTTP status 400 Bad Request") + + }) + }) }) diff --git a/workspaces/backend/internal/helper/validation.go b/workspaces/backend/internal/helper/validation.go new file mode 100644 index 000000000..adf00e827 --- /dev/null +++ b/workspaces/backend/internal/helper/validation.go @@ -0,0 +1,145 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helper + +import ( + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" +) + +// ValidationError represents a field-specific validation error. +type ValidationError struct { + Field string + Message string +} + +// Field represents a field's value and its type for validation. +type Field struct { + Value string + Type string +} + +// Error generates an error message for a given validation error. +func Error(err *ValidationError) error { + return fmt.Errorf("request validation failed on %s: %s", err.Field, err.Message) +} + +// Validator defines the interface for field validation. +type Validator interface { + Validate(field *Field) error +} + +// NotNullValidator ensures the field value is not empty. +type NotNullValidator struct{} + +func (v *NotNullValidator) Validate(field *Field) error { + if field.Value == "" { + return Error(&ValidationError{ + Field: field.Type, + Message: fmt.Sprintf("%s cannot be empty", field.Type), + }) + } + return nil +} + +// DNSLabelValidator validates that the field value conforms to DNS label standards. +type DNSLabelValidator struct{} + +func (v *DNSLabelValidator) Validate(field *Field) error { + if errors := validation.IsDNS1123Label(field.Value); errors != nil { + return Error(&ValidationError{ + Field: field.Type, + Message: strings.Join(errors, "; "), + }) + } + return nil +} + +// DNSSubdomainValidator validates that the field value conforms to DNS subdomain standards. +type DNSSubdomainValidator struct{} + +func (v *DNSSubdomainValidator) Validate(field *Field) error { + if errors := validation.IsDNS1123Subdomain(field.Value); errors != nil { + return Error(&ValidationError{ + Field: field.Type, + Message: strings.Join(errors, "; "), + }) + } + return nil +} + +// ValidateWorkspace validates namespace and name of a workspace. +func ValidateWorkspace(namespace string, workspaceName string) error { + if err := ValidateNamespace(namespace, true); err != nil { + return err + } + + if err := ValidateWorkspaceName(workspaceName); err != nil { + return err + } + + return nil +} + +// ValidateNamespace validates the namespace field, ensuring it is not null (if required) +// and conforms to DNS label standards. +func ValidateNamespace(namespace string, required bool) error { + if !required && namespace == "" { + return nil + } + + field := Field{namespace, "namespace"} + validators := []Validator{ + &NotNullValidator{}, + &DNSLabelValidator{}, + } + return runValidators(&field, validators) +} + +// ValidateWorkspaceName validates the workspace name, ensuring it is not null +// and conforms to DNS label standards. +func ValidateWorkspaceName(workspaceName string) error { + field := Field{workspaceName, "workspace"} + validators := []Validator{ + &NotNullValidator{}, + &DNSLabelValidator{}, + } + return runValidators(&field, validators) +} + +// ValidateWorkspaceKind validates the workspace kind, ensuring it is not null +// and conforms to DNS subdomain standards. +func ValidateWorkspaceKind(param string) error { + field := Field{param, "workspacekind"} + validators := []Validator{ + &NotNullValidator{}, + &DNSSubdomainValidator{}, + } + return runValidators(&field, validators) +} + +// runValidators applies all validators to a given field. +func runValidators(field *Field, validators []Validator) error { + for _, validator := range validators { + if err := validator.Validate(field); err != nil { + return err + } + } + return nil +}