spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.78k stars 1.08k forks source link

Herd/Valet SSL issue with addMediaFromUrl #3608

Closed jamesh-purr closed 6 months ago

jamesh-purr commented 6 months ago

Hey,

The Media Downloader needs a change as I came across an issue which took me a little while to work out. I have two laravel apps that communicate with each other and I have a need where one laravel app needs to download a file from the other as they are on different servers(pending moving files all over to be stored s3).

Now if you are using https locally using herd/valet ssl certificate, you will run into an error stating an error saying the file is no reachable. Now this kinda got me for some time as the file was accessible via the url I was passing.

After some digging I checked the media downloader and under the hood use fopen which will verify the ssl. This is where the problem lies. If you fail over to http every works as expected however I need ssl to be enabled to work with some other services.

My solution is in my case is

$context = stream_context_create([
            "ssl" => [
                "verify_peer" => config('app.env') === 'production',
                "verify_peer_name" => config('app.env') === 'production'
            ],
            "http" => [
                "header" => "User-Agent: Spatie MediaLibrary",
            ],
        ]);

However you could add a media config option instead and drive this via the .env e.g MEDIA_LIBRARY_SSL=false

$context = stream_context_create([
            "ssl" => [
                "verify_peer" => config('media-library.ssl'),
                "verify_peer_name" => config('media-library.ssl')
            ],
            "http" => [
                "header" => "User-Agent: Spatie MediaLibrary",
            ],
        ]);

I think this should be in as its a bit of a gotcha that would drive people insane if they don't notice its related to local ssl.I hope you implement this :)

patinthehat commented 6 months ago

@jamesh-purr Thanks for posting the solution you found! :+1:

I think any time we're talking about disabling SSL verification, it's worth a quick discussion before we work on a PR. I think if the default is to require verification and it can only be turned off through an intentional setting, it's probably okay; but I'd feel better about it if the setting only worked in non-production environments. thoughts, @freekmurze @sebastiandedeyne @timvandijck ?

freekmurze commented 6 months ago

For me adding such a flag would be ok. Default should indeed to require verification.

No need to add protections that it shouldn't work in production, I trust the users on doing the right thing for their needs.

webdevhayes commented 6 months ago

Thanks @freekmurze I have opened a PR. First time contributor so please go easy on me haha https://github.com/spatie/laravel-medialibrary/pull/3609