Skip to content

fix(core): generic 500 body on the no-/failed-OnError fallbacks (no E.Message leak) - #631

Open
freitasjca wants to merge 1 commit into
HashLoad:masterfrom
freitasjca:fix/core-onerror-message-leak
Open

freitasjca wants to merge 1 commit into
HashLoad:masterfrom
freitasjca:fix/core-onerror-message-leak

Conversation

@freitasjca

Copy link
Copy Markdown
Contributor

fix(core): generic 500 body on the no-OnError and failed-OnError fallbacks (no E.Message leak)

The problem

When an unhandled exception reaches Horse's core error handling and the
application has registered no usable THorse.OnError, the raw exception text is
sent to the client. Two sibling sites do this:

1. The router's no-OnError fallback — when the exception escapes the
middleware/onRequest chain and no OnError is registered:

FResponse.Send('Internal Application Error: ' + E.Message)
  .Status(THTTPStatus.InternalServerError);

2. The ExecuteOnError wrapper — when an OnError handler is registered but
it itself raises (Horse.Core.pas):

AResponse.Send('Internal Application Error in OnError: ' + E.Message)
  .Status(THTTPStatus.InternalServerError);

E.Message for an arbitrary Exception routinely carries internal detail a
remote, possibly unauthenticated, client should never see — database error
text, file-system paths, connection strings, stack-relevant state. Returning it
verbatim in the 500 body is an information-disclosure weakness (CWE-209,
"Generation of Error Message Containing Sensitive Information").

Site 1 is in Horse core routing, so it is reached by every provider (Indy,
HttpSys, IOCP, Epoll, Console, Daemon, VCL, and the external CrossSocket /
mORMot / nghttp2 / ICS providers) — they all funnel unhandled requests through
THorseRouterTree.Execute / THorseRouterRadix.Execute. A provider that
installs its own hardened 500 handler does not help: the router catches the
exception and returns normally, so the provider's except block never runs and
this fallback is what the client gets. Site 2 is shared by all of them too.

The five occurrences — the router fallback is one per router × one per compiler
branch, plus the single wrapper site:

Unit Line (master) Path
Horse.Core.RouterTree.pas 190 TRouterTreeExecutor.Run (FPC branch of Execute)
Horse.Core.RouterTree.pas 465 THorseRouterTree.Execute (Delphi branch)
Horse.Core.Router.Radix.pas 215 TRadixExecutor.Run (FPC branch)
Horse.Core.Router.Radix.pas 992 THorseRouterRadix.Execute (Delphi branch)
Horse.Core.pas 1449 THorseCore.ExecuteOnError (OnError handler raised)

Fix

Each send drops ': ' + E.Message and emits only the generic phrase, preserving
the existing status and the leading text clients may already match on:

FResponse.Send('Internal Application Error').Status(THTTPStatus.InternalServerError);
// and, in the wrapper:
AResponse.Send('Internal Application Error in OnError').Status(THTTPStatus.InternalServerError);

The change is a string-literal edit plus a comment at each site — identical on
Delphi and FPC, no uses, no new types, no new constant unit. The other arms of
each on E: Exception do block are untouched:

  • EHorseCallbackInterrupted → re-raised (or re-raised only when no OnError, in
    the Delphi branches) — unchanged.
  • EHorseException → still returns EHorseException.ToJSON with its own
    status. This message is author-chosen (built with .Error(text) by the
    application), so it is intentional output and stays as-is.
  • THorse.HasOnError → still delegates to THorse.ExecuteOnError(Req, Res, E);
    only the message in that method's own crash-fallback changes.

How to keep the exception detail (no capability is lost)

THorse.OnError is the designed extension point and it receives the full E.
An application that wants the message in the body, or wants it logged
server-side, registers a handler:

THorse.OnError(
  procedure(Req: THorseRequest; Res: THorseResponse; E: Exception)
  begin
    Writeln(ErrOutput, 'Unhandled: ' + E.Message);           // server-side log
    Res.Send('Internal Application Error').Status(500);       // generic to client
  end);

This PR deliberately does not add Writeln/console I/O to core: core runs
inside console, VCL, service, daemon and FPC hosts, and an unconditional write
to a detached ErrOutput can itself raise. Server-side logging is the
application's call, via OnError.

Behaviour change — note for review

This changes a documented, tested default. Upstream has tests asserting the
old bodies in four suites — Tests.Integration.ErrorHandling,
Tests.Horse.Core.RouterTree, Tests.Horse.Core.Router.Radix and
Tests.Api.Console — all updated here. The 500 status is unchanged; only the
body text changes, and only on the no-OnError / failed-OnError paths.
Applications relying on the exception text reaching the client must now register
(a non-crashing) THorse.OnError.

Tests

Every path is covered by existing unit/integration tests that asserted the leaky
body; each is updated to assert the generic body and gains an explicit
no-leak assertion so the leak can never silently return:

Assert.AreEqual('Internal Application Error', FResponse.BodyText);
Assert.IsFalse(FResponse.BodyText.Contains('route error'),
  'no-OnError fallback must not leak E.Message (CWE-209)');

