Repository navigation
fix(core): generic 500 body on the no-/failed-OnError fallbacks (no E.Message leak) - #631
Open
freitasjca wants to merge 1 commit into
Open
freitasjca wants to merge 1 commit into
freitasjca wants to merge 1 commit into
Conversation
….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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 issent 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
ExecuteOnErrorwrapper — when an OnError handler is registered butit itself raises (
Horse.Core.pas):AResponse.Send('Internal Application Error in OnError: ' + E.Message) .Status(THTTPStatus.InternalServerError);E.Messagefor an arbitraryExceptionroutinely carries internal detail aremote, 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 thatinstalls its own hardened 500 handler does not help: the router catches the
exception and returns normally, so the provider's
exceptblock never runs andthis 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:
Horse.Core.RouterTree.pasTRouterTreeExecutor.Run(FPC branch ofExecute)Horse.Core.RouterTree.pasTHorseRouterTree.Execute(Delphi branch)Horse.Core.Router.Radix.pasTRadixExecutor.Run(FPC branch)Horse.Core.Router.Radix.pasTHorseRouterRadix.Execute(Delphi branch)Horse.Core.pasTHorseCore.ExecuteOnError(OnError handler raised)Fix
Each send drops
': ' + E.Messageand emits only the generic phrase, preservingthe existing status and the leading text clients may already match on:
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 ofeach
on E: Exception doblock are untouched:EHorseCallbackInterrupted→ re-raised (or re-raised only when no OnError, inthe Delphi branches) — unchanged.
EHorseException→ still returnsEHorseException.ToJSONwith its ownstatus. This message is author-chosen (built with
.Error(text)by theapplication), so it is intentional output and stays as-is.
THorse.HasOnError→ still delegates toTHorse.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.OnErroris the designed extension point and it receives the fullE.An application that wants the message in the body, or wants it logged
server-side, registers a handler:
This PR deliberately does not add
Writeln/console I/O to core: core runsinside console, VCL, service, daemon and FPC hosts, and an unconditional write
to a detached
ErrOutputcan itself raise. Server-side logging is theapplication'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.RadixandTests.Api.Console— all updated here. The 500 status is unchanged; only thebody 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:
Updated assertions:
Tests.Horse.Core.RouterTree— generic fallback + rethrow path, andFallbackFlushesCookies.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
EHorseExceptionand 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 unitassertions 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.Integration.ErrorHandling.TestGenericExceptionReturnsHTTP500(updated inthe same pass) passed, confirming the end-to-end wire body is now
Internal Application Error. TheHorse.Core.paswrapper fix +TestOnErrorCallbackCrashwere added after this run.Full green (2026-11-04), with all updated files applied:
tests\run_delphi_tests.ps1on 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.dprcompiles 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). Runtests/fpc-exception-propagation/run-exception-propagation-test.sh(FPC 3.2.2); expect
PASSlines, exit 0, no unfreed-memory report.Files changed
src/Horse.Core.pas—ExecuteOnErrorcrash-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).