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 {