Roave / SecurityAdvisories

:closed_lock_with_key: Security advisories as a simple composer exclusion list, updated daily
MIT License
2.72k stars 106 forks source link

laravel/laravel is not a valid package to include #81

Closed LukeTowers closed 3 years ago

LukeTowers commented 3 years ago

https://github.com/Roave/SecurityAdvisories/commit/bc10788e8d1fc3c962d92faa2442bd1852c08c03#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34R139 is not a valid inclusion in this package because it is unactionable and prevents existing Laravel projects from updating their dependencies while requiring this package.

The inclusion of that line stems from https://github.com/advisories/GHSA-246r-r2wf-frhx which is a security advisory about a weak default value in the laravel/laravel project demo / template repository. The problem stems from the fact that laravel/laravel is not a dependency that is pulled into other projects, it is instead a base point that other projects start from.

This causes issues with a large amount of existing (and new) Laravel projects because most people don't change the name property in their project's composer.json file, meaning that a significant number of projects out there have name: laravel/laravel in their composer.json files, which means that this package will (falsely) now conflict with their very own project's composer.json file.

The specific error message presented to users in the above case is the following:

Problem 1
    - laravel/laravel is present at version 1.0.0+no-version-set and cannot be modified by Composer
    - roave/security-advisories dev-master conflicts with roave/security-advisories dev-master.
    - Root composer.json requires roave/security-advisories dev-master -> satisfiable by roave/security-advisories[dev-master].
Ocramius commented 3 years ago

@LukeTowers that has to be brought up with the authors/maintainers of https://github.com/advisories/GHSA-246r-r2wf-frhx

This project only mirrors the advisories, and is a generated compacted output of all advisories put together.

Ocramius commented 3 years ago

As for people using name: laravel/laravel in their project, the bug is on their end :-)

LukeTowers commented 3 years ago

As for people using name: laravel/laravel in their project, the bug is on their end :-)

Oh I agree with that for sure @Ocramius.

@todb-r7, @DanielCoulbourne, @denisdulici, @cuneytsenturk do any of you know who published https://github.com/advisories/GHSA-246r-r2wf-frhx and who would have access to adjust the package referenced by that advisory?

@driesvints @taylorotwell did either of you publish the advisory targeting laravel/laravel? And if you didn't, I'd recommend getting in touch with GitHub support to get them to remove the laravel/laravel package from that advisory. Personally I'd consider it a bug in GitHub to have allowed anyone other than you two to publish an advisory that affects the laravel/laravel package.

todb-r7 commented 3 years ago

Hi @LukeTowers happy to help -- from what I recall here, the fundamental issue isn't technically a vulnerability in Laravel, but it does feel like a footgun; it's a feature, but it's a feature that is easy to mess up, which is why we pointed to it in our vulnerability disclosure on Akaunting (CVE-2021-36804).

I don't know who wrote up GHSA-246r-r2wf-frhx but it's incorrect to say that this CVE ID is targeted at Laravel's projects. It merely refers to them.

LukeTowers commented 3 years ago

@todb-r7 thanks for the response! That lines up with my impression too, sounds like we'll have to contact GitHub support then to get the advisory corrected.

LukeTowers commented 3 years ago

I've submitted a ticket to GitHub support to fix the advisory as well as a pull request to the Laravel docs (https://github.com/laravel/docs/pull/7284) to recommend changing the name property in composer.json after installing.

driesvints commented 3 years ago

As for people using name: laravel/laravel in their project, the bug is on their end :-)

I definitely agree that it's not a sane default to keep but we can't expect people to change this out of their own all the time. If people want to use this package they can change it.

did either of you publish the advisory targeting laravel/laravel?

We didn't. I'll get in touch with some of my GitHub contacts to see if we can get that pulled. I also agree that it's weird that others besides ourselves can submit advisories for our packages.

denisdulici commented 3 years ago

Unfortunately, we don't know who published that advisory.

LukeTowers commented 3 years ago

@driesvints I contacted github support and they said they're raising the issue with their engineering team to change the targeted package in the advisory.

LukeTowers commented 3 years ago

@driesvints they've updated the advisory now so we should be good. Might still be worth it for you to talk to your contacts about preventing advisories being issued that target the laravel namespace in the future though.

@Ocramius I assume that your code will automatically detect the change in the package targeted by the advisory and remove the laravel/laravel line from the generated composer.json?

Ocramius commented 3 years ago

This repo is updated every hour, so yes.

driesvints commented 3 years ago

Thanks @LukeTowers

Ocramius commented 3 years ago

FWIW: https://github.com/Roave/SecurityAdvisories/commit/05f521f12b6e072bc77aaa78191907987d0454ff

driesvints commented 3 years ago

Thanks @Ocramius :)