search

skeeto / emacs-web-server

Extensible Emacs HTTP 1.1 server
The Unlicense
240 stars 31 forks source link

Add feature showing backtrace on error page. #20

Closed conao3 closed 5 years ago

conao3 commented 5 years ago

Hi. I think that displaying the backtrace will help developers debugging. Sometimes this display is cumbersome, so I prepared an optional variable.

httpd-error: show backtrace when httpd-show-backtrace-when-error is nonnil. httpd-show-backtrace-when-error: New custom variable (default T)

NOTE:

  (concat nil)
  ;;=> ""
conao3 commented 5 years ago

Sample screenshot. capture 2019-01-10 11 46 33

skeeto commented 5 years ago

I like this, and I almost accepted it as-is except for a couple issues.

First, as-is this actually introduces a security issue. Error messages aren't sanitized and were never intended to contain untrusted data. These strings are passed directly to an HTML template that contains "%s", so there's no escaping the data before sending it back to the user as HTML. A backtrace contains arbitrary data from the client and it's injected state into the HTML template. Here's a demonstration of such an attack via a custom header:

curl -H "X: " http://localhost:8080/

A client rendering the response from this request will execute that JavaScript, making this a form of XSS attack. At the very least, this feature would need to be disabled by default. However, I'm still not comfortable with this non-default feature being so dangerous, even if that danger was clearly and obviously documented to the user (e.g. httpd-show-backtrace-when-error-this-is-dangerous). I'd much prefer to do the right thing, then still leave it disabled by default.

Second, the backtrace should really only be enabled for 5xx errors, not 404 and such. That's what's communicated via the info argument.

Third, I don't want to actually compute and serialize the backtrace unless it's necessary. Sometimes backtraces can be huge, such as when the error is a stack overflow — e.g. endless recursion with bulky arguments. This could even become a denial-of-service issue. In this PR the backtrace is generated even when backtraces are disabled.

I followed up the merge (fast-forward) with two more commits to fix all these issues (5854c48, f1c160f). Thanks!

conao3 commented 5 years ago

OK. thanks!

I apologize that my code had many problems. I learned a lot today. Thanks!