Protect experiment reporting with privileged auth and bounded inputs - #38
Merged
Conversation
GET /api/v1/experiments/report and POST /api/v1/experiments/report/export had no authentication dependency, letting any network caller scan every student profile and trigger a file write, contrary to the documented "every /api/v1/* route requires an API key" policy. - Add agent.auth.require_researcher (admin or researcher role) and gate both routes on it via a new experiments sub-router, so the policy is centralized at the router boundary and inherited by future routes added under /experiments. - Bound retention_days to 1-365 via FastAPI Query validation on both routes, rejected with 422 before any profile scan or file write. - Return only the exported artifact's filename instead of the host filesystem path. - Document the role requirement in the README. - Add tests/test_api_security.py: auth/role coverage for the experiment routes, retention_days bounds, and a route-table audit that fails if a future /api/v1 route is added without an auth dependency (with an explicit allowlist for the two routes that are intentionally public). Closes DogStark#21
llinsss
approved these changes
Aug 22, 2026
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
GET /api/v1/experiments/reportandPOST /api/v1/experiments/report/exporthad no authentication dependency, even though the README documents that every/api/v1/*route requires a bearer API key. Any unauthenticated caller could trigger a full scan of every student profile and force a file write to disk. This PR closes that gap: both routes now require a privileged (admin/researcher) account,retention_daysis bounded, exports no longer leak a host filesystem path, and a new route-table test guards against a future route being added without auth.Closes #21 — Protect experiment reporting with privileged auth and bounded inputs
Problem:
get_experiment_reportandexport_experiment_reportinapi/routes.pyhad noDepends(...)auth dependency at all. The report scans all student profiles; the export additionally writes derived data to disk. Both were reachable by any network caller with no credentials, andretention_dayswas an unboundedintquery parameter.Required behavior (from the issue):
401; authenticated but unprivileged accounts →403; a privileged account can retrieve/export the report.retention_daysbounded (1–365) with422on invalid values, and no profile scan/file write on an invalid value./api/v1routes and enforce the auth policy, with public endpoints explicitly allowlisted.What changed:
agent/auth.py: addedrequire_researcher, a FastAPI dependency requiring theadminorresearcherrole (authorize_role(account, {"admin", "researcher"})), mirroring the existingrequire_adminpattern used for curriculum management.api/routes.py:experiments_routersub-router (prefix="/experiments") withdependencies=[Depends(require_researcher)]set at the router level, and moved both experiment endpoints onto it, thenrouter.include_router(experiments_router). Because the dependency lives on the sub-router rather than each endpoint, any future route added under/experimentsinherits the privileged-role requirement automatically.retention_dayschanged from a bareintdefault toQuery(default=DEFAULT_RETENTION_DAYS, ge=1, le=365)on both routes — FastAPI validates and rejects out-of-range/non-integer values with422before the handler body (and therefore the profile scan or file write) ever runs.{"exported_file": os.path.basename(path)}instead of{"exported_to": path}, so only the artifact's filename is exposed, never the full host path.README.md: documented that curriculum-management and experiment-reporting routes require a privilegedadmin/researcheraccount rather than any parent/teacher account, documented theretention_daysbounds, and updated the export example response to the newexported_fileshape.Tests (
tests/test_api_security.py, new file):TestExperimentReportAuth: missing credentials →401; invalid credentials →401; parent/teacher accounts →403; admin and researcher accounts →200on both report and export; export response contains only a bare filename (never a path separator).TestRetentionDaysValidation: out-of-range (0,-1,366,10000) and non-integerretention_days→422; a monkeypatchedcompute_variant_metrics/export_experiment_report_jsonthat raisesAssertionErrorif called proves an invalidretention_daysnever reaches the profile scan or file write; boundary values1and365are accepted.TestRouteAuthPolicy: walks the FastAPI dependant tree of every registered/api/v1/*route (including router-leveldependencies=) and assertsrequire_accountis present, failing the test if a future route is added with no auth dependency, except for an explicitPUBLIC_API_V1_PATHSallowlist (the two routes that are already, and intentionally, unauthenticated today —POST /api/v1/hintandGET /api/v1/neighbors/{word}— which are out of scope for this issue). A companion test also asserts the two experiment routes specifically resolverequire_researcher, and another guards the allowlist itself against going stale.Verification
Ran with Python 3.14 in a clean venv against
requirements-dev.txt(the project's declared range is 3.11–3.12; CI runs those versions — see note below):Known Pre-existing Issues
Two pre-existing test failures are Windows-only environment artifacts, unrelated to this change (neither touches auth, experiments, or reporting code):
tests/test_concurrency.py::TestConcurrency::test_concurrent_attempt_same_studentandtest_concurrent_storage_read_write—PermissionErrorfromos.replaceracing on a hardcoded/tmp/...path under concurrent writes on Windows (this repo's dev container is Linux in CI; these pass there).tests/test_ai_safety.py::TestAdversarialInputCorpus::test_huge_payloads_rejected(4 parametrized cases) —ValueError: the environment variable is longer than 32767 characters, a Windows subprocess/env-var length limit hit by this test's large payload construction.Both reproduce identically on a clean checkout of
origin/mainbefore this PR's changes, confirming they predate it.Scope Confirmation
This PR is limited to issue #21. No unrelated bugs were fixed, no refactors were performed outside the experiment-reporting auth boundary, and no dependencies were changed.
Closes #21