search

PerlDancer / Dancer2

Perl Dancer Next Generation (rewrite of Perl Dancer)
http://perldancer.org/
Other
543 stars 273 forks source link

Internal server errors in Dancer code should not include stack trace in response. #586

Open baynes opened 10 years ago

baynes commented 10 years ago

Perl errors (in Dancer outside routes) give a stack trace in the error page.

stephenb:~:3$ curl -i -X JUUJKJ 192.168.137.140:81/ui/uidoc
HTTP/1.1 500 Internal Server Error
Date: Tue, 13 May 2014 16:05:47 GMT
Server: Apache/2
Connection: close
Transfer-Encoding: chunked
Content-Type: text/plain

Internal Server Error

isa check for "method" failed: JUUJKJ is not a Dancer2HTTPMethod! at (eval 269) line 384
    Dancer2::Core::Request::new('Dancer2::Core::Request', 'env', 'HASH(0x3839ba0)') called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/Context.pm line 51
    Dancer2::Core::Context::_build_request('Dancer2::Core::Context=HASH(0x38c6078)') called at (eval 266) line 29
    Dancer2::Core::Context::request('Dancer2::Core::Context=HASH(0x38c6078)') called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/App.pm line 238
    Dancer2::Core::App::_init_for_context('Dancer2::Core::App=HASH(0x2fb4528)', 'Dancer2::Core::Context=HASH(0x38c6078)') called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/App.pm line 70
    Dancer2::Core::App::__ANON__('Dancer2::Core::App=HASH(0x2fb4528)', 'Dancer2::Core::Context=HASH(0x38c6078)') called at (eval 265) line 26
    Dancer2::Core::App::context('Dancer2::Core::App=HASH(0x2fb4528)', 'Dancer2::Core::Context=HASH(0x38c6078)') called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/Dispatcher.pm line 53
    Dancer2::Core::Dispatcher::dispatch('Dancer2::Core::Dispatcher=HASH(0x389a9a0)', 'HASH(0x3839ba0)', undef, undef) called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/Dispatcher.pm line 165
    Dancer2::Core::Dispatcher::__ANON__('CODE(0x2f87988)', 'Dancer2::Core::Dispatcher=HASH(0x389a9a0)', 'HASH(0x3839ba0)') called at (eval 137) line 1
    Dancer2::Core::Dispatcher::__ANON__('Dancer2::Core::Dispatcher=HASH(0x389a9a0)', 'HASH(0x3839ba0)') called at (eval 139) line 2
    Dancer2::Core::Dispatcher::dispatch('Dancer2::Core::Dispatcher=HASH(0x389a9a0)', 'HASH(0x3839ba0)') called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/Role/Server.pm line 76
    eval {...} called at /usr/share/perl5/site_perl/5.14.2/Dancer2/Core/Role/Server.pm line 76
    Dancer2::Core::Role::Server::__ANON__('HASH(0x3839ba0)') called at /usr/share/perl5/site_perl/5.14.2/Plack/Util.pm line 142
    eval {...} called at /usr/share/perl5/site_perl/5.14.2/Plack/Util.pm line 142
    Plack::Util::run_app('CODE(0x31f2898)', 'HASH(0x3839ba0)') called at /usr/share/perl5/site_perl/5.14.2/Plack/Handler/FCGI.pm line 134
    Plack::Handler::FCGI::run('Plack::Handler::FCGI=HASH(0x2160ec8)', 'CODE(0x31f2898)') called at /usr/share/perl5/site_perl/5.14.2/Plack/Loader.pm line 84
    Plack::Loader::run('Plack::Loader=HASH(0x2087820)', 'Plack::Handler::FCGI=HASH(0x2160ec8)') called at /usr/share/perl5/site_perl/5.14.2/Plack/Runner.pm line 277
    Plack::Runner::run('Plack::Runner=HASH(0x1f16468)') called at /usr/bin/plackup line 10

[Above using Apache with Fast CGI to talk to Plackup which runs dancer.]

The stack trace should not normally be included in the error response as it may leak information that could be used to attack the server. It should be logged to the logfile instead.

The relevant code is the eval in sub psgi_app in Dancer2::Core::Role::Server .

It would be OK to control the presence of the stack trace in the page by the "show_errors" configuration option. Ideally it would also use the same configurable error page as obtained by a Perl error during a route but that is not important and there might be issues if the error came from generating the error page.

One solution is not to use eval here at all, and let Plack do the error trapping. Then things like Plack::Middleware::StackTrace can be used as desired. This is quite acceptable as errors in Dancer code outside routes should be a very rare occurrence and would reduce the amount of code needed to be supported.

xsawyerx commented 10 years ago

Seems to make sense (not to display error strings from internal errors on the website).

omar-m-othman commented 10 years ago

I'd like to work on that.

shumphrey commented 10 years ago

I'm inclined to agree with @baynes, internal server errors shouldn't happen, so letting plack handle it seems ok to me. Of course the priority should be to stop the internal server errors in the first place.