diff --git a/changes/46282-handle-client-error-decoding-errors b/changes/46282-handle-client-error-decoding-errors new file mode 100644 index 0000000000..f6c44f6e68 --- /dev/null +++ b/changes/46282-handle-client-error-decoding-errors @@ -0,0 +1 @@ +- Fixed an issue where ACME urls would throw a 500 error on malformed URLs. \ No newline at end of file diff --git a/server/mdm/acme/internal/service/endpoint_utils.go b/server/mdm/acme/internal/service/endpoint_utils.go index 63f8f3a687..4701696941 100644 --- a/server/mdm/acme/internal/service/endpoint_utils.go +++ b/server/mdm/acme/internal/service/endpoint_utils.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/json" "errors" + "fmt" "io" "net/http" "net/url" @@ -13,6 +14,7 @@ import ( "github.com/fleetdm/fleet/v4/server/mdm/acme/api" "github.com/fleetdm/fleet/v4/server/mdm/acme/internal/types" eu "github.com/fleetdm/fleet/v4/server/platform/endpointer" + platform_errors "github.com/fleetdm/fleet/v4/server/platform/errors" platform_http "github.com/fleetdm/fleet/v4/server/platform/http" "github.com/go-kit/kit/endpoint" kithttp "github.com/go-kit/kit/transport/http" @@ -34,10 +36,17 @@ func encodeResponse(ctx context.Context, w http.ResponseWriter, response any) er func acmeErrorEncoder(ctx context.Context, err error, w http.ResponseWriter) { var acmeErr *types.ACMEError if !errors.As(err, &acmeErr) { - // TODO: If we can get access to a logger, we can log the details here, to help troubleshoot service errors. - // if it's not already an ACME error, it is because it is an internal server - // error (or a dev error, for 4xx we should always return ACMEError). - acmeErr = types.InternalServerError("") // not passing err.Error() as we don't want to leak internal details + + // Check if it's a client error, if so then return a MalformedError to avoid returning a 500. + var clientErr platform_errors.ErrWithIsClientError + if errors.As(err, &clientErr) && clientErr.IsClientError() { + acmeErr = types.MalformedError(fmt.Sprintf("The request was malformed: %s", clientErr.Error())) + } else { + // TODO: If we can get access to a logger, we can log the details here, to help troubleshoot service errors. + // if it's not a client error, it is because it is an internal server + // error (or a dev error, for 4xx we should always return ACMEError). + acmeErr = types.InternalServerError("") // not passing err.Error() as we don't want to leak internal details + } } w.Header().Set("Content-Type", "application/problem+json") diff --git a/server/mdm/acme/internal/tests/integration_test.go b/server/mdm/acme/internal/tests/integration_test.go index 8235313b1b..34b51d3ef4 100644 --- a/server/mdm/acme/internal/tests/integration_test.go +++ b/server/mdm/acme/internal/tests/integration_test.go @@ -30,6 +30,7 @@ func TestIntegration(t *testing.T) { {"GetAuthorization", testGetAuthorization}, {"FinalizeOrder", testFinalizeOrder}, {"DoChallengeDeviceAttestation", testDoChallengeDeviceAttestation}, + {"InvalidPathIDs", testInvalidPathIDs}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -1208,6 +1209,35 @@ func testListAccountOrders(t *testing.T, s *integrationTestSuite) { }) } +// testInvalidPathIDs exercises the account and order id decoding to return a malformed request error rather than internal server error. +func testInvalidPathIDs(t *testing.T, s *integrationTestSuite) { + // A valid enrollment is not required: the invalid ID fails to decode before + // the request ever reaches the service layer, so any path identifier works. + const pathID = "some-identifier" + + cases := []struct { + desc string + url string + }{ + {"order id", fmt.Sprintf("%s/api/mdm/acme/%s/orders/not-a-uint", s.server.URL, pathID)}, + {"account id", fmt.Sprintf("%s/api/mdm/acme/%s/accounts/not-a-uint/orders", s.server.URL, pathID)}, + {"certificate order id", fmt.Sprintf("%s/api/mdm/acme/%s/orders/not-a-uint/certificate", s.server.URL, pathID)}, + {"authorization id", fmt.Sprintf("%s/api/mdm/acme/%s/authorizations/not-a-uint", s.server.URL, pathID)}, + {"challenge id", fmt.Sprintf("%s/api/mdm/acme/%s/challenges/not-a-uint", s.server.URL, pathID)}, + {"finalize order id", fmt.Sprintf("%s/api/mdm/acme/%s/orders/not-a-uint/finalize", s.server.URL, pathID)}, + {"negative order id", fmt.Sprintf("%s/api/mdm/acme/%s/orders/-1", s.server.URL, pathID)}, + } + + for _, c := range cases { + t.Run(c.desc, func(t *testing.T) { + _, acmeErr, resp := doACMERequest[struct{}](t, http.MethodPost, c.url, []byte("{}")) + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.NotNil(t, acmeErr) + require.Contains(t, acmeErr.Type, "malformed") + }) + } +} + func testGetCertificate(t *testing.T, s *integrationTestSuite) { // create enrollments shared across sub-tests for error cases enrollRevoked := &types.Enrollment{Revoked: true, NotValidAfter: new(time.Now().Add(24 * time.Hour))}