Add Forkana Docker deployment configuration#128
Add Forkana Docker deployment configuration#128pedrogaudencio wants to merge 17 commits intomasterfrom
Conversation
* create Dockerfile based on rootless variant
pedrogaudencio
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/crush_fast AI review started. |
Advanced AI Review
Click to expand reviewCode Review: Forkana Docker Deployment ConfigurationIssue 1: Overly Permissive Trusted Proxies Configuration 🔴
Location: REVERSE_PROXY_TRUSTED_PROXIES = *Problem: Setting Recommendation: Restrict to known proxy networks. Since nginx is on the same host: REVERSE_PROXY_TRUSTED_PROXIES = 127.0.0.0/8,::1/128,172.16.0.0/12,10.0.0.0/8Or if the nginx proxy is in a Docker network, use the appropriate CIDR range. Issue 2: Contradictory SSH Configuration 🔴
Location: DISABLE_SSH = $DISABLE_SSH
; In rootless container only internal ssh server is supported
START_SSH_SERVER = trueProblem: The template always sets Recommendation: Make DISABLE_SSH = $DISABLE_SSH
START_SSH_SERVER = $START_SSH_SERVERAnd in DISABLE_SSH=${DISABLE_SSH:-"true"} \
START_SSH_SERVER=${START_SSH_SERVER:-"false"} \Issue 3: Unnecessary Runtime Dependency on Registry 🟡
Location: depends_on:
postgres:
condition: service_healthy
registry:
condition: service_startedProblem: The Recommendation: Remove the registry dependency: depends_on:
postgres:
condition: service_healthyIssue 4: Missing Modern Security Headers in nginx 🟡
Location: # Security headers
add_header Strict-Transport-Security "max-age=63072000; includeSubDomains" always;
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-Content-Type-Options "nosniff" always;
add_header X-XSS-Protection "1; mode=block" always;
add_header Referrer-Policy "strict-origin-when-cross-origin" always;Problem: Missing Recommendation: Add modern security headers: # Security headers
add_header Strict-Transport-Security "max-age=63072000; includeSubDomains" always;
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-Content-Type-Options "nosniff" always;
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self';" always;
add_header Permissions-Policy "geolocation=(), microphone=(), camera=()" always;Issue 5: Missing INTERNAL_TOKEN Configuration 🟡
Location: Problem: Recommendation: Add to # Internal token for API communication (generate with: gitea generate secret INTERNAL_TOKEN)
FORKANA_INTERNAL_TOKEN=your-internal-token-hereAdd to GITEA__security__INTERNAL_TOKEN: ${FORKANA_INTERNAL_TOKEN}Issue 6: Database Password Logging Risk in Template 🟡
Location: PASSWD = $DB_PASSWDProblem: The database password is written to Recommendation: This is consistent with upstream Gitea behavior, but consider documenting that the config volume ( Issue 7: Potentially Fragile Array Handling in gitea Wrapper ⚪️
Location: if [ -z "$APP_INI_SET" ]; then
CONF_ARG=("-c" "${GITEA_APP_INI:-$APP_INI}")
fi
# Provide docker defaults
GITEA_WORK_DIR="${GITEA_WORK_DIR:-$WORK_DIR}" exec -a "$0" "$GITEA" "${CONF_ARG[@]}" "$@"Problem: If Recommendation: Initialize the array at declaration: CONF_ARG=()
APP_INI_SET=""
for i in "$@"; do
case "$i" in
"-c")
APP_INI_SET=1
;;
# ... rest of cases
esac
done
if [ -z "$APP_INI_SET" ]; then
CONF_ARG=("-c" "${GITEA_APP_INI:-$APP_INI}")
fiIssue 8: Hardcoded Domain in nginx.conf ⚪️
Location: server_name dev.forkana.org;
...
ssl_certificate /etc/letsencrypt/live/dev.forkana.org/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/dev.forkana.org/privkey.pem;Problem: The domain Recommendation: The DEPLOYMENT_GUIDE already documents the server_name YOUR_DOMAIN_HERE; # Replace with your actual domainOr create an Issue 9: Missing OAuth2 JWT_SECRET Configuration ⚪️
Location: Problem: OAuth2 functionality requires a Recommendation: Add to Summary
The deployment configuration is generally well-structured with good practices like:
The most critical issues to address are Issue 1 (trusted proxies) and Issue 2 (SSH configuration contradiction). Review generated using |
…r, systemd WorkingDirectory * add to Dockerfile custom-defaults loop so Forkana CSS/images are baked in * change nginx.conf placeholder from dev.forkana.org to dev.forkana.example (RFC 2606), making the deployment guide's sed command match correctly * fix systemd WorkingDirectory: ~/... → %h/... (systemd does not expand tilde)
* add FORKANA_INTERNAL_TOKEN and FORKANA_JWT_SECRET to .env.example and wire them via GITEA__ env overrides in dev.yml * initialize CONF_ARG=() before arg-parsing loop in gitea wrapper to prevent errors under set -u
|
/crush_fast AI review started. |
Advanced AI Review
Click to expand reviewI'll review these Docker deployment changes thoroughly. Let me first examine some existing files in the codebase to understand the context and check for any potential conflicts.Now let me check if there are any existing deployment-related scripts that might conflict:Now I have a comprehensive understanding of the codebase. Let me review the changes in detail: Code Review: Forkana Docker Deployment ConfigurationIssue 1: Potential postgres volume path issue when HOME is not set
Rating: 🔴 High importance File: volumes:
- ${HOME}/forkana/postgres:/var/lib/postgresql/dataThe postgres volume uses Suggestion: Use volumes:
- forkana-postgres:/var/lib/postgresql/data
# Under volumes section:
volumes:
forkana-postgres:This also improves portability and avoids issues with permission mismatches. Issue 2: Missing GITEA_APP_INI environment variable in compose file
Rating: 🟡 Medium importance File: The entrypoint script references exec /usr/local/bin/gitea -c ${GITEA_APP_INI} webThe environment variable is set in the Dockerfile ( Issue 3: Inconsistent variable expansion in envsubst - DOMAIN not defaulted consistently
Rating: 🟡 Medium importance File: ROOT_URL=${ROOT_URL:-"http://${DOMAIN:-localhost}:${HTTP_PORT:-3000}/"} \The DOMAIN=${DOMAIN:-"localhost"} \The ordering is correct (DOMAIN is set before ROOT_URL in the same multi-line command), but this pattern is fragile. If someone accidentally reorders the lines, ROOT_URL=${ROOT_URL:-"http://localhost:3000/"} \Or document the ordering requirement clearly. Issue 4: Comment typo in docker-entrypoint.sh
Rating: ⚪️ Low importance File: # Protect against buggy runc in docker <20.10.6 causing problems with Alpine >= 3.14The original Gitea comment had a typo ("problems in with") that was fixed in the Forkana version. This is actually correct in the PR. No change needed. Issue 5: Registry image digest pinning - verify SHA256
Rating: ⚪️ Low importance (verification) File: image: registry:3.0.0@sha256:6c5666b861f3505b116bb9aa9b25175e71210414bd010d92035ff64018f9457eThis is good practice (digest pinning). I cannot verify the digest matches the official registry:3.0.0 image without network access. Recommend verifying against Docker Hub or the official registry repository. Issue 6: DB_PASSWD empty default in setup script
Rating: 🟡 Medium importance File: DB_PASSWD=${DB_PASSWD:-""} \The empty default for DB_PASSWD=${DB_PASSWD:-""}
if [ "$DB_TYPE" = "postgres" ] && [ -z "$DB_PASSWD" ]; then
echo "WARNING: DB_PASSWD is empty but DB_TYPE is postgres"
fiOr require it to be set: if [ "$DB_TYPE" = "postgres" ] && [ -z "$DB_PASSWD" ]; then
echo "ERROR: DB_PASSWD must be set when using PostgreSQL"
exit 1
fiIssue 7: nginx.conf missing Content-Security-Policy header
Rating: 🟡 Medium importance File: # Security headers
add_header Strict-Transport-Security "max-age=63072000; includeSubDomains" always;
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-Content-Type-Options "nosniff" always;
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
add_header Permissions-Policy "geolocation=(), microphone=(), camera=()" always;Consider adding a Content-Security-Policy header for additional protection against XSS attacks. Since Gitea/Forkana is a complex application with inline scripts, a restrictive CSP might break functionality, but at minimum consider: add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self';" always;Test thoroughly as this may break certain features. Issue 8: Deploy script duplicate code between Debian and Fedora versions
Rating: ⚪️ Low importance (maintainability) Files: The two deploy scripts share approximately 90% of their code. Only the package installation section (step 0 in Fedora, absent in Debian) differs. Consider extracting common logic to a shared library: # deploy_common.sh - sourced by both deploy scripts
log() { printf '[deploy %s] %s\n' "$(date -u +%Y-%m-%dT%H:%M:%SZ)" "$*"; }
die() { log "FATAL: $*"; exit 1; }
# ... rest of shared functionsThen each OS-specific script sources it and adds only its package-specific logic. However, this adds complexity and the current approach works fine for two scripts. Issue 9: FORKANA_INTERNAL_TOKEN and FORKANA_JWT_SECRET generation commands not provided
Rating: 🟡 Medium importance File: # Internal token for API communication between Gitea processes
# (generate with: gitea generate secret INTERNAL_TOKEN)
FORKANA_INTERNAL_TOKEN=
# OAuth2 JWT secret for token signing
# (generate with: gitea generate secret JWT_SECRET)
FORKANA_JWT_SECRET=The comments reference Consider providing fallback generation commands in the comments: # Internal token for API communication between Gitea processes
# Generate with: head -c 32 /dev/urandom | base64
FORKANA_INTERNAL_TOKEN=Issue 10: Potential race condition in health check loop
Rating: ⚪️ Low importance File: for i in $(seq 1 30); do
if curl -sf http://127.0.0.1:3000/api/healthz > /dev/null 2>&1; then
log "Service is healthy!"
log "Deployment of ${COMMIT_SHA} complete."
exit 0
fi
log " attempt ${i}/30 — not ready yet"
sleep 5
doneThis is fine for the intended use case. The 150-second timeout should be sufficient for most deployments. No change needed. Issue 11: PostgreSQL password exposure in environment
Rating: 🟡 Medium importance (informational) File: DB_PASSWD: ${POSTGRES_PASSWORD}The PostgreSQL password is passed as an environment variable which is visible in Issue 12: dev.yml missing explicit container_name for postgres
Rating: ⚪️ Low importance File: postgres:
image: postgres:14-alpine
container_name: forkana-postgresActually, this is present. No issue. Issue 13: Missing restart policy for registry could cause orphaned containers
Rating: ⚪️ Low importance File: restart: unless-stoppedThe registry has a proper restart policy. No issue. Issue 14: DEPLOYMENT_GUIDE.md contains sensitive example values that could be copy-pasted
Rating: 🟡 Medium importance File: The guide provides shell commands to generate the .env file. However, the domain echo "FORKANA_DOMAIN=dev.forkana.org" >> $DEPLOY_HOME/forkana/compose/.envSomeone might run this verbatim and end up with an incorrect domain. Consider using a placeholder that's more obviously wrong: echo "FORKANA_DOMAIN=your-domain.example" >> $DEPLOY_HOME/forkana/compose/.envIssue 15: Potential issue with .env file location in compose
Rating: ⚪️ Low importance (documentation) File: The guide creates Issue 16: Dockerfile missing LABEL for version/version control
Rating: ⚪️ Low importance File: LABEL maintainer="Forkana Maintainers <maintainers@forkana.org>"Consider adding OCI image labels for better image provenance: LABEL org.opencontainers.image.source="https://github.com/okTurtles/forkana" \
org.opencontainers.image.description="Forkana - a fork of Gitea for encyclopedia articles" \
org.opencontainers.image.licenses="MIT"Summary
The most important issue is Issue 1 regarding the postgres volume path. The deployment configuration is generally well-structured with good security practices including digest pinning, SSH forced-command restrictions, and proper secret handling documentation. The main areas for improvement are around defensive coding for missing environment variables and some documentation enhancements to prevent copy-paste errors. Review generated using |
* use Docker's official repo and DNF5 syntax for Fedora 41+ installs * add step to verify/fix deploy user home directory ownership (common Fedora issue with --system users) * replace tilde (~) paths with explicit from getent passwd and use sudo -Hiu for correct HOME resolution
Closes /issues/111
Co-authored by: Claude Opus 4.6, Sonnet 4.6 and GLM-5