Skip to content

fix: address multi-dimensional review findings (nginx routing, hardening, CI) - #7

Merged
CybotTM merged 5 commits into
mainfrom
fix/review-findings
Jun 28, 2026
Merged

fix: address multi-dimensional review findings (nginx routing, hardening, CI)#7
CybotTM merged 5 commits into
mainfrom
fix/review-findings

Conversation

@CybotTM

@CybotTM CybotTM commented Jun 28, 2026

Copy link
Copy Markdown
Member

Adversarial review (security, Dockerfile/image, entrypoint, compose, CI, importer) surfaced 35 findings; this fixes the actionable ones. Each fix was verified against a running stack / the exact CI linters.

Critical

  • nginx blocked GLPI's legacy .php URLs. The blanket location ~ \.php$ { deny all; } 403'd /apirest.php (the API), /front/central.php (post-login landing) and /ajax/*.php — GLPI 11 routes those through index.php. The login page (/) worked, so the original smoke test missed it. Removed the block; execution stays confined to index.php. Verified: /apirest.php 403→400, /front/central.php 403→302.

High / Medium

  • Health decoupled from the scheduler — app healthcheck is now php-fpm /ping only (was gated on a scheduler heartbeat, so a dead scheduler blocked web from starting).
  • cap_drop: ALL on every service (was app+web only) with minimal re-adds — caught + fixed valkey/socket-proxy cap needs by testing.
  • ofelia no longer holds the raw host docker socket (== host root): a least-privilege, healthchecked docker-socket-proxy (CONTAINERS+EXEC+POST only) sits in front; verified ofelia execs through it while /info stays 403.
  • Manifest-aware GHCR cleanupactions/delete-package-versions could delete a live image's per-arch children / cosign+SLSA referrers (the failure mode that bricked ghcr's phpbu-docker).
  • bake derives MINOR/MAJOR from VERSION (no drift); Renovate now tracks base images + bake version; release make_latest gate; remove the Dependabot/Renovate overlap.

Low / cleanup

  • cache marker stores a DSN hash (not a cleartext Redis password); SESSION_COOKIE_SECURE toggle (verified); pm.max_children sized to the cgroup; dev override uses GLPI env + loopback binds; caddy logging; GLPI_DEFAULT_LANGUAGE forwarded; README/Makefile/.env fixes.

Full stack verified healthy after all changes; hadolint/shellcheck/actionlint/yamllint/hardening-check/container-structure-test (20/20)/bats (42/42) all green. The importer findings were fixed separately in glpi-demo.

CybotTM added 4 commits June 28, 2026 11:39
…/*, ajax/*)

The blanket `location ~ \.php$ { deny all; }` matched /apirest.php, /front/central.php,
/ajax/*.php etc. before try_files could route them through public/index.php — so the
REST API and the entire post-login UI returned 403 (the login page at / worked, which
is why the original smoke test missed it). Removed the block; GLPI 11 routes everything
through index.php and execution stays confined to it (the ^/index\.php$ location is the
only fastcgi pass). Verified: /apirest.php 403->400, /front/central.php 403->302.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
…socket-proxy

- app healthcheck is now php-fpm /ping only (was gated on a scheduler-touched cron
  heartbeat, so a dead scheduler made app unhealthy and blocked web from starting);
  dropped the now-unused glpi-heartbeat job.
- cap_drop: ALL on db, valkey, scheduler, backup, app-assets, socket-proxy (only app+web
  had it) with minimal re-adds; verified every service still starts healthy.
- ofelia no longer mounts the raw host docker socket (== host root): a least-privilege
  docker-socket-proxy (CONTAINERS+EXEC+POST only, healthchecked) sits in front; ofelia
  reaches it over the internal network. Pinned ofelia by digest.
- forward GLPI_DEFAULT_LANGUAGE + SESSION_COOKIE_SECURE; add scheduler TZ; fix the
  mem_limit/pm.max_children sizing comment.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
…ion tracking

- entrypoint stores a SHA-256 of the cache DSN (not the cleartext DSN, which may carry a
  Redis password) and no longer logs the raw DSN; writes a session.cookie_secure drop-in
  from SESSION_COOKIE_SECURE so HTTPS deploys get the Secure flag.
- pm.max_children 25->10 to actually fit the 1.5g app cgroup; comment explains the math.
- docker-bake.hcl derives MINOR/MAJOR from GLPI_VERSION (no drift); dev target uses a
  neutral cache so it builds locally. Renovate now tracks the base images (PHP/Alpine)
  and the bake GLPI version too.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
…e bots, dev override

- cleanup-packages: switch to dataaxiom/ghcr-cleanup-action — actions/delete-package-versions
  is not manifest-list aware and could delete a live image's per-arch children or
  cosign/SLSA referrers (the failure mode that bricked ghcr's phpbu-docker).
- release.yml: only the newest semver release moves the 'Latest' badge (make_latest gate).
- remove dependabot.yml — Renovate already manages actions + docker + versions (no dup PRs).
- compose.override.yml.example: GLPI_ENVIRONMENT_TYPE=development (GLPI has no APP_DEBUG),
  loopback-bind db/valkey; caddy overlay logging; .env redis-auth note; README/Makefile
  console + lint + backup-profile fixes.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
github-actions[bot]
github-actions Bot previously approved these changes Jun 28, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

…n guard)

The serve check only hits / (the login page), which stayed 200 even while a blanket
nginx .php deny 403'd /apirest.php and /front/*.php. Add an explicit check that those
entry points are not forbidden so the nginx-routing regression can't recur.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request hardens the GLPI Docker setup by dropping capabilities, introducing a least-privilege Docker socket proxy for the scheduler, binding dev ports to loopback, and hashing cache credentials. However, several improvements are recommended: routing .php requests to FastCGI in Nginx to prevent raw source code exposure, configuring PHP session security via PHP-FPM pool settings instead of runtime file generation to support read-only filesystems, removing the invalid POST environment variable from the socket proxy, dropping capabilities for the app-assets service, and refining Renovate regex patterns to handle line endings and avoid overly generic matches.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

I am having trouble creating individual review comments. Click here to see my feedback.

config/nginx/conf.d/default.conf (72-73)

security-high high

Removing the location ~ \.php$ { deny all; } block entirely introduces a security vulnerability. If any other .php file (such as a test file like info.php or a file added by a plugin) exists in the public/ directory, Nginx will serve its raw source code as static text because there is no FastCGI handler for general .php files. This can leak sensitive credentials or application logic.

To safely allow legacy .php URLs to be routed to index.php while preventing raw source code leakage, you should route all .php requests to FastCGI using try_files so they are executed rather than served as static text.

  location ~ \.php$ {
    try_files $uri /index.php$is_args$args;
    fastcgi_pass unix:/run/php-fpm/glpi.sock;
    include fastcgi_params;
    fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
  }

rootfs/usr/local/bin/entrypoint.sh (252-258)

medium

Writing to /usr/local/etc/php/conf.d/zz-session-secure.ini at runtime prevents running the container with a read-only root filesystem (a highly recommended security hardening practice).

Instead of dynamically generating a .ini file in the entrypoint, you can leverage php-fpm's built-in support for setting PHP INI values directly in the pool configuration (rootfs/usr/local/etc/php-fpm.d/zz-glpi.conf) using environment variables:

php_value[session.cookie_secure] = $SESSION_COOKIE_SECURE

This simplifies the entrypoint and makes the container fully compatible with read-only root filesystems.

compose.yml (361)

medium

POST is not a valid environment variable for tecnativa/docker-socket-proxy. The proxy automatically allows the necessary POST requests for creating and starting exec instances when EXEC: "1" is enabled. Setting POST: "1" has no effect and can mislead maintainers into thinking it is a supported configuration. You can safely remove this line.

compose.yml (210-211)

medium

The app-assets service is missing cap_drop: - ALL, which goes against the PR's stated goal of hardening all services with cap_drop: ALL. Since app-assets only copies static files and exits, it does not require any Linux capabilities. Adding cap_drop: - ALL will further harden this service.

    security_opt:
      - no-new-privileges:true
    cap_drop:
      - ALL

renovate.json (42-43)

medium

Using a literal \\n in the regex pattern is sensitive to line endings and will fail to match if the file uses Windows-style CRLF (\r\n) line endings. Using \\r?\\n or \\s+ makes the regex robust against different line endings.

        "# renovate: datasource=docker depName=php versioning=docker\\r?\\nARG PHP_VERSION=(?<currentValue>\\S+)",
        "# renovate: datasource=docker depName=alpine versioning=docker\\r?\\nARG ALPINE_VERSION=(?<currentValue>\\S+)"

renovate.json (53)

medium

The regex default = "(?<currentValue>\d+\.\d+\.\d+)" is too generic and will match any variable in docker-bake.hcl that has a semantic version as its default value. This can lead to incorrect updates if other variables are added in the future. Making the regex specific to the GLPI_VERSION variable prevents false positives.

        "variable \\"GLPI_VERSION\\" \\{\\s*default = \\"(?<currentValue>\\d+\\.\\d+\\.\\d+)\\"

@CybotTM
CybotTM merged commit f2940ab into main Jun 28, 2026
21 checks passed
@CybotTM
CybotTM deleted the fix/review-findings branch June 28, 2026 09:44
@sonarqubecloud

Copy link
Copy Markdown

CybotTM added a commit to netresearch/phpbu-docker that referenced this pull request Jun 28, 2026
…dren) (#162)

## What broke

The scheduled `Cleanup Old Packages` run on 2026-06-28 04:35 deleted
**50 untagged versions** with `actions/delete-package-versions`
(`delete-only-untagged-versions: true`). For a multi-arch image, the
per-arch **child manifests** and the cosign/SLSA **referrers** are
stored as *untagged* versions — so it deleted the children of live
tagged images, leaving every tag's index pointing at missing children.
Result: `docker pull ghcr.io/netresearch/phpbu-docker:6` (and every
other tag) now fails with **`manifest unknown`**, which broke every
downstream consumer (the GLPI and Snipe-IT compose stacks).

## Fix

Switch to `dataaxiom/ghcr-cleanup-action` (SHA-pinned), which is
manifest-list/referrer aware: it keeps the children +
signature/attestation referrers of every **tagged** image and only
prunes genuine orphans, partial pushes, and ghost tags.

> Images are being **republished** in parallel via a manual `build.yml`
run, so the tags pull again immediately; this PR prevents the next
weekly cleanup from re-breaking them.

(Same root cause + fix as netresearch/glpi-docker-compose-stack#7.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant