Phpinspectionsea: Security advisories for Composer package placement.

[niconoe-]

Hi guys,

I can see the inspection Security advisories for Composer package warn me even if I add the "roave/security-advisories": "dev-master" line in the "require-dev" section of my composer.json.

Does this metapackage must really be set in the "require" section and can't be simply added in the "require-dev" section to avoid security-checking process in my production environment?

I mean, in good practices, I should run composer update on my local environment where the security-checks will be done, but then I'll use the generated composer.lock file in my production environment with a "--no-dev" flag and where all packages are already checked, isn't it?

Is it possible to detect the presence of the "roave/security-advisories": "dev-master" line either in the require or require-dev section?

Thanks.
Regards,

[kalessil]

Hi @niconoe- ,

Hm, require-dev was not intended.

Lets ask @Ocramius: showd we check require-dev for the advisories or best practice is to place it in the require-section only?

[Ocramius]

It should be in the "require" section, as you want it to exclude packages specifically for production. composer update --no-dev MUST exclude security-sensitive issues.

Please note that installing dependencies for production should happen via composer install --no-dev, and not via composer update, since your composer.lock file must be used to determine which packages are compatible.

[kalessil]

Sounds good to me.

@niconoe- : I'll add the note from Ocramius to corresponding chapter of documentation. I also can update the warning text to mention the case, if you want.

[niconoe-]

Hm, the point is, working on an application implies to commit the generated composer.lock file each time ones update its dependencies, whether they're in require or require-dev sections.
So, in this case where the lock file is commited, it has to be generated so you're pretty sure there are no conflicts inside even if the security check is in the "require-dev" section.

But the pros of it is you don't have (and you don't want) to check again the security once the composer install --no-dev runs in production environment, as it's already done and specified in the lock file. When building a production environment, it saves you few seconds to avoid this check (and believe me in my micro-services system, this time-saving can grow very fast!)

I know for libraries, where the lockfile may be not commited, it's a different case.
That's why, what I suggest is to create an option in the inspection we can call "Authorize security packages in 'require-dev' section", defaults to false.
Depending on whether working on an application where the lockfile is commited or a library where the lockfile is not necessary to be commited, the security packages can (or not) be declared in the "require-dev" section.

What do you think about it?

Thanks guys

[Ocramius]

But the pros of it is you don't have (and you don't want) to check again the security once the composer install --no-dev

Checking only happens during composer update, never during composer install.

When building a production environment, it saves you few seconds to avoid this check

Checks are completely ignored with composer install. Lock file rules there.

I think there's simply some misunderstanding on how the package works

[kalessil]

There is a chance that my documentation is what missleading:
The firewall declares vulnerable components as conflicting and not allows to install them via Composer.

[niconoe-]

Checking only happens during composer update, never during composer install.

I'm not sure about this: if you do not own a composer.lock file, the composer install command will act like the composer update , and so your package will check the dependencies.

[Ocramius]

Yes, but then it's your mistake (reads: fuckup). Deployable projects must
ALWAYS have a lock file. This applies to every dependency resolution
system, not just composer.

On 7 Feb 2017 16:23, "Nicolas Giraud" [e-mail] wrote:

Checking only happens during composer update, never during composer
install.

I'm not sure about this: if you do not own a composer.lock file, the composer
install command will act like the composer update , and so your package
will check the dependencies.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kalessil/phpinspectionsea/issues/131#issuecomment-278032373,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakPLpp7_tgc9i7nPaEfY0Y_4g0B6Pks5raIxUgaJpZM4L5YxP
.

[kalessil]

I'd like to keep the behavior as it is, as I normally encourage everyone using --no-dev in production and this needs the inspection to check require. For sake of securing production environments without the possibility to shoot own foot.

And I trying to get into the workflow, there is one Idea for @niconoe- : the package can be installed globally on dev-machines composer require roave/security-advisories:dev-master, that's would be a solution.

EDIT: composer update in CI/CD is actually a way to ensure no surprises at rollout.

[Ocramius]

This package has 0 deployment risks: it only ever comes into play during composer update, which should be performed in a CI/CD environment anyway

[niconoe-]

@Ocramius : Ok, in this case, you're right.
I thought about libraries that may not versionize composer.lock as recommended by composer itself: https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file

The main problem for me was about performance and not doing what has been already done. If the security-check only occurs during the change of the lockfile (either with an update or because of the missing lockfile), then there's no performance issue to place the package in the "require" section.

It is easy to think to all cases, in classical workflows, and see that there are no trouble to declare it in the "require" section.

Sorry to disturb you on this, my mistake :).
Anyway, @kalessil, I'll probably try to apply this installation globally: I think it would be a good solution too.

Thanks again guys.
Regards,

[kalessil]

@Ocramius 👍 , made my comment more specific ;)

[kalessil]

@niconoe-, sounds good.

And to share the experience: I had no issues using the package in production, but take your time start with dev- and CI-machines. @Ocramius is a person you can trust.

[Ocramius]

@niconoe- if you have any documentation improvement suggestions, also for Roave/SecurityAdvisories, please do suggest them. The discussion was hopefully useful, but it needs to be conveyed further to all other users.

[kalessil]

Added to EA documentation:

• Note: if the composer.lock is coming with you project, adding roave/security-advisories brings 0 deployment risks.
• Best practices: consider adding composer update into your CI/CD to get informed about security issues early.

[kelunik]

I'd like to re-open the discussion here. While require might be right for projects, require-dev should be the way to go for libraries. You don't want to end up with a dev-master dependency in require for libraries.

[Ocramius]

Shouldn't need it at all for libraries

On 22 Sep 2017 14:08, "Niklas Keller" [e-mail] wrote:

I'd like to re-open the discussion here. While require might be right for
projects, require-dev should be the way to go for libraries. You don't
want to end up with a dev-master dependency in require for libraries.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kalessil/phpinspectionsea/issues/131#issuecomment-331429286,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakOKwLbqlT6p4Qp5k1GZjDy4Vnr24ks5sk6M9gaJpZM4L5YxP
.

[niconoe-]

Shouldn't need it at all for libraries

Totally agreed with that.

So, is there a possibility for the PHPStormInspections EA Extended to NOT warn the developer when the package is missing in the composer.json and the "type" configuration is set to "library"?
Also, according to the composer.json documentation, when missing the "type" config is set to "library" as default value. It should then be ignored the same way when no "type" defined.

[kalessil]

I had already multiple discussions about the topic, but we always end up with a simple example:

• randomly selecting a package from the advisories (-> symfony/security),
• checking it's manifest: "type": "library"
• All are satisfied with the current solution.

I'm particularly interested in cases when the inspection causes issues with delivery - then we can find out what to do, regular incident management.

[kalessil]

@kelunik, @niconoe- : can you share your use-cases, please (how adding the package into require makes any harm to you/your packages consumers)?

[kelunik]

@kalessil If I'm not mistaken a dev-master dependency in a dependency requires to have minimum-stability: dev in Composer, no? At least unless the root package also requires that dependency with dev-master. It would therefore result in _"Your requirements could not be fulfilled"_ error messages during installation.

[kalessil]

@kelunik: sold, I'll play with this case and will deploy necessary changes.

[kalessil]

Fixed, the inspection doesn't fire anymore for "type": "library" (docs updated as well).
Thank you for reporting and use-cases guys!