Updated assertions:

  • Tests.Horse.Core.RouterTree — generic fallback + rethrow path, and FallbackFlushesCookies.
  • Tests.Horse.Core.Router.Radix — the identical pair.
  • Tests.Integration.ErrorHandling.TestGenericExceptionReturnsHTTP500 — end-to-end wire body.
  • Tests.Api.Console.TestOnErrorDefault500 — no-OnError path over a live server.
  • Tests.Api.Console.TestOnErrorCallbackCrash — the failed-OnError wrapper path.

The EHorseException and healthy custom-OnError paths are unchanged.

Negative control — observed, not just intended (Delphi 10.4 Sydney, Default
and Default+Radix, 2026-11-04):
with the router src/ sends fixed but the unit
assertions still on the old string, exactly the tests that exercise the fixed
paths failed, each with the fix's fingerprint — proof the change reaches both
routers, the generic fallback, the rethrow path and the cookie-flush fallback:

Tests.Horse.Core.RouterTree.ExceptionPropagation.GenericFallback   Expected [Internal Application Error: route error] but got [Internal Application Error]
Tests.Horse.Core.RouterTree.ExceptionPropagation.GenericRethrow    Expected [Internal Application Error: route error] but got [Internal Application Error]
Tests.Horse.Core.RouterTree.FallbackFlushesCookies                 Expected [Internal Application Error: cookie error] but got [Internal Application Error]
Tests.Horse.Core.Router.Radix.ExceptionPropagation.GenericFallback (same)
Tests.Horse.Core.Router.Radix.ExceptionPropagation.GenericRethrow  (same)
Tests.Horse.Core.Router.Radix.FallbackFlushesCookies               (same)
Tests.Api.Console.TestOnErrorDefault500                            Contains(...) now False

Tests.Integration.ErrorHandling.TestGenericExceptionReturnsHTTP500 (updated in
the same pass) passed, confirming the end-to-end wire body is now
Internal Application Error. The Horse.Core.pas wrapper fix +
TestOnErrorCallbackCrash were added after this run.

Full green (2026-11-04), with all updated files applied: tests\run_delphi_tests.ps1
on Delphi 10.4 Sydney and Delphi 12 Athens, all six provider configs each
(Default, Default+Radix, HttpSys, HttpSys+Radix, IOCP, IOCP+Radix) — 12/12
SUCESSO, 0 failures, 0 errors
(per-config totals 335/334/339 reflect
provider-specific test registration). This exercises the fix on every provider
path on both compilers. (The two earlier Tests.Integration.Telemetry.*
failures were an unrelated port-9091 squat on the test host, resolved by freeing
the port; they are not related to this change.)

FPC: the standalone tests/fpc-exception-propagation/ExceptionPropagationCheck.dpr
compiles all three changed core units under FPC and exercises this exact
fallback; it asserted the old leaky body too and is updated here (generic body +
Pos('route error', …) = 0). Run tests/fpc-exception-propagation/run-exception-propagation-test.sh
(FPC 3.2.2); expect PASS lines, exit 0, no unfreed-memory report.

Files changed

  • src/Horse.Core.pas — ExecuteOnError crash-fallback (master line 1449).
  • src/Horse.Core.RouterTree.pas — two fallback sends (master lines 190, 465).
  • src/Horse.Core.Router.Radix.pas — two fallback sends (master lines 215, 992).
  • tests/src/tests/Tests.Horse.Core.RouterTree.pas — generic body + no-leak assertion (route + cookie).
  • tests/src/tests/Tests.Horse.Core.Router.Radix.pas — same pair.
  • tests/src/tests/Tests.Integration.ErrorHandling.pas — TestGenericExceptionReturnsHTTP500.
  • tests/src/tests/Tests.Api.Console.pas — TestOnErrorDefault500 + TestOnErrorCallbackCrash.
  • tests/fpc-exception-propagation/ExceptionPropagationCheck.dpr — FPC default-fallback body assertion.

Staged under patches/horse/ (CRLF, no BOM — matches the live 3.3.16 files;
CRLF-normalised diff is only the lines above).

….Message leak)

When an unhandled exception reaches Horse's core error handling and no usable
THorse.OnError is registered, the raw exception text was sent to the client.
Two sibling sites did this:
 - the router no-OnError fallback: 'Internal Application Error: ' + E.Message
   (RouterTree + Radix, Delphi + FPC branches); and
 - ExecuteOnError, when a registered OnError handler itself raises:
   'Internal Application Error in OnError: ' + E.Message (Horse.Core.pas).

E.Message for an arbitrary Exception can carry internal detail (DB errors,
paths, state), so returning it verbatim is an information-disclosure weakness
(CWE-209). The router site is in core, so it reaches every provider: the router
catches the exception and returns normally, bypassing each provider's own
hardened 500 handler and running this leaky fallback instead.

Drop ': ' + E.Message at all five sites; send the generic body with the same
500. The EHorseException and healthy-OnError arms are unchanged: an app that
wants the detail (in the body or a server-side log) registers THorse.OnError,
which receives the full E. No console I/O is added to core.

Updated every suite that codified the leaky body to assert the generic body
plus an explicit no-leak check: Tests.Horse.Core.RouterTree,
Tests.Horse.Core.Router.Radix, Tests.Integration.ErrorHandling and
Tests.Api.Console (TestOnErrorDefault500 + TestOnErrorCallbackCrash). The FPC
standalone tests/fpc-exception-propagation check asserted the same body and is
updated too.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant