Skip to content
This repository was archived by the owner on Aug 25, 2026. It is now read-only.

Sp7 stats fixes #5

Open
acwhite211 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Sp7 stats fixes #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
681888b
Update nginx service volumes in docker-compose
acwhite211 Sep 9, 2025
ebffd4e
Refactor isa_number assignment with null coalescing
acwhite211 Sep 9, 2025
132c89c
Modify Nginx config for HTTPS and access logging
acwhite211 Sep 9, 2025
e0dfda1
Update require_once path in refresh_data.php
acwhite211 Sep 9, 2025
5f640b7
Update require_once paths to use dirname function
acwhite211 Sep 9, 2025
70bfc0a
fix: merge remote changes
grantfitzsimmons Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@ version: '3.9'
services:
nginx:
build: https://github.com/specify/nginx-with-github-auth.git#main
# build:
# context: ~/nginx-with-github-auth
ports:
- '80:80'
- '443:443'
volumes:
- './sp7-stats/config/auth.conf:/etc/nginx/auth.conf:ro'
- './sp7-stats/config/nginx.conf:/etc/nginx/conf.d/default.conf:ro'
- './sp7-stats/:/var/www/:ro'
- './sp7-stats/config/fullchain.pem:/etc/letsencrypt/live/sp7-stats/fullchain.pem:ro'
- './sp7-stats/config/privkey.pem:/etc/letsencrypt/live/sp7-stats/privkey.pem:ro'
- '/etc/letsencrypt:/etc/letsencrypt:ro'
- './access-logs/:/var/log/nginx/external/:rw'
networks:
- nginx
depends_on:
Expand All @@ -33,6 +35,20 @@ services:
networks:
- nginx

certbot:
image: certbot/certbot
container_name: certbot
volumes:
- '/etc/letsencrypt:/etc/letsencrypt'
- './sp7-stats/:/var/www/certbot/'
- '/var/run/docker.sock:/var/run/docker.sock'
entrypoint: >
sh -c "trap exit TERM; while :; do certbot renew --webroot -w /var/www/certbot --deploy-hook 'docker exec sp7-stats-nginx-1 nginx -s reload'; sleep 12h; done"
Comment on lines +38 to +46

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Add an initial certificate bootstrap path.

Line 46 only runs certbot renew, which requires an existing cert. On a fresh Ubuntu 24 host with empty /etc/letsencrypt, nginx will try to load the live cert files before certbot can create them, so startup can fail. Add a one-time certbot certonly --webroot ... -d stats.specifycloud.org -d sp7-stats.specifycloud.org bootstrap or a documented temp-cert flow before nginx starts.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker-compose.yml` around lines 38 - 46, The certbot container in
docker-compose only runs the certbot renew loop, so it cannot create the first
certificate on a fresh host. Update the certbot service entrypoint to include an
initial bootstrap step in addition to the existing renew behavior, using certbot
certonly with the webroot path and the required domains before nginx depends on
the live certificate files. Keep the existing renew/deploy-hook logic in the
certbot service, and reference the certbot service and its entrypoint command
when making the change.

Comment on lines +44 to +46

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the hard-coded deploy-hook target used by certbot.
# Expect: the running nginx container name exactly matches sp7-stats-nginx-1.
docker compose ps nginx
docker container inspect sp7-stats-nginx-1 --format '{{.Name}} {{.State.Running}}'

Repository: specify/sp7-stats

Length of output: 239

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git ls-files docker-compose.yml
wc -l docker-compose.yml
sed -n '1,140p' docker-compose.yml
printf '\n-- references --\n'
rg -n "sp7-stats-nginx-1|docker.sock|certbot renew|nginx -s reload|certbot" -S .

Repository: specify/sp7-stats

Length of output: 2290

🏁 Script executed:

#!/bin/bash
set -euo pipefail

wc -l docker-compose.yml
sed -n '1,140p' docker-compose.yml
printf '\n-- references --\n'
rg -n "sp7-stats-nginx-1|docker.sock|certbot renew|nginx -s reload|certbot" -S .

Repository: specify/sp7-stats

Length of output: 2271

Remove Docker socket access from certbot. The deploy hook also hard-codes sp7-stats-nginx-1; use a host-managed reload, a restricted socket proxy, or a dedicated reload sidecar instead.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker-compose.yml` around lines 44 - 46, The certbot service is still
mounting the Docker socket and using a hard-coded nginx container name in the
deploy hook, which should be removed. Update the certbot entrypoint so
certificate renewal triggers a host-managed reload, a restricted socket proxy,
or a dedicated reload sidecar instead of calling docker exec directly. Keep the
change localized to the certbot service configuration and ensure any reload
mechanism does not depend on sp7-stats-nginx-1 or unrestricted socket access.

networks:
- nginx
depends_on:
- nginx

networks:
nginx:

