search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
6.97k stars 332 forks source link

dockerized version changes uid/gid to root when used with -w #977

Open cdleonard opened 1 year ago

cdleonard commented 1 year ago

Example usage:

docker run -v "$PWD:/workdir" --workdir=/workdir -it mvdan/shfmt -w ./script.sh

If changes are made the uid/gid of script.sh is changed to 0:0, likely because the process inside docker runs as root. It would better for shfmt to preserve uid/gid.

mvdan commented 1 year ago

Interesting point. We could and likely should make our docker image run as a non-root user, but that wouldn't be a complete fix on its own. Like you say, we should probably retain the original owner, just like we already retain the original permission bits.

I thought about this some time ago in https://github.com/google/renameio/issues/19 already. We currently use https://pkg.go.dev/github.com/google/renameio/v2@v2.0.0/maybe#WriteFile with the permission bits obtained via https://pkg.go.dev/os#Lstat, which doesn't keep the original owner.

Worth noting that https://github.com/mvdan/sh/issues/843 is somewhat similar, in that files which are symlinks are currently replaced by regular files. Though the fix is unlikely to be the same for both.

Happy to review a PR which makes our docker image run as non-root.

Also happy to review a PR about keeping the original owner/group for input files. I think the cleanest solution would be to call https://pkg.go.dev/os#Chown after we've written each file. I wouldn't want to lose the atomic file writes, because that can lead to data corruption. A mid-way crash leaving the wrong owner/group info is much less worrying than a crash leaving the file empty or partially written.