Add a script to verify the open security PRs against a deployment - #61
Open
setusher wants to merge 4 commits into
Open
Add a script to verify the open security PRs against a deployment#61setusher wants to merge 4 commits into
setusher wants to merge 4 commits into
Conversation
Every PR in the fix wave is checked in CI at the level of the files it
changes. None of them is checked against a deployment that has actually been
restarted, which is where a merged fix and a still-vulnerable server look
identical: an image that was not rebuilt, a container that was not recreated,
a reload that never happened.
`scripts/test/pr_verification.sh` covers fourteen of the sixteen open PRs, one
section per PR, following the conventions already in `test_auth_security.sh`
and `security_fixes.sh`. Sections are named after the pull request, so a
reviewer who has deployed one branch can check only that one:
./scripts/test/pr_verification.sh https://staging.example maintainer-site-15
Route tables were read out of each repository's `http_urls.py` and `config.yml`
rather than guessed, which matters more than it sounds: `registration` mounts
`create_session` and `set_pass` without trailing slashes, and a guessed slash
turns a working allow-list into a 404 that reads like a broken one.
Three sections carry checks that are worth more than a status code. The
maintainer-site section asserts that an anonymous `OPTIONS` still returns an
`actions` block, because DRF drops it when a permission denies the cloned
write request and the public Team and Alumni pages dereference
`options.actions.POST` in `render()` with no guard. The lost-and-found section
reads the list payload and looks for a row whose visibility flag is off but
which still carries an email address. The filemanager section calls `get_root`
with a session, which is the check that says whether the R Drive concern
blocking that PR is real against this data.
gym-pravesh#12 and marketplace#6 are deliberately not covered. Those
repositories are not readable with the account this was written against, so
their routes and permissions could not be established, and inventing checks
for them would produce something that looks authoritative and verifies
nothing.
Anything needing a logged-in caller is reported as a warning rather than a
pass when `SESSION_COOKIE` is unset, and the summary says in as many words
that warnings are checks which did not run.
`tests/test_pr_verification_script.py` runs the script against
`tests/fixtures/mock_pr_deployment.py` in a fixed and a vulnerable
configuration. It asserts every section executes, that no section produces a
shell error, and that on the vulnerable configuration the script reports each
specific defect by name: the open faculty write routes, the leaked visibility
arrays, the live `getItem` route, contact details behind a false flag, the
dropped `OPTIONS` actions and the anonymous user mutation. Measured across the
two, 8 failures against 25.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A __pycache__ directory was committed alongside the test file, which is build output pinned to one interpreter version and conflicts on every rebase. The noticeboard repository never had this problem because its .gitignore already carries the rule, so the rule is the fix rather than remembering not to stage it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…itself Same two defects as the sibling script, found by running it against staging. With every check skipped, `total` was zero, `FAILED` was zero, and the summary printed "ALL TESTS PASSED" and exited 0 while telling the reader in the same breath that warnings are not passes. A run that asserted nothing now prints "NO CHECKS RAN" and exits non-zero. The script can also obtain its own session from STAGING_USER and STAGING_PASSWORD, fetching a CSRF token from /api/ensure_csrf/ and posting to /api/session_auth/login/. Roughly half the per-PR checks need a logged-in caller, and requiring a cookie pasted out of a browser has already left them unrun twice. SESSION_COOKIE still wins when set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every path in this script went through $API, but core services are mounted at the root of the deployment and only apps declaring `isApi` in their configuration sit under /api/. Fifteen core paths were therefore requested somewhere NGINX does not route to Django, which answers 200 text/html from the React bundle, so the backend#227, formula-one#19 and backend#226 sections graded a page load rather than an endpoint. Verified against the deployment rather than assumed: /ensure_csrf/, /manifest/, /bootstrap/site_branding/, /session_auth/login/ and /kernel/who_am_i/ answer application/json unprefixed and 404 under /api/, while /api/people_search/student_search/ answers JSON and its unprefixed form returns the bundle. `url_for` resolves the base from the first path segment against a list of core prefixes, rather than each call site choosing, so a check added later cannot pick the wrong one by hand. The wider problem is that a status code cannot distinguish an endpoint from the frontend answering for it. An unrouted path is 200 text/html, which reads as success in any check expecting 200, and reads as a routing failure in one expecting 401. Both expect helpers now require application/json before grading, and a route answering HTML is reported as proving nothing. The fixture served core routes under /api/ and answered every request as JSON, so it modelled neither the mount points nor the fallback and could not have caught this. It now mirrors both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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
Adds
scripts/test/pr_verification.sh, which checks fourteen of the sixteen open security PRs against a running deployment over HTTP, one section per PR, plus a self-test that proves it still reports a deployment that has the problem.Issue
Every PR in the wave now has unit tests in CI, but all of them check a file - a permission declaration, a config fragment, a compose definition, a dependency pin. None checks a deployment that has actually been restarted.
That gap is precisely where a merged fix and a still-vulnerable server look identical:
Green CI, vulnerable server, and nothing in the repository can tell the difference.
test_auth_security.shinomniport-backendcovers the previous wave this way; this is the equivalent for the current one.Steps to reproduce the bug/issue
Tooling rather than a defect, so there is no bug to reproduce - but the gap is reproducible:
omniport-app-noticeboard#27and let its CI pass. The permission declarations are correct in the repository.curl https://<host>/api/noticeboard/new/with no session still returns 200 and every notice.Nothing in any repository can distinguish that state from a correct deploy.
Steps done to fix it and test added for the same
The script. One section per PR, named after it:
Routes were read, not guessed. Every path comes from the relevant repository's
http_urls.pyandconfig.yml. This matters more than it sounds -registrationdeclarescreate_sessionandset_passwithout trailing slashes, so a guessed slash produces a 404 that reads exactly like a broken allow-list.Three sections do more than compare a status code:
maintainer-site-15asserts an anonymousOPTIONSstill returns anactionsblock.SimpleMetadata.determine_actionsclones the request as PUT/POST and runscheck_permissions; a permission that denies anonymous writes makes DRF drop the key entirely, andteam-page.js,alumni-page.jsandteam-individual-view.jsall dereferenceoptions.actions.POSTinrender()with no guard. That is a white-screened public site, and it is invisible to a status-code check.lost-and-found-7reads the list payload and looks for any row whosecontactVisibleis false but which still carries anemailAddressorprimaryPhoneNumber- the exact shape of the original leak.filemanager-80callsget_rootwith a session. That is the check that answers whether the R Drive concern blocking that PR is real against this data, rather than in principle.A 5xx is a distinct failure class, not just "wrong code". Several of these findings fail closed - the non-object JSON body in the groups permission class, the bare
exceptin the filemanager - so collapsing a 500 into "not 403" would hide them.Two PRs are deliberately not covered.
gym-pravesh#12andmarketplace#6live in repositories that are not readable with the account this was written against, so their routes and permissions could not be established. Inventing checks for them would produce something that looks authoritative and verifies nothing.Test.
tests/test_pr_verification_script.pyruns the real script againsttests/fixtures/mock_pr_deployment.pyin both configurations:TestTheScriptIsRunnablebash -n, every listed section has a function, unknown sections refusedTestItRunsWithoutShellErrorscommand not found,unary operator expected,unbound variable- thingsbash -ncannot see inside a functionTestItReportsAVulnerableDeploymentgetItem, contact details behind a false flag, droppedOPTIONSactions, anonymous user mutationTestItPassesTheClosedChecksWhenFixedMeasured across the two configurations:
15 tests, all passing.
CI.
.github/workflows/pr-verification-tests.ymlruns the self-test on every push and pull request. It is a separate workflow and fixture filename from #58, #59 and #60, so none of these conflict.scripts/README.mdgains an entry alongside the existing scripts.Criteria for issue to be resolved
bash -n scripts/test/pr_verification.shis clean./scripts/test/pr_verification.sh <url> listprints all fourteen sectionsSESSION_COOKIE, authenticated checks are reported as warnings rather than passesgym-pravesh#12andmarketplace#6are documented as uncovered rather than silently missingNote on the two gaps
If read access to
omniport-app-gym-praveshandomniport-app-marketplaceis granted, both sections can be added -marketplace#6in particular, since the non-object JSON body defect it fixes is the same one thegroups-8section already probes for.