Failing to create bitmaps_test folder in GitHub Actions after upgrade to 6.2.0

[davidcmoulton]

We have successfully been using backstopjs in our GitHub Actions with 6.1.4, using npx backstop --docker test. On upgrading to 6.2.0 the backstop run in GitHub Actions fails, erroring with a bunch of:

Browser Console Log 0: JSHandle:BackstopTools have been installed.
Error: EACCES: permission denied, mkdir 'backstop_data/bitmaps_test'

We suspect the regression may be stemming from the changes in user introduced in https://github.com/garris/BackstopJS/commit/6d221a275686bad7c7b58fd7ff1325e7067e1bcb.

[garris]

@tkrah 👆 any thoughts on this one?

[tkrah]

Going to submit a PR for this, can you try that @davidcmoulton ? You can even use the 6.2.0 release to test that, just change the dockerCommandTemplate like written in the README.md to match that PR here.

[garris]

Thank you @tkrah for the fast enhancement! Also, curious what change caused this issue?

@davidcmoulton can you please validate that this patch solved your issue?

[erkannt]

I'm part of the same ensemble as @davidcmoulton and can confirm that the patch works for us:

https://github.com/sciety/sciety/pull/2168

[tkrah]

Thank you @tkrah for the fast enhancement! Also, curious what change caused this issue?

@davidcmoulton can you please validate that this patch solved your issue?

backstopjs runs as user node now, not anymore as root user, but better care about the user mapping than having root owned files which a user can't delete anymore ;-).

[theghostbel]

Does it work when running from Windows?

[tkrah]

Does it work when running from Windows?

Depends - if you have an environment where id is available yes, if not than you need to customize the dockerCommandTemplate like written in the README.md of backstopjs to use one which does work for your environment.

See https://github.com/garris/BackstopJS/pull/1467#issuecomment-1483018288 .

[theghostbel]

@tkrah, I tried with "--user %USERNAME" but got strange error:

Delegating command to Docker... docker run --rm -it --user %USERNAME% --mount type=bind,source="C:\proj\devcards",target=/src backstopjs/backstopjs:6.2.1 test "--config=backstop.js" "--moby"
docker: Error response from daemon: unable to find user nemig: no matching entries in passwd file.

If I remove --user part fully, I get error of mkdir permissions

[tkrah]

You need to use a username which your docker daemon in windows does know about. As I don't know your setup, you have to find out yourself which username you need to provide. You can also try "--user root" but that will get you files owned by root - which may or may not be a problem for you to be removed later because of the ownership, ymmv.

[theghostbel]

ok, --user root works well, no problems so far. Should add this to docs, quite a tricky problem to investigate.

dockerCommandTemplate: 'docker run --rm -it --user root --mount type=bind,source="{cwd}",target=/src backstopjs/backstopjs:{version} {backstopCommand} {args}'

[garris]

@tkrah if root works in all cases, should we just update the default config to use root and instead encourage users improve security by customizing their config?

This solution needs to work out of the box otherwise users will have a hard time adopting this update.

[tkrah]

@garris Your code, your decision ;-).

Running as root is considered bad practice, their are numerous articles (use your search engine of your choice) about why it should not be done and I don't see why backstopjs needs that root privilege.

For me the current default is the better one, because before the change I did not care about my user settings, it did run it as root by default and I ended up having root owned files in my user directory which I was unable to delete now (that's why I changed the user to the node user in the first place which the node base image does create). After that I needed to configure an explicit user and change the default template to be able to have a usable image (to me).

So having a default solution (dockerCommandTemplate) which sets the user with $(id -u) which makes all those files owned by the user who runs that (me) is an improvement (at least to me), because I don't have files now which I can't delete anymore.

root of cause will always run, because that is the user which normally is always there - but it has it nitpicks though like described.

Maybe you can change the default template to make a distinction here.

If Linux as running OS is detected (https://www.npmjs.com/package/detect-os or something of your likes - I am not an npm expert) we use the default template like it is now (with --user $(id-u):$(id -g)) and if something else is detected you use --user root ? Just an idea - you can of cause use root again like in the past although I would not vote for that, but in the end, you are the one in charge here and it is your decision, I just did add my 2 cents here, I hope it helps you in finding a decision. ;-)

[garris]

@tkrah yes, you are raising very good points! Here are the trade offs I am seeing...

1) The --user option you added is a big improvement (at least on Linux) since now files are attributed to the correct user. This is good hygiene.

2) multi-platform branching tends to be brittle and hard to develop and test. I try to avoid this when possible.

3) an important principle of BackstopJS is that features should just work without need for custom setup or reading the docs.

Since this is essentially your feature, would you be willing to propose a solution to better balance these three things? (If it is your preference, I am happy to compromise on point 2 so that point 3 is not broken.)

Please LMK. Thanks!

[tkrah]

@garris It is hard to cope with windows here, docker daemon runs in a virtual machine there - that's why the local user is not known as seen above. So if you don't want to cope with 2, we have to use root again as a default because of that, as that is the only user in that VM and of cause we must revert that too, if you want to honor your point 3 that users should not read docs (although I find that point questionable - why do you have docs in the first place if no one should read them ;-) - it is like man pages, I am always happy if man <tool> does have a usable output, but I guess that is just personal preference - nothing I have to impose on anyone).

To be honest, I can live with going back to root as default to honor your principle 2+3 and we just change the docs to mention the "new" old default template and document that at least all non windows users should really consider to use the current template as a default to fix the attributes on the files in their config, although that would mean that the default smoke tests will again produce files with wrong attributes, but I am not going to run that one so often anyway :-P.

If you agree on that I can provide a PR, but it may take a few days, short of time at the moment.

[garris]

@tkrah I was thinking about slightly better ways to address this. We could log out a warning if we find --user root in the docker command template -- and we could link it to here in the docs. So for beginners, running it for the first time just works, but users will see a warning in the logs until they fix the security weakness.

[tkrah]

I would have removed the user command from the docker file all together, so I don't know if we would have that template still in use. Let's just document it (windows user have no chance anyway to change it, which would again will be against your rule 2). I'll prepare a PR and we can discuss it there further. ;-)

[tkrah]

Fixed and can be closed @garris .

[garris]

Ah! Yes. Thank you. Cheers