Refuse to set signed or encrypted cookies with an insecure default secret #2252
Conversation
|
It seems a little overengineered, but i also don't know a better solution. |
| my $secret = $app->secrets->[0]; | ||
| my $moniker = $app->moniker; | ||
|
|
||
| Carp::croak 'Your secret passphrase must be changed to set session data (see FAQ for more)' |
There was a problem hiding this comment.
I don't think these secrets are "passphrase"s (and the FAQ doesn't call them that either)
| my $moniker = $app->moniker; | ||
|
|
||
| Carp::croak 'Your secret passphrase must be changed to set signed cookies (see FAQ for more)' | ||
| if !$ENV{MOJO_ALLOW_INSECURE_SECRET} and (!length $secret or $secret eq $moniker); |
There was a problem hiding this comment.
Would it make sense to move this check into $app->session?
There was a problem hiding this comment.
There is no session method for Mojolicious; that would be calling the session helper, which just forwards to the session method on the controller which is handled above (albeit with some issues as described in the initial comment).
There was a problem hiding this comment.
yes, sorry, github threw away my comment, and i crankly re-typed it wrong.
I mean should it be in $app->secrets
There was a problem hiding this comment.
That attribute may be accessed by applications that don't actually do anything with it, such as Mojolicious::Plugin::Mount. But perhaps we can work around that more easily than the problems with this approach.
Summary
Rough attempt at taking the approach of dying if encrypted or signed cookies are requested to be set but the application secret is still set to the default of the application moniker.
This is fairly straightforward to do in the signed_cookie and encrypted_cookie methods, but this also requires a check in the session method, because setting the session cookie occurs too late to error out sensibly.
I'm not satisfied with this check in the session method, because: there still may be instances where this session method is called while a response is already being rendered; and session data can be set by assigning to
$c->session->{foo}and there's no way to know that will happen at this juncture.Additionally, this check does not help cases where the secret has been set to a value that is also insecure but is not the current application's moniker, such as with Mojolicious::Plugin::Mount.
I am not sure if these issues can be solved, but wanted to put this idea into practice for others to review.
This also affects use of features using the session implicitly, such as the flash and csrf_token default helpers.
The
MOJO_ALLOW_INSECURE_SECRETenv variable is provided as a bypass option for applications that can't be made secure for whatever reason.Motivation
The risk of insecure secrets being used in applications using signed or encrypted cookies is not sufficiently mitigated by the annoying warning in the logs. But we don't want to cause applications to fail if they are not using these features.
References
Implements a part of #2200 by refusing to set signed or encrypted cookies by default unless the secret has been changed from the default.