From 18fa1913a4b59dfd5d4380fbf72d32c8aac308a8 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 22:37:28 +0200 Subject: [PATCH] fix: the contact form's messages were readable by anyone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/contact/submissions` had no authentication. `require_admin` was imported inside the function body and never used as a dependency, so the import read as protection and was none: anyone who guessed the path could read every sender's name, email address and message. `PUT .../read` was open the same way. Both now depend on `require_admin`, with a test that a learner gets 403 and an administrator gets the list. A row with a null timestamp no longer takes the whole listing down with it — which is the only reason the hole showed up as a 500 rather than as data. Also: the tutor's site switch lives in Redis, which the tests share with the running site, so turning the tutor off in the interface turned a test red. The test now sets the flag it depends on and puts it back. And the tutor button is hidden until the server says it is allowed, rather than shown and then withdrawn — on a site with it switched off that flicker reads as a bug rather than a policy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/routers/contact.py | 18 +++++-- backend/tests/test_contact_privacy.py | 78 +++++++++++++++++++++++++++ backend/tests/test_related_privacy.py | 8 +++ frontend/src/pages/QuizPage.jsx | 5 +- 4 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_contact_privacy.py diff --git a/backend/app/routers/contact.py b/backend/app/routers/contact.py index c48a6e3..55a96e8 100644 --- a/backend/app/routers/contact.py +++ b/backend/app/routers/contact.py @@ -10,7 +10,9 @@ from sqlalchemy.orm import Session from app.database import get_db from app.config import settings +from app.models.user import User from app.services import captcha +from app.utils.auth import require_admin router = APIRouter() log = logging.getLogger(__name__) @@ -75,9 +77,15 @@ async def _email_admin(name: str, email: str, type_: str, message: str): @router.get("/submissions") def list_submissions( db: Session = Depends(get_db), + current_user: User = Depends(require_admin), ): - """Admin: list all contact submissions.""" - from app.utils.auth import require_admin + """Every message the contact form has taken. Administrators only. + + It was open to the internet: `require_admin` was imported inside the + function body and never used as a dependency, so the import read as + protection and was none. Anyone who guessed the path could read every + sender's name, address and message. + """ rows = db.execute(text( "SELECT id, name, email, type, message, read, created_at FROM contact_submissions ORDER BY created_at DESC" )).fetchall() @@ -85,7 +93,9 @@ def list_submissions( { "id": r.id, "name": r.name, "email": r.email, "type": r.type, "message": r.message, "read": r.read, - "created_at": r.created_at.isoformat(), + # Some early rows have no timestamp, and a listing that raises on + # one of them shows none of the others. + "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows ] @@ -95,7 +105,9 @@ def list_submissions( def mark_read( submission_id: int, db: Session = Depends(get_db), + current_user: User = Depends(require_admin), ): + """Mark one as read. Administrators only, for the same reason as the list.""" result = db.execute(text("UPDATE contact_submissions SET read=1 WHERE id=:id"), {"id": submission_id}) db.commit() if result.rowcount == 0: diff --git a/backend/tests/test_contact_privacy.py b/backend/tests/test_contact_privacy.py new file mode 100644 index 0000000..4224470 --- /dev/null +++ b/backend/tests/test_contact_privacy.py @@ -0,0 +1,78 @@ +"""Who may read the contact form's messages. + +Run: DATABASE_URL=sqlite:// PYTHONPATH=backend python -m unittest discover -s backend/tests +""" +import os +os.environ.setdefault("DATABASE_URL", "sqlite://") + +import unittest + +from fastapi import FastAPI +from fastapi.testclient import TestClient +from sqlalchemy import create_engine, text +from sqlalchemy.orm import Session +from sqlalchemy.pool import StaticPool + +from app.database import get_db +from app.models.user import User +from app.routers import contact +from app.utils.auth import get_current_user + + +class ContactPrivacyTests(unittest.TestCase): + """The listing was open to the internet. + + `require_admin` was imported inside the function body and never used as a + dependency, so the import read as protection and was none: anyone who + guessed the path could read every sender's name, address and message. + """ + + def setUp(self): + self.engine = create_engine("sqlite://", connect_args={"check_same_thread": False}, + poolclass=StaticPool) + # Only the table this router touches, plus users. `create_all` pulls in + # every model in the metadata, and one of them carries a raw-SQL table + # SQLite cannot build. + User.__table__.create(self.engine, checkfirst=True) + self.db = Session(self.engine) + self.db.execute(text( + "CREATE TABLE IF NOT EXISTS contact_submissions (" + "id INTEGER PRIMARY KEY, name TEXT, email TEXT, type TEXT, message TEXT," + " read INTEGER DEFAULT 0, created_at TIMESTAMP)")) + self.db.execute(text( + "INSERT INTO contact_submissions (id, name, email, type, message, read, created_at)" + " VALUES (1, 'A Sender', 'sender@example.test', 'question', 'Private message', 0, NULL)")) + self.db.commit() + + self.admin = User(id=1, email="admin@example.test", name="Admin", role="admin") + self.learner = User(id=2, email="learner@example.test", name="Learner", role="user") + self.db.add_all([self.admin, self.learner]) + self.db.commit() + + self.user = self.learner + app = FastAPI() + app.include_router(contact.router, prefix="/contact") + app.dependency_overrides[get_db] = lambda: self.db + app.dependency_overrides[get_current_user] = lambda: self.user + self.client = TestClient(app) + + def tearDown(self): + self.db.close() + self.engine.dispose() + + def test_a_learner_may_not_read_or_touch_the_messages(self): + self.assertEqual(self.client.get("/contact/submissions").status_code, 403) + self.assertEqual(self.client.put("/contact/submissions/1/read").status_code, 403) + + def test_an_administrator_may(self): + self.user = self.admin + listing = self.client.get("/contact/submissions") + self.assertEqual(listing.status_code, 200, listing.text) + self.assertEqual(listing.json()[0]["email"], "sender@example.test") + # A row with no timestamp does not take the whole listing down with it. + self.assertIsNone(listing.json()[0]["created_at"]) + self.assertEqual(self.client.put("/contact/submissions/1/read").status_code, 200) + + +if __name__ == "__main__": + unittest.main() diff --git a/backend/tests/test_related_privacy.py b/backend/tests/test_related_privacy.py index 3ba5bf2..e77c89b 100644 --- a/backend/tests/test_related_privacy.py +++ b/backend/tests/test_related_privacy.py @@ -97,6 +97,14 @@ class PrivacyTests(unittest.TestCase): self.assertEqual(self.chat(4).status_code, 200) def test_tutor_course_attempt_modes_pool_and_review(self): + # The tutor's site switch lives in Redis, which these tests share with + # the running site — so an administrator turning the tutor off in the + # interface used to turn this test red. What is under test here is who + # may reach the tutor when it is *on*, so it says so. + from app.services import site_settings + site_settings.set_flag("tutor_in_quiz", True) + self.addCleanup(site_settings.set_flag, "tutor_in_quiz", False) + self.enroll() self.assertEqual(self.chat(5).status_code, 403) for mode in ('exam', None): diff --git a/frontend/src/pages/QuizPage.jsx b/frontend/src/pages/QuizPage.jsx index ddc9e8f..6f8ad92 100644 --- a/frontend/src/pages/QuizPage.jsx +++ b/frontend/src/pages/QuizPage.jsx @@ -570,7 +570,10 @@ export default function QuizPage() { // Suspending is not abandoning: it ends on the session's own analysis, where // what has been answered so far is scored and the Resume button sits. A // course quiz still returns to the course it belongs to. - const [tutorAllowed, setTutorAllowed] = useState(true) + //: Withheld until the server says otherwise. Defaulting to allowed made the + //: tutor's button appear for a moment on every session and then vanish on a + //: site that has it switched off, which reads as a bug rather than a policy. + const [tutorAllowed, setTutorAllowed] = useState(false) useEffect(() => { let live = true