Framework: Cannot generate an app key if session is encrypted and a response macro exists

[lindyhopchris]

• Laravel Version: 5.4.32
• PHP Version: 7.0.18
• Database Driver & Version: n/a

Description:

An application with these three things:

• session.encrypt config set to true
• A response macro registered in a service provider boot method
• No application key (e.g. because you've just cloned an existing application from your team's version control).

You cannot run any Artisan commands (including key:generate) because you get the following error:

[RuntimeException]                                                                        
The only supported ciphers are AES-128-CBC and AES-256-CBC with the correct key lengths. 

Looking at the stack trace it's because resolving the responses factory to register the macro results in the following being resolved out of the container:

1. The redirect service...
2. Which is injected with the session.store...
3. session.store calls driver() on the session immediately
4. Which results in the encrypted session being created, injected with the encrypter:
   https://github.com/illuminate/session/blob/master/SessionServiceProvider.php#L47

As well as causing the above problem, this also all seems very costly just to register a response macro. I'm thinking somewhere in the chain something needs to be injected with the container and only resolve the things it needs when it actually needs them. My suggestion would be the responses factory or the redirector.

If you advise what in the chain should change, I'd be willing to work on a PR for a fix.

Steps To Reproduce:

1. Create a brand new Laravel application
2. Remove the application key
3. Set session.encrypt to true
4. Add the following to your AppServiceProvider::boot() method:

    public function boot()
    {
        Response::macro('caps', function ($value) {
            return Response::make(strtoupper($value));
        });
    }

[Dylan-DPC]

Yes. The error will show because you don't have an App Key. So I don't see this as a bug if you remove the app key.

[lindyhopchris]

That's just to reproduce the bug - you have to remove the app key from a fresh Laravel install because a fresh install generates a key.

The bug is that if you are cloning an existing application, i.e. an established application that your team is working on and already has the session set to encrypted and a response macro in a service provider, then you get the bug.

Plus as described above, there's the bigger point of the whole thing is very costly and will be happening every single time the application runs (either for a HTTP request or running in the console) just to register a response macro.

[Dylan-DPC]

My point was -> if your application doesn't have a key, it will throw that error...

[joshmanders]

@Dylan-DPC I don't think you're understanding what @lindyhopchris is saying.

[Dylan-DPC]

Ah i guess i kinda understood now. It could be that you need the key while encrypting the session? You need to "boot" up the framework either way even if you are running a command.

[laurencei]

So I guess the question is; should response macros work on CLI commands? i.e. if you are calling from the CLI - then are you expecting a response macro to kick in at all?

If the answer is no - then we just do a CLI check before registering the macro.

If the answer is yes - then it is a "chicken/egg" problem - and the only way to solve is the manual copying of an app_env key.

[lindyhopchris]

yeah, so i agree that the response macro doesn't need to be added when running in the console, but the docs don't say anything about not doing that.

so yeah, if the solution is to wrap the response macro registration in a console check, then the docs need updating because it doesn't say to do that when registering the response macro. which I'm happy to do a PR for if that's the verdict, otherwise the response macro registering needs to be more lazy!!

[lindyhopchris]

@laurencei

Unfortunately, your suggested fix doesn't work. If you wrap the response macro in a runningInConsole check, your tests fails.

Yes, you could both check whether you're running in the console plus whether the environment is testing, but I'd suggest that's getting pretty complicated just to register a response macro.

[laurencei]

Ok - in that case then the question becomes - should a response macro registration trigger the session?

If the answer is no - then it's a bug?

If the answer is yes - then I dont see a way around it (chicken/egg problem) and the only solution would seem to be a manual creation of an app key...?

[lindyhopchris]

Yep, exactly.

Considering all we are doing here is registering a macro, triggering the session feels completely unnecessary and therefore IMHO this is a bug.

[jameswmcnab]

This seems like a bug to me in that using two features together (encrypted cookies and response macros) causes the artisan cli to be unusable; specially since running key:generate is part of framework (and therefore app) installation steps (https://laravel.com/docs/5.4/installation#configuration).

[Dylan-DPC]

I don't think this would be fixed as it will cause some major changes. Could be targetted for 5.6 though if needed

[srmklive]

@lindyhopchris If the issue is copying .env files, then the APP_KEY value can be added as default in config/app.php by updating 'key' => env('APP_KEY') to 'key' => env('APP_KEY', 'SOMEKEY'), so the error you are referring to doesn't exists. You always need to have an APP_KEY

[lindyhopchris]

App keys must never be committed to version control. Plus what you're suggesting doesn't deal with the problem that the session, encrypter etc are all being resolved out of the service container just to register a response macro.

[laurencei]

@lindyhopchris - I think @srmklive means to put a generic default dev app_key - which is overridden by your production .env - so there is no issue around version control

[ghost]

@lindyhopchris

Yesterday I opened an ISSUE going through the same problem. I was able to leave my macros normally in Middleware but using Html::macro() instead of Form::macro().

Take this test and give us a feedback.

[lindyhopchris]

I can't use either of those macros to replace Response::macro.

I believe this a design problem in that macro registration via a Facade is too eager in generating services out of the container. I think a better solution is to potentially use the afterResolving hook on the container to register the macro at the point the responses service is resolved out of the container, but I haven't tried it yet.

[ghost]

@lindyhopchris

I would think you'd better use an Observer for your entity attributes and eliminate this Response::macro(). In fact, I particularly _made a Trait for my entities_. Within each specific Trait I implemented the getter methods.

Example:

Entity: User.php / Namespace: App/Entities;
Trait: UserTrait.php / Namespace: App/Traits;

<?php
use App/Traits/UserTrait;

class Userextends Model {
     use UserTrait;
     protected $fillable = ['name'];
}

trait UserTrait {
     public function getNameAttribute($value)
     {
          return uppercase($value);
     }
}

I do not particularly consider that to be a bad idea. I await your return.

[lindyhopchris]

@samuelcdossantos thanks for your message, but this doesn't match the use case of the response macro. The response macro is for:

defin[ing] a custom response that you can re-use in a variety of your routes and controllers
https://laravel.com/docs/5.5/responses#response-macros

I have a package that provides JSON API spec compliant responses in Laravel applications. I'm using the response macro so that people can return JSON API responses from the response() helper function like this:

return response()->jsonApi()->content($data)

[ghost]

@lindyhopchris

Okay, maybe I would have to visualize your development scenario. When I need the API (I do not know what you are returning in your), I already direct return the entity response.

Example:

namespace App\Controllers;

use App\Entities\User;

class UsersController extends Controller {
     // Model Binding
     public function show(User $user) {
          return \Response::json(['data' => $user]);
          // Or return return $user;
     }
}

Sorry if I could not help but that was my idea. If you need any other help I am available to help as much as possible.

I still never get into a case like yours because I always do a Domain / Answer Contract in which I use a JSON response pattern.

Example:

UsersController.php

namespace App\Controllers;

use App\Entities\User;
use App\Domains\ResponseDomain;

class UsersController extends Controller {
     // Model Binding
     public function show(User $user) {
          return ResponseDomain::toJsonApi($user);
     }
}

ResponseDomain.php

namespace App\Domains;

class ResponseDomain {     
     public static function toJsonApi(array $data, $status) {
          return \Response::json(['data' => '$data, 'status' => ($status ?: 200)]);
     }
}

[driesvints]

This seems to have been fixed in the meantime because I can't reproduce this on a fresh 5.7 install.