search

inviqa / harness-base-php

Base PHP harness for workspace https://github.com/my127/workspace
Other
4 stars 5 forks source link

Executing Ruby gems produces fatal error due to insecure PATH locations in Akeneo harness #555

Open jameshalsall opened 3 years ago

jameshalsall commented 3 years ago

In the Akeneo harness the bin and vendor/bin directories added to PATH (https://github.com/inviqa/harness-akeneo/blob/1.1.x/harness/attributes/docker.yml#L12). They also get 0777 permissions, and when you try to run a ruby gem from that container you get:

docker-compose exec -T -e PACT_BROKER_TOKEN=[MASKED] -e PACT_BROKER_BASE_URL=... console pact-broker publish /app/data/pacts/consumer -a b31275f9 -t master

/usr/local/lib/ruby/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/shared_helpers.rb:78: warning: Insecure world writable dir /app/bin in PATH, mode 040777

I can't see a reason for putting these in PATH, so I propose removing this line as a general rule, seeing as it causes problems here and probably is not best practice.

kierenevans commented 3 years ago

/app/bin is there to support composer's bin-dir and being able to run phpcs, console, etc without having to specify bin/phpcs, bin/console.

This is set up in https://github.com/my127/docker-php/blob/main/base.Dockerfile#L47

I'd prefer to fix the world writable issue (as they shouldn't be if you are using mutagen or normal mounts), and align akeneo's bin-dir to be bin/

jameshalsall commented 3 years ago

Agree, it might be nicer to sort out those permissions. However, in my experience bin/console has always been convention on Symfony projects so I don't see huge value in adding those to the PATH. I don't think adding those to the PATH supports the bin-dir in any way, just adds convenience?

kierenevans commented 3 years ago

It's about the developer experience across all of the php harnesses and affects any tooling placed in the bin folder. We could move the PATH setting from the php-fpm base image to the console image though the same symptom would occur.

The only way around this without inhibiting DX or having a security vulnerability akin to https://blog.golang.org/path-security is to set up bash aliases for tools used. This would need a login shell as ~/.bashrc is not looked at otherwise. We already do this for commands needing npm/yarn/node as node version manager is only invoked via ~/.bashrc: https://github.com/inviqa/harness-base-php/blob/304e35a23f03c935388a5b7e6cf51887bedd3a38/src/_base/harness/config/commands.yml#L121-L127

jameshalsall commented 3 years ago

I am not sure bin/console vs. console is a huge DX boost, not when we see the result of it with this bug :) but if we can use bash aliases and get the best of both worlds then 👍

kierenevans commented 3 years ago

I'd focus on the usage of behat or phpcs rather than console. Less keystrokes for tools meant to be run all the time