[release-4.19] OCPBUGS-61662: Add API validation to reject # or whitespace in spec.path

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
 }
 

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 {