Require admin auth on /scripts/ database-write endpoints#89
Open
santichausis wants to merge 1 commit into
Open
Require admin auth on /scripts/ database-write endpoints#89santichausis wants to merge 1 commit into
santichausis wants to merge 1 commit into
Conversation
These are internal data-migration/admin tools (ecobrick and training processing, DB delete of training records) with no link from any public page or JS, yet were reachable by anyone who found the URL and would execute INSERT/UPDATE/DELETE against the GoBrik database with no auth check at all. Adds includes/require-admin.php, a shared gate that checks a token (?admin_token=... or POST field) against a secret defined in a new admin_env.php file at the repo root -- not committed, following the same convention as ecobricks_env.php/gobrikconn_env.php. Once validated, the token is remembered in the session so the existing multi-step migration flows (these scripts redirect to each other) don't need to re-authenticate every request. Left untouched: carbon_check.php (fetched directly by public JS, js/website-carbon-badges.js) and the read-only ajax-*/get-*/ training-detail.php/ssp.class.php scripts, which mirror the site's public "Open Books" transparency APIs under /api/ and aren't referenced in the reported vulnerability.
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
1 participant
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
/scripts/contains internal data-migration/admin tools:process_ecobrick-full.php,process_ecobrick-old.php,process_ecobrick.php,process_ecobrick_for_maker_id.php,process_training.php,process_training2.php,process_training3.php,update-training-database.php, andtraining.php(which has aDELETE FROM tb_trainingsaction). None of these are linked from any public page or referenced by any front-end JS (verified by grepping the whole repo for their filenames), yet they had zero authentication — anyone who found/guessed the URL could trigger INSERT/UPDATE/DELETE against the live GoBrik database.This PR adds a shared gate,
includes/require-admin.php:?admin_token=...or POST fieldadmin_token) matched withhash_equals()againstADMIN_TOKEN, defined in a newadmin_env.phpfile at the repo root.admin_env.phpis not committed — it already matches the repo's existing*_env.phpgitignore rule, same pattern asecobricks_env.php/gobrikconn_env.php.$_SESSIONso the existing multi-step flows (these scripts redirect to each other, e.g.process_training2.php→training.php?training_id=...) don't need the token on every request.admin_env.phpdoesn't exist on the server, every gated script 503s instead of silently allowing access.You'll need to create
admin_env.phpon the server (and locally if testing) with:Left untouched, on purpose
scripts/carbon_check.php— fetched directly by public JS (js/website-carbon-badges.js), must stay open.scripts/ajax-*.php,scripts/get-*.php,scripts/training-detail.php,scripts/ssp.class.php— all read-only (no INSERT/UPDATE/DELETE), and appear to mirror the site's public "Open Books" transparency APIs under/api/. Didn't want to guess at gating these without confirming with maintainers whether they're meant to be public, since gating them could break the transparency features the org is built around. Happy to do a follow-up PR if you'd like these locked down too.Testing
No test suite/CI in this repo. Verified the gate itself in isolation with PHP's built-in server (
php -S) against a throwaway fixture mirroring the real include structure:403 Forbidden403 Forbidden?admin_token=...→ passes through, sets sessionAlso ran
php -lon all 10 changed/added files — no syntax errors.Note: an earlier draft of
require-admin.php's doc comment had a<?php ... ?>code example inside a//comment — PHP closes tag mode on a literal?>even inside a line comment, which silently turned the entire rest of the file into inert HTML output (no fatal error,php -lpasses, but the auth check never runs). Caught this by actually exercising the gate end-to-end rather than relying on lint alone; the shipped version avoids embedding a literal closing tag in the comment.