gstone/nextcloud-selfhosted
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix more issues

Browse Source
This commit is contained in:
Thomas Gräfenstein
2026-03-22 12:29:58 +01:00
parent caa1c7f471
commit 89b806fd5b
9 changed files with 104 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
review.md
View File

@@ -1,14 +1,16 @@
# Code Review Issues
# Repo Review — nextcloud-selfhosted
| # | Severity | File | Issue | Status |
|----|----------|---------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------|
| 1 | Critical | `scripts/deploy.sh` | `SCRIPT_DIR` resolves to `scripts/` but paths assume repo root (e.g. `$SCRIPT_DIR/caddy/docker-compose.yml`). All scripts broken after move to `scripts/`. Fix: use `REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"` | DONE |
| 2 | Critical | `scripts/backup.sh` | Same broken `SCRIPT_DIR` path issue | DONE |
| 3 | Critical | `scripts/restore.sh` | Same broken `SCRIPT_DIR` path issue | DONE |
| 4 | High | `scripts/backup.sh:20` | `pg_dumpall -U nextcloud` hardcodes DB username instead of reading from env | DONE |
| 5 | High | `scripts/restore.sh:68` | `psql -U nextcloud` hardcodes DB username instead of reading from env | DONE |
| 6 | High | `scripts/deploy.sh:13` | `source .env` in a root-privileged script can execute arbitrary commands. Consider safer parsing or variable validation | DONE |
| 7 | Medium | `monitoring/docker-compose.yml` | Docker socket + `/proc` + `/sys` + `/` mounted into Alloy container. Consider using a Docker socket proxy to limit API access | DONE |
| 8 | Medium | `caddy/Caddyfile` | No rate limiting configured at the reverse proxy layer | DONE |
| 9 | Low | `gitea/docker-compose.yml` | `gitea/gitea:latest-rootless` unpinned — pin to specific version like Nextcloud does | DONE |
| 10 | Low | `monitoring/docker-compose.yml` | `grafana/alloy:latest` unpinned — pin to specific version | DONE |
| # | Priority | Category | Issue | Location | Suggestion | Status |
|----|----------|-------------|-----------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------|---------|
| 1 | High | Security | `backup.sh` and `restore.sh` use `source` to load `.env` files, which executes arbitrary shell code | `scripts/backup.sh:5-6`, `scripts/restore.sh:5-6` | Replace `source` with the safe `eval "$(grep ...)"` parser used in `deploy.sh:14` | DONE |
| 2 | High | Correctness | Cron path hint is wrong — says `$REPO_ROOT/backup.sh` instead of `$REPO_ROOT/scripts/backup.sh` | `scripts/backup.sh:49` | Change to `$REPO_ROOT/scripts/backup.sh` | DONE |
| 3 | Medium | Correctness | Postgres readiness check uses `sleep 5` instead of a proper wait | `scripts/restore.sh:66` | Use `docker compose up -d --wait postgres` or poll with `pg_isready` in a loop | DONE |
| 4 | Medium | Correctness | `pg_dumpall` output restored with `psql -U $POSTGRES_USER` — role creation statements may fail | `scripts/restore.sh:69` | Restore against the `postgres` database: `psql -U "$POSTGRES_USER" -d postgres` | DONE |
| 5 | Medium | Reliability | No Docker log rotation — JSON log driver can fill disk | All `docker-compose.yml` files | Add `logging: { driver: json-file, options: { max-size: "10m", max-file: "3" } }` to each service, or configure in `/etc/docker/daemon.json` | DONE |
| 6 | Medium | Security | Alloy container mounts entire root filesystem (`/:/host/root:ro`) — exposes secrets in `.env` files | `monitoring/docker-compose.yml:42` | Mount only needed paths (e.g., `/etc:/host/etc:ro`) or use a more restrictive bind | SKIPPED |
| 7 | Medium | Reliability | Rate limits mentioned in commit `0f12c5f` but not present in Caddyfile | `caddy/Caddyfile` | Add `rate_limit` directive or verify the commit wasn't partially reverted | SKIPPED |
| 8 | Low | Backup | Caddy TLS certificates (`${DATA_ROOT}/caddy/data/`) not included in backup | `scripts/backup.sh` | Add a `tar` step for `caddy/data` — avoids Let's Encrypt rate limits on restore | DONE |
| 9 | Low | Reliability | `deploy.sh` doesn't pull latest images before starting | `scripts/deploy.sh:78-88` | Add `docker compose pull` before each `up -d` call | DONE |
| 10 | Low | Security | Redis has no password — reachable from any container on `nextcloud-internal` network | `nextcloud/docker-compose.yml:38-42` | Add `command: redis-server --requirepass $REDIS_PASSWORD` and pass the password to Nextcloud via `REDIS_HOST_PASSWORD` | DONE |
| 11 | Low | Reliability | No healthchecks on Nextcloud, Gitea, or Caddy containers | `nextcloud/docker-compose.yml`, `gitea/docker-compose.yml`, `caddy/docker-compose.yml` | Add `healthcheck` blocks (e.g., `curl -f http://localhost` for Nextcloud, `caddy validate` for Caddy) | DONE |
| 12 | Low | Reliability | No container resource limits — a runaway process can OOM the VPS | All `docker-compose.yml` files | Add `mem_limit` and `cpus` to at least Nextcloud, Postgres, and Alloy | SKIPPED |