chore: merge final consecutive RUN in Dockerfile (close docker:S7031)#12
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Consolidates the remaining consecutive RUN instructions in the runtime stage of the Dockerfile to satisfy SonarCloud rule docker:S7031 and reduce image layers, while keeping the same filesystem-setup behavior.
Changes:
- Merged the
/run/php-fpmdirectory setup into the existing runtime-stage filesystem-setupRUNto remove an extra layer. - Expanded/rewrote the surrounding Dockerfile commentary to document the runtime-stage setup steps and rationale.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates multiple RUN instructions in the Dockerfile into a single layer to comply with SonarCloud rule docker:S7031 and reduce the number of image layers. Specifically, the creation and ownership assignment of the /run/php-fpm directory have been merged into the main setup block, and the internal documentation has been updated to reflect these changes. I have no feedback to provide as there were no review comments.
SonarCloud reported docker:S7031 ("Merge this RUN instruction with the
consecutive ones") at Dockerfile:201 after the runtime-stage cleanup in
ea909c9. The remaining two RUNs were:
RUN set -eux; mkdir -p /var/lib/snipeit && cp deps-manifest.txt && chmod && rm && chown storage bootstrap/cache /var/lib/snipeit && chmod entrypoint.sh
RUN mkdir -p /run/php-fpm && chown www-data:www-data /run/php-fpm
Both are filesystem-setup steps that can run in the same layer. Folded
into a single RUN: mkdir picks up /run/php-fpm too, chown gets it via
the existing -R group, the comment block above is restructured to
document all four logical blocks (manifest / writable surfaces /
socket dir / executable bit). One fewer image layer.
Verified locally:
- buildx --check: no warnings
- runtime image builds clean
- `ls -lhd /run/php-fpm` shows `drwxr-xr-x 2 www-data www-data` —
permissions identical to the pre-merge layer-pair.
SonarCloud quality state on main projected after this lands:
OPEN issues: 1 → 0 (this was the last)
Hotspots: 2 (unchanged — Dockerfile:131 root-by-design,
Dockerfile:193 chown=root:www-data is the
conventional pattern; both need to be marked
REVIEWED+SAFE in the SonarCloud UI by a
project admin, since the repo has no
SONAR_TOKEN secret for API-based marking).
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Copilot caught two inaccuracies in the comment block introduced by 3cf6820. Both are fixed in this commit. 1. Dockerfile comment claimed `/run/php-fpm` is "re-chown'd by entrypoint.sh", but the entrypoint's chown list only covered storage, bootstrap/cache, /var/lib/snipeit. Added `/run/php-fpm` to the entrypoint's mkdir + chown blocks so the claim becomes true — and so that the tmpfs case (compose mounts a tmpfs at /run/php-fpm, which masks the image-layer chown) works without relying on tmpfs default 1777 mode. Standalone `docker run` keeps using the image-layer chown. 2. Dockerfile comment said "NO TCP port is exposed", but the base image php:8.5-fpm-alpine inherits `EXPOSE 9000` as OCI metadata. Dockerfile has no `UNEXPOSE`. The real security property is that nothing LISTENS on TCP 9000 — our `php-fpm.d/zz-snipe-it.conf` sets `listen = /run/php-fpm/snipeit.sock`, so the FastCGI bypass is closed by config, not by the absence of EXPOSE metadata. Comment rewritten to be accurate about this distinction. Verified: - shellcheck clean on entrypoint.sh - bash -n clean - docker buildx build --check: no warnings - runtime image builds clean - `ls -lhd /run/php-fpm` in the rebuilt image: drwxr-xr-x www-data (same ownership as before — the additional entrypoint chown is a no-op when the image-layer chown already matches, and the safety net for the tmpfs case). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
force-pushed
the
chore/dockerfile-merge-final-runs
branch
from
a2f85a8 to
0b3d762
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the last remaining SonarCloud OPEN issue on
main: docker:S7031 ("Merge this RUN instruction with the consecutive ones") at Dockerfile:201.After ea909c9's chown-hardening + first RUN consolidation, two consecutive
RUNs remained in the runtime stage:RUN set -eux; mkdir -p /var/lib/snipeit && cp ... && chown storage,bootstrap/cache,/var/lib/snipeit && chmod entrypointRUN mkdir -p /run/php-fpm && chown www-data /run/php-fpmBoth are filesystem-setup steps that can share an image layer. Folded into a single RUN; mkdir picks up
/run/php-fpmand chown extends to it. Net -1 image layer, no functional change.Test plan
docker buildx build --check .— no warningsls -lhd /run/php-fpmin built image:drwxr-xr-x 2 www-data www-data— identical to pre-merge stateSonarCloud projected state after merge
Dockerfile:193—COPY --chown=root:www-data(defense-in-depth chown; SonarCloud rule wants any group ownership flagged for review)Dockerfile:131—php:fpm-alpineruns as root by default (intentional; entrypoint drops to www-data viasu-execafter chown'ing fresh volumes)Both hotspots are by-design and need to be marked REVIEWED + SAFE in the SonarCloud UI. The repo has no
SONAR_TOKENsecret, so this can't be automated.