Expand Down
5 changes: 3 additions & 2 deletions sp7-stats/components/institutions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ function compile_institutions($lines_data, $file_name){
$institution = $line_data['institution'];
$discipline = $line_data['discipline'];
$collection = $line_data['collection'];
$isa_number = $line_data['isaNumber'];
// $isa_number = $line_data['isaNumber'];
$isa_number = $line_data['isaNumber'] ?? '';
$browser = $line_data['browser'];
$domain = $line_data['domain'];
$os = $line_data['os'];
Expand Down Expand Up @@ -293,4 +294,4 @@ function sort_months($x,$y){
file_put_contents(WORKING_LOCATION.'institutions_id.json',json_encode($institutions4));
file_put_contents(WORKING_LOCATION.'institutions.json',json_encode($institutions3));

}
}
92 changes: 67 additions & 25 deletions sp7-stats/config/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,81 @@
# Outbound DNS for Lua http requests
# resolver 1.1.1.1 8.8.8.8 valid=300s;
# resolver_timeout 5s;

# Trust store path on Alpine (nginx:alpine puts the bundle here)
# lua_ssl_trusted_certificate /etc/ssl/cert.pem;
# lua_ssl_verify_depth 5;

# See https://github.com/specify/nginx-with-github-auth
include nginx-with-github-auth/http.conf;

# Redirect HTTP to HTTPs
# --- HTTP (port 80) ---
server {
listen 80 default_server;
server_name _;
return 301 https://$host$request_uri;
listen 80 default_server;
listen [::]:80 default_server;

server_name stats.specifycloud.org sp7-stats.specifycloud.org;

# Serve ACME challenge over HTTP without redirect
location ^~ /.well-known/acme-challenge/ {
root /var/www/;
try_files $uri =404;
}

# Everything else: redirect to HTTPS
location / {
return 301 https://$host$request_uri;
}
}

# --- HTTPS (port 443) ---
server {
listen 443 ssl default_server;
listen 443 ssl default_server;
listen [::]:443 ssl default_server;

server_name stats.specifycloud.org sp7-stats.specifycloud.org;

# Use the certbot-standard live path for THIS domain name
ssl_certificate /etc/letsencrypt/live/sp7-stats.specifycloud.org/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/sp7-stats.specifycloud.org/privkey.pem;

ssl_session_cache shared:SSL:10m;
ssl_session_timeout 5m;
ssl_prefer_server_ciphers on;

include auth.conf;
include nginx-with-github-auth/server.conf;

ssl_certificate /etc/letsencrypt/live/sp7-stats/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/sp7-stats/privkey.pem;
ssl_session_cache shared:SSL:10m;
ssl_session_timeout 5m;
ssl_prefer_server_ciphers on;
index index.php index.html;
keepalive_timeout 70;
error_log /var/log/nginx/error.log;
access_log /var/log/nginx/access.log;
root /var/www/;

include auth.conf;
include nginx-with-github-auth/server.conf;
access_log /var/log/nginx/external/access.log combined;

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep the external stats log scoped to /capture.

Line 55 mirrors every HTTPS request into the host-mounted external log, while raw_data.php only consumes /capture? entries. This persists unrelated app/auth URLs and query strings and can bloat the stats input. Keep the external log only on Line 69.

Proposed change 
-    access_log /var/log/nginx/external/access.log combined;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
access_log /var/log/nginx/external/access.log combined;
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sp7-stats/config/nginx.conf` at line 55, The nginx access_log configuration
is too broad because it mirrors all HTTPS traffic into the external host-mounted
log instead of only the stats capture endpoint. Update the server/location block
around access_log so that the `/capture`-scoped logging remains enabled in the
`capture` handling path, and remove or disable the generic external access_log
entry from the main HTTPS scope. Use the existing nginx config sections and the
`/capture` location as the unique anchors when making the change.


index index.php index.html;
server_name sp7-stats;
keepalive_timeout 70;
error_log /var/log/nginx/error.log;
access_log /var/log/nginx/access.log;
root /var/www/;
location = /capture {
# Allow browser JS to call this cross-origin
add_header 'Access-Control-Allow-Origin' '*' always;
add_header 'Access-Control-Allow-Methods' 'GET, OPTIONS' always;
add_header 'Access-Control-Allow-Headers' 'Content-Type, Authorization' always;
add_header 'Access-Control-Max-Age' 86400 always;

location ~ ^/.+\.php$ {
include nginx-with-github-auth/location.conf;
# Handle preflight (some browsers/frameworks trigger this)
if ($request_method = OPTIONS) {
return 204;
}

include fastcgi_params;
fastcgi_pass php:9000;
fastcgi_index index.php;
fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
}
access_log /var/log/nginx/external/access.log combined;
add_header Content-Type text/plain;
return 204;
}

location ~ ^/.+\.php$ {
include nginx-with-github-auth/location.conf;
include fastcgi_params;
fastcgi_pass php:9000;
fastcgi_index index.php;
fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
}
}
3 changes: 2 additions & 1 deletion sp7-stats/cron/refresh_data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
global $no_gui;
$no_gui = TRUE;

require_once('../refresh_data/index.php');
//require_once('../refresh_data/index.php');
require_once __DIR__ . '/../refresh_data/index.php';
Loading