From d1820bac2ebba639b4dd1386d931a53f9f563146 Mon Sep 17 00:00:00 2001 From: Ali Syed Date: Mon, 28 Apr 2025 16:40:06 +0100 Subject: [PATCH] API validation:reject # or whitespace in spec.path This commit introduces api validation to reject # or whitespace in the spec.path. This is becaause it causes HaProxy error and the inress to become degraded. The validation will reject any new route that tries to create with whitespace or # in spec.path. It will allow for ratcheting. Test cases are also introduced in the validation_test.go file Related bug: https://issues.redhat.com/browse/OCPBUGS-47773 --- pkg/route/validation/validation.go | 24 +++- pkg/route/validation/validation_test.go | 148 +++++++++++++++++++++++- 2 files changed, 166 insertions(+), 6 deletions(-) diff --git a/pkg/route/validation/validation.go b/pkg/route/validation/validation.go index a3896006f9..55f34fe755 100644 --- a/pkg/route/validation/validation.go +++ b/pkg/route/validation/validation.go @@ -77,7 +77,11 @@ var ( ) func ValidateRoute(ctx context.Context, route *routev1.Route, sarCreator routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { - return validateRoute(ctx, route, true, sarCreator, secretsGetter, opts) + allErrs := field.ErrorList{} + pathFieldPath := field.NewPath("spec", "path") + allErrs = append(allErrs, validatePath(route.Spec.Path, pathFieldPath)...) + allErrs = append(allErrs, validateRoute(ctx, route, true, sarCreator, secretsGetter, opts)...) + return allErrs } // validLabels - used in the ValidateRouteUpdate function to check if "older" routes conform to DNS1123Labels or not @@ -215,11 +219,29 @@ func validateRoute(ctx context.Context, route *routev1.Route, checkHostname bool return result } +// validatePath tests the spec.path field for invalid syntax when the +// rewrite-target annotation is set. This addresses OCPBUGS-47773 by rejecting invalid spec.path +// values, while preserving compatibility for users who may have depended on the unintended +// behavior caused by the bug. +func validatePath(pathValue string, fldPath *field.Path) field.ErrorList { + result := field.ErrorList{} + + if strings.ContainsAny(pathValue, "# ") { + result = append(result, field.Invalid(fldPath, pathValue, "cannot contain # or spaces")) + } + + return result +} + func ValidateRouteUpdate(ctx context.Context, route *routev1.Route, older *routev1.Route, sarc routecommon.SubjectAccessReviewCreator, secrets corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { allErrs := validateObjectMetaUpdate(&route.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata")) allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(route.Spec.WildcardPolicy, older.Spec.WildcardPolicy, field.NewPath("spec", "wildcardPolicy"))...) hostnameUpdated := route.Spec.Host != older.Spec.Host allErrs = append(allErrs, validateRoute(ctx, route, hostnameUpdated && validLabels(older.Spec.Host), sarc, secrets, opts)...) + if route.Spec.Path != older.Spec.Path { + pathFieldPath := field.NewPath("spec", "path") + allErrs = append(allErrs, validatePath(route.Spec.Path, pathFieldPath)...) + } return allErrs } diff --git a/pkg/route/validation/validation_test.go b/pkg/route/validation/validation_test.go index d14edc2905..d2f05c3f1c 100644 --- a/pkg/route/validation/validation_test.go +++ b/pkg/route/validation/validation_test.go @@ -112,6 +112,18 @@ func TestValidateRoute(t *testing.T) { var headerNameXSS string = "X-XSS-Protection" invalidNumRequests := make([]routev1.RouteHTTPHeader, maxRequestHeaderList+1) invalidNumResponses := make([]routev1.RouteHTTPHeader, maxResponseHeaderList+1) + routeWithPath := func(path string) *routev1.Route { + return &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wildcardpolicy", + Namespace: "foo", + }, + Spec: routev1.RouteSpec{ + To: createRouteSpecTo("serviceName", "Service"), + Path: path, + }, + } + } tests := []struct { name string route *routev1.Route @@ -1007,13 +1019,34 @@ func TestValidateRoute(t *testing.T) { }, expectedErrors: 1, }, + { + name: "spec.path should not contain hash symbol in the spec.path", + route: routeWithPath("/new-path#"), + expectedErrors: 1, + }, + { + name: "spec.path should not contain whitespace in the spec.path", + route: routeWithPath("/new path"), + expectedErrors: 1, + }, + { + name: "spec.path should not contain whitespace or # in the spec.path", + route: routeWithPath("/invalid-path # "), + expectedErrors: 1, + }, + { + name: "spec.path is valid", //accepts a valid spec.path + route: routeWithPath("/valid-path"), + expectedErrors: 0, + }, } - for _, tc := range tests { - errs := ValidateRoute(context.Background(), tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{AllowExternalCertificates: false}) - if len(errs) != tc.expectedErrors { - t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateRoute(context.Background(), tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{AllowExternalCertificates: false}) + if len(errs) != tc.expectedErrors { + t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs) + } + }) } } @@ -2448,6 +2481,111 @@ func TestValidateRouteUpdate(t *testing.T) { }, // new route is invalid - do labels check expectedErrors: 1, }, + { + name: "Reject update to spec.path containing #", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/valid-path", // Initial valid path + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Path = "/path#invalid" // Introduce # + }, + expectedErrors: 1, + }, + { + name: "Reject update to spec.path containing whitespace", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/valid-path", + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Path = "/invalid path" // Introduce whitespace + }, + expectedErrors: 1, + }, + { + name: "Allow update without changing spec.path", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/valid-path", + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Host = "new.host.example.com" // Change unrelated field + }, + expectedErrors: 0, + }, + { + name: "Allow updates without modifying invalid spec.path", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/path#invalid", // Pre-existing invalid path + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Host = "new.host.example.com" + }, + expectedErrors: 0, + }, + { + name: "Reject update that modifies invalid spec.path to another invalid value", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/path with space", // Pre-existing invalid path + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Path = "/path#invalid" // Change to another invalid path + }, + expectedErrors: 1, + }, } for i, tc := range tests {