Skip to content

security: escape exception messages in default HTML error response#1385

Open
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-xss-error-fix-5073066849316392072
Open

security: escape exception messages in default HTML error response#1385
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-xss-error-fix-5073066849316392072

Conversation

@renich

@renich renich commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Escapes raw HTML exception messages in the default error controller to prevent reflected XSS.

Co-developed-by: Gemini AI renich+gemini@woralelandia.com
Signed-off-by: Rénich Bon Ćirić renich@woralelandia.com

The default HTML error response directly embedded the exception message without escaping it, leading to a Reflected Cross-Site Scripting (XSS) vulnerability.

This commit properly requires the `html` standard library module and utilizes `HTML.escape` to neutralize the exception message before embedding it in the HTML template. It also properly handles the `String?` type of `@ex.message` in Crystal.

Co-authored-by: renich <225115+renich@users.noreply.github.com>
@crimson-knight

Copy link
Copy Markdown
Member

This is a legitimate fix and we'll accept it — thank you. ✅

I confirmed the vulnerability is real: src/amber/controller/error.cr interpolates @ex.message directly into the HTML 500 page, and Amber::Exceptions::RouteNotFound embeds the raw request.path into that message, so a request to /<script>...</script> reflects unescaped into the response. It fires in non-development environments serving Accept: text/html (development renders the exception_page shard instead), so it's a real production reflected-XSS. HTML.escape(@ex.message || "") is the correct, complete fix for that response, and the nil guard is right since Exception#message : String?.

One thing needed before merge:

  • Add a spec asserting the body is escaped. The existing spec/amber/pipes/error_spec.cr only checks status codes, so nothing guards against regression. Please add a case that drives an exception whose message contains <script> (e.g. via RouteNotFound with a <script> path) and asserts the response body contains the escaped entities (&lt;script&gt;) and not the raw tag.

No need to remove .jules/sentinel.md — keeping the agent notes is fine with us. Once the spec is in, this is good to merge.

Add a regression spec in error_spec.cr verifying that exception messages
are properly escaped when text/html is requested in production mode.

Co-developed-by: Gemini AI <renich+gemini@woralelandia.com>
Signed-off-by: Rénich Bon Ćirić <renich@woralelandia.com>
@renich

renich commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

I have updated the PR to address your feedback.

Following your suggestion, I have added a regression spec in spec/amber/pipes/error_spec.cr that sets the Accept: text/html header and drives an exception whose message contains HTML characters (e.g. <script>alert('xss')</script>), verifying that the response status is 500 and the returned HTML body contains the escaped entities (&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;) instead of raw unescaped tags.

All specs compile and pass.

(Note: This contribution was co-developed with Gemini AI. Rénich has directed, reviewed, tested, and takes full responsibility for this code.)

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.

2 participants