From 2b2a5991a4f736da8934704ce658f9c0eb3c5fc3 Mon Sep 17 00:00:00 2001 From: Magnus Jensen Date: Fri, 10 Jul 2026 18:31:17 +0200 Subject: [PATCH] handle client error decoding errors in ACME urls (#49137) **Related issue:** Resolves #46282 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Malformed ACME URLs and resource identifiers now return a clear **400 Bad Request** response instead of a **500 Internal Server Error**. * Error details were improved to more accurately distinguish malformed client requests. * **Tests** * Added an integration test covering invalid ACME endpoint path IDs across resource types, verifying **400** responses with the expected malformed error type. --- .../46282-handle-client-error-decoding-errors | 1 + .../acme/internal/service/endpoint_utils.go | 17 ++++++++--- .../acme/internal/tests/integration_test.go | 30 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 changes/46282-handle-client-error-decoding-errors 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))}