HMS-2714 fix description and title validation

[frasertweedale]

The validation regexes for the description and title fields are too
restrictive. Reasonable values are rejected.

Instead, use callback validators to check that the string values
(which are byte sequences) are valid UTF-8, and they do not contain
control characters (except the description field allows Carriage
Return, Line Feed, and Horizontal Tab).

ALSO

Currently input validation failure does NOT cause processing to
stop. If somehow the backend operation succeed, that can result in
a 400 Bad Request response, but the target backend operation
completed and database modifications potentially performed.
Also, the normal response data gets appended to the error message.

Update the validation middleware to stop processing when an input
validation error occurs, after setting the response status and body
content (error message). As a result, no further processing will be
performed.

[app-sre-bot]

Can one of the admins verify this patch?

[tiran]

Code looks good to me.

Please add some positive and negative test cases to validate_api_test.go.

[avisiedo]

I have checked the code by using the payload below for test/data/http/patch-rhel-idm-domain.json to force the failure on validation; See below the response is a plain text instead of json document; please see notes at the end.

---

The below is to replay for the request validation

{
    "title": "mypatched.example \u0010",
    "description": "My patched description for mydomain.example domain",
    "auto_enrollment_enabled": true
}

To force the validation failure; on this situation the response I got is:

# ./test/scripts/local-domains-token.sh
# # Set UUID and TOKEN accordin to the response above
# # Register the domain
# ./test/scripts/local-domains-register.sh "$TOKEN"
# # Patch the domain with the wrong payload above
# ./test/scripts/local-domains-patch.sh "$UUID"
HTTP/1.1 400 Bad Request
Content-Type: text/plain
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Rh-Insights-Request-Id: test_4795483c7630454cbc97
X-Xss-Protection: 1; mode=block
Date: Wed, 18 Oct 2023 08:42:38 GMT
Content-Length: 139

request body has an error: doesn't match schema #/components/schemas/UpdateDomainUserRequest: Error at "/title": invalid code point:

Note Content-Type: text/plain; I guess it is coming from here: https://github.com/podengo-project/idmsvc-backend/blob/main/internal/infrastructure/middleware/validate_api.go#L233

Note the response is not a json document, I guess is the line below: https://github.com/podengo-project/idmsvc-backend/blob/main/internal/infrastructure/middleware/validate_api.go#L234

---

To replay the response validation

• It needs to enable the validation in the code; thanks @cryptomilk
• The steps to replay, given a domain registered, we update directly the database register by

# make db-cli
# update domains set title=E'my domain\n';
# \q

# # Now we read the record by
# ./test/scripts/local-domain-read.sh "$UUID"

---

I suggest returning by something similar to the below:

return echo.NewHTTPError(http.StatusBadRequest, err.Error())

---

I am not sure about the same change at:

• https://github.com/podengo-project/idmsvc-backend/blob/main/internal/infrastructure/middleware/validate_api.go#L257
• https://github.com/podengo-project/idmsvc-backend/blob/main/internal/infrastructure/middleware/validate_api.go#L258

---

Echo framework will handle the error response by the default error handler (https://echo.labstack.com/docs/error-handling#default-http-error-handler).

[avisiedo]

Thanks for the PR!

An additional change in the comments; I am debugging the other change I am not 100% sure; and I agree with @tiran on adding unit tests for the callbacks.

I would add unit tests for the middleware in a new ticket as it could be more complex; happy to do some pairing to add them.

[cryptomilk]

There are actually two places which need fixing:

internal/infrastructure/middleware/validate_api.go | 45 ++++++++++------------
 1 file changed, 20 insertions(+), 25 deletions(-)
 
diff --git a/internal/infrastructure/middleware/validate_api.go b/internal/infrastructure/middleware/validate_api.go
index 60d61a2..87354c9 100644
--- a/internal/infrastructure/middleware/validate_api.go
+++ b/internal/infrastructure/middleware/validate_api.go
@@ -260,9 +260,7 @@ func RequestResponseValidatorWithConfig(config *RequestResponseValidatorConfig)
 					requestValidationInput,
 				)
 				if err != nil {
-					c.Response().Header().Set(echo.HeaderContentType, "text/plain")
-					c.String(http.StatusBadRequest, err.Error())
-					return nil // stop processing
+					return echo.NewHTTPError(http.StatusBadRequest, err.Error())
 				}
 			}
 
@@ -272,32 +270,29 @@ func RequestResponseValidatorWithConfig(config *RequestResponseValidatorConfig)
 				resRec := &ResponseRecorder{buffer: &bytes.Buffer{}, original: rw}
 				c.Response().Writer = resRec
 
-				defer func() {
-					responseValidationInput := &openapi3filter.ResponseValidationInput{
-						RequestValidationInput: requestValidationInput,
-						Status:                 resRec.status,
-						Header:                 resRec.Header(),
-					}
-					responseValidationInput.SetBodyBytes(resRec.buffer.Bytes())
-
-					if err = openapi3filter.ValidateResponse(
-						ctx,
-						responseValidationInput,
-					); err != nil {
-						// write error response
-						c.Response().Header().Set(echo.HeaderContentType, "text/plain")
-						c.String(http.StatusInternalServerError, err.Error())
-					} else {
-						// Write original response
-						rw.WriteHeader(resRec.status)
-						resRec.buffer.WriteTo(rw)
-					}
-				}()
-
 				defer func() {
 					// reset the response, using the original ResponseWriter
 					c.SetResponse(echo.NewResponse(rw, c.Echo()))
 				}()
+				err = next(c)
+				responseValidationInput := &openapi3filter.ResponseValidationInput{
+					RequestValidationInput: requestValidationInput,
+					Status:                 resRec.status,
+					Header:                 resRec.Header(),
+				}
+				responseValidationInput.SetBodyBytes(resRec.buffer.Bytes())
+
+				if err = openapi3filter.ValidateResponse(
+					ctx,
+					responseValidationInput,
+				); err != nil {
+					// write error response
+					return echo.NewHTTPError(http.StatusBadRequest, err.Error())
+				}
+				// Write original response
+				rw.WriteHeader(resRec.status)
+				resRec.buffer.WriteTo(rw)
+				return err
 			}
 
 			return next(c)

[tiran]

@cryptomilk Can you open a new PR with these fixes, please?

[frasertweedale]

Thanks for the reviews fellas. I'll update this PR by end of week.

[tiran]

Testing our new bot

[tiran]

LGTM, thanks for the tests.

[frasertweedale]

Thanks @avisiedo and @tiran - and @cryptomilk for filing follow-up PR #23