diff --git a/backend/app/main.py b/backend/app/main.py index 994e964..2572c02 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -3,7 +3,6 @@ from contextlib import asynccontextmanager from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware -from fastapi.staticfiles import StaticFiles from app.config import settings from app.logging_config import setup_logging @@ -12,7 +11,7 @@ from app.logging_config import setup_logging setup_logging(settings.LOG_LEVEL) from app.database import engine, Base, SessionLocal from app.routers import auth, documents, quizzes, attempts, admin, tts, nextcloud, categories, questions, question_categories, favorites, teach, contact, tags, flashcards, courses, mobile, mynote -from app.routers import study_tools +from app.routers import study_tools, uploads from app.utils.auth import get_password_hash from app.utils.scheduler import start_scheduler, stop_scheduler @@ -614,7 +613,7 @@ from app.middleware.request_logging import RequestLoggingMiddleware app.add_middleware(RequestLoggingMiddleware) # Serve uploaded images as static files -app.mount("/uploads", StaticFiles(directory=settings.UPLOAD_DIR), name="uploads") +app.include_router(uploads.router) app.include_router(auth.router, prefix="/api/auth", tags=["auth"]) app.include_router(documents.router, prefix="/api/documents", tags=["documents"]) diff --git a/backend/app/routers/questions.py b/backend/app/routers/questions.py index e91221e..20c9e9e 100644 --- a/backend/app/routers/questions.py +++ b/backend/app/routers/questions.py @@ -14,7 +14,9 @@ from pydantic import BaseModel from sqlalchemy import cast, String, or_, text as sa_text, func from sqlalchemy.orm import Session +from app.config import settings from app.database import get_db +from app.utils.upload_access import validate_image_attachments, stored_upload_path from app.models.question import Question from app.models.question_category import QuestionCategory from app.models.quiz import Quiz @@ -83,7 +85,8 @@ def edit_question( is_mod = current_user.role in ("admin", "moderator") if question.user_id != current_user.id and not is_mod: raise HTTPException(status_code=403, detail="Not authorized to edit this question") - for field, value in data.model_dump(exclude_unset=True).items(): + values = validate_image_attachments(db, current_user, data.model_dump(exclude_unset=True)) + for field, value in values.items(): setattr(question, field, value) db.commit() db.refresh(question) @@ -344,10 +347,6 @@ def _strip_html(text: str) -> str: return clean -UPLOAD_DIR = os.path.join(os.path.dirname(os.path.dirname(__file__)), "..", "uploads", "questions") -os.makedirs(UPLOAD_DIR, exist_ok=True) - - class ManualQuestionCreate(BaseModel): question_text: str question_type: str = "mcq" @@ -376,6 +375,7 @@ def create_question_manually( if data.question_type == "mcq" and data.correct_answer not in data.options: raise HTTPException(status_code=400, detail="Correct answer must be one of the options") + images = validate_image_attachments(db, current_user, data.model_dump()) question = Question( question_text=q_text, question_type=data.question_type, @@ -383,8 +383,8 @@ def create_question_manually( correct_answer=data.correct_answer.strip(), explanation=data.explanation, question_category_id=data.question_category_id, - image_path=data.image_path, - explanation_image_path=data.explanation_image_path, + image_path=images["image_path"], + explanation_image_path=images["explanation_image_path"], user_id=current_user.id, is_shared=1, ) @@ -422,11 +422,13 @@ def upload_question_image( ext = file.filename.rsplit(".", 1)[-1].lower() if file.filename and "." in file.filename else "png" if ext not in ("png", "jpg", "jpeg", "gif", "webp", "svg"): raise HTTPException(status_code=400, detail="Unsupported image format") - filename = f"{uuid.uuid4().hex[:12]}.{ext}" - filepath = os.path.join(UPLOAD_DIR, filename) + filename = f"{uuid.uuid4().hex}.{ext}" + directory = os.path.join(settings.UPLOAD_DIR, "questions", str(current_user.id)) + os.makedirs(directory, exist_ok=True) + filepath = os.path.join(directory, filename) with open(filepath, "wb") as f: f.write(file.file.read()) - rel_path = f"questions/{filename}" + rel_path = f"questions/{current_user.id}/{filename}" return {"image_path": rel_path, "url": f"/uploads/{rel_path}"} @@ -436,19 +438,13 @@ def list_question_images( current_user: User = Depends(get_current_user), ): """List all unique question images for the image bank browser.""" - rows = db.execute(sa_text(""" - SELECT path FROM ( - SELECT image_path AS path FROM questions - WHERE image_path IS NOT NULL AND image_path <> '' - UNION - SELECT explanation_image_path AS path FROM questions - WHERE explanation_image_path IS NOT NULL AND explanation_image_path <> '' - ) AS image_paths - ORDER BY path - LIMIT 200 - """)).fetchall() - paths = [row.path for row in rows] - return [{"image_path": path, "url": f"/uploads/{path}"} for path in paths] + stems = bank_query(db, current_user).with_entities(Question.image_path.label("path")).filter( + Question.image_path.isnot(None), Question.image_path != "") + explanations = bank_query(db, current_user).with_entities(Question.explanation_image_path.label("path")).filter( + Question.explanation_image_path.isnot(None), Question.explanation_image_path != "") + paths = sorted({stored_upload_path(row.path) or row.path + for row in stems.union(explanations).order_by("path").limit(200).all()}) + return [{"image_path": path, "url": path if re.match(r"^https?://|^/uploads/", path, re.I) else f"/uploads/{path}"} for path in paths] @router.get("/builder/count") diff --git a/backend/app/routers/quizzes.py b/backend/app/routers/quizzes.py index f5f9f92..c59b693 100644 --- a/backend/app/routers/quizzes.py +++ b/backend/app/routers/quizzes.py @@ -4,6 +4,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query from sqlalchemy import cast, String, or_, and_, func from sqlalchemy.orm import Session +from app.utils.upload_access import validate_image_attachments from app.database import get_db from app.models.quiz import Quiz from app.models.question import Question as QuestionModel @@ -452,6 +453,7 @@ def update_question( raise HTTPException(status_code=404, detail="Question not found") allowed = {"question_text", "options", "correct_answer", "explanation", "question_type", "image_path", "explanation_image_path"} + data = validate_image_attachments(db, current_user, data) for key, value in data.items(): if key in allowed: setattr(question, key, value) diff --git a/backend/app/routers/teach.py b/backend/app/routers/teach.py index c837076..d089b4c 100644 --- a/backend/app/routers/teach.py +++ b/backend/app/routers/teach.py @@ -9,6 +9,8 @@ from app.database import get_db from app.models.question import Question from app.models.ai_model_config import AIModelConfig from app.models.user import User +from app.utils.quiz_access import require_question_access +from app.services.quiz_builder import bank_question_predicate from app.utils.auth import get_current_user, check_rate_limit router = APIRouter() @@ -30,6 +32,7 @@ class ChatRequest(BaseModel): model_config = {"protected_namespaces": ()} question_id: int + attempt_id: int | None = None messages: list[ChatMessage] model_id: int | None = None # AIModelConfig.id — if None, use default @@ -58,26 +61,16 @@ def _get_teach_model(db: Session, model_config_id: int | None = None): return (m.model_id, m.api_key or None) if m else None -def _find_similar_questions(db: Session, question: Question, limit: int = 4) -> list[Question]: - """Return up to `limit` questions with similar embeddings, excluding the current one.""" +def _find_similar_questions(db: Session, question: Question, user: User, limit: int = 4) -> list[Question]: + """Filter eligible context in SQL before similarity ranking and LIMIT.""" if question.embedding is None: return [] try: - from sqlalchemy import text as sa_text - emb = question.embedding - # Validate all values are finite floats before using in SQL - emb_literal = "[" + ",".join(str(float(x)) for x in emb) + "]" - rows = db.execute(sa_text(""" - SELECT id, 1 - (embedding <=> CAST(:vec AS vector)) AS sim - FROM questions - WHERE embedding IS NOT NULL AND id != :qid - ORDER BY embedding <=> CAST(:vec AS vector) - LIMIT :lim - """), {"vec": emb_literal, "qid": int(question.id), "lim": int(limit)}).fetchall() - ids = [r.id for r in rows if float(r.sim) >= 0.35] - if not ids: - return [] - return db.query(Question).filter(Question.id.in_(ids)).all() + distance = Question.embedding.cosine_distance(question.embedding) + return db.query(Question).filter( + bank_question_predicate(user), Question.id != question.id, + Question.embedding.isnot(None), distance <= 0.65, + ).order_by(distance).limit(limit).all() except Exception: return [] @@ -85,8 +78,7 @@ def _find_similar_questions(db: Session, question: Question, limit: int = 4) -> def _build_system_prompt(question: Question, similar: list[Question]) -> str: opts = "" if question.options: - letters = "ABCDE" - opts = "\n".join(f" {letters[i]}) {opt}" for i, opt in enumerate(question.options)) + opts = "\n".join(f" {i}) {opt}" for i, opt in enumerate(question.options, 1)) prompt = ( "You are a medical education tutor. A student is studying the question below.\n" @@ -146,6 +138,9 @@ async def chat( current_user: User = Depends(get_current_user), ): """Send a message to the teach AI with full question context.""" + question = db.query(Question).filter(Question.id == req.question_id).first() + require_question_access(db, question, current_user, req.attempt_id, review=True) + # Daily AI coach quota. Admins, moderators, and unthrottled users are exempt. quota_day, quota_ttl = _daily_teach_limit() check_rate_limit( @@ -163,11 +158,7 @@ async def chat( ) model_id, api_key = model_info - question = db.query(Question).filter(Question.id == req.question_id).first() - if not question: - raise HTTPException(status_code=404, detail="Question not found") - - similar = _find_similar_questions(db, question) + similar = _find_similar_questions(db, question, current_user) system_prompt = _build_system_prompt(question, similar) messages = [{"role": "system", "content": system_prompt}] diff --git a/backend/app/routers/uploads.py b/backend/app/routers/uploads.py new file mode 100644 index 0000000..309637b --- /dev/null +++ b/backend/app/routers/uploads.py @@ -0,0 +1,48 @@ +"""Native browser media authentication only; API authentication stays bearer-only.""" +from fastapi import APIRouter, Depends, HTTPException, Request +from fastapi.responses import FileResponse +from sqlalchemy.orm import Session + +from app.database import get_db +from app.utils.auth import get_current_user +from app.utils.upload_access import ( + LEGACY_LMS_PREFIXES, local_upload_path, upload_file, references, + document_for_file, can_read_upload, card_deck_ids, +) + +router = APIRouter() + + +@router.api_route("/uploads/{path:path}", methods=["GET", "HEAD"]) +def read_upload(path: str, request: Request, attempt_id: int | None = None, db: Session = Depends(get_db)): + headers = {"Cache-Control": "private, no-store", "Vary": "Cookie, Authorization", "X-Content-Type-Options": "nosniff"} + try: + return _read_upload(path, request, attempt_id, db, headers) + except HTTPException as exc: + exc.headers = {**(exc.headers or {}), **headers} + raise + + +def _read_upload(path, request, attempt_id, db, headers): + try: + path = local_upload_path(path) + target = upload_file(path) + except HTTPException: + raise HTTPException(404, "File not found") + questions = references(db, path) + cards = card_deck_ids(db, path) + # Default private also prevents orphaned question/card files becoming anonymous. + protected = (not path.startswith(LEGACY_LMS_PREFIXES) or document_for_file(db, path) + or questions or cards) + if protected: + authorization = request.headers.get("authorization", "") + token = authorization[7:] if authorization.lower().startswith("bearer ") else request.cookies.get("pedshub_media", "") + user = get_current_user(token, db) + if not can_read_upload(db, path, user, questions, cards, attempt_id): + raise HTTPException(404, "File not found") + # Known legacy LMS directories retain their policy; this is not a whole-LMS audit. + if not target.is_file(): + raise HTTPException(404, "File not found") + if target.suffix.lower() == ".svg": + headers["Content-Security-Policy"] = "sandbox; default-src 'none'; style-src 'unsafe-inline'" + return FileResponse(target, headers=headers) diff --git a/backend/app/utils/quiz_access.py b/backend/app/utils/quiz_access.py index 6f87f37..0f506fb 100644 --- a/backend/app/utils/quiz_access.py +++ b/backend/app/utils/quiz_access.py @@ -5,6 +5,8 @@ Course access is deliberately separate: publication/sharing never grants enrollm from fastapi import HTTPException from sqlalchemy import or_, select +from app.models.attempt import QuizAttempt +from app.utils.quiz_questions import question_in_quiz from app.models.course import Course, CourseEnrollment from app.models.question import Question from app.models.quiz import Quiz @@ -64,3 +66,28 @@ def set_quiz_shared(db, quiz, user, shared): quiz.is_published = 0 db.commit() return {"id": quiz.id, "is_shared": quiz.is_shared, "is_published": quiz.is_published} + + +def require_question_access(db, question, user, attempt_id=None, review=False): + """Authorize tutor/answer content or a stem, never trusting a course sharing flag.""" + if question is None: + raise HTTPException(404, "Question not found") + if attempt_id is not None: + attempt = db.query(QuizAttempt).filter_by(id=attempt_id, user_id=user.id).first() + if (not attempt or not question_in_quiz(db, attempt.quiz_id, question.id) + or (attempt.selected_question_ids is not None and question.id not in attempt.selected_question_ids)): + raise HTTPException(403, "Question is not available in this attempt") + require_quiz_access(db, attempt.quiz, user, review=review) + if review and attempt.mode != "study" and attempt.completed_at is None: + raise HTTPException(403, "Complete the attempt before reviewing") + return + if user.is_moderator: + return + if db.query(Question.id).filter(Question.id == question.id, bank_question_predicate(user)).first(): + return + quiz = db.get(Quiz, question.source_quiz_id) if question.source_quiz_id else None + if quiz and quiz.course_id is not None: + require_quiz_access(db, quiz, user, review=review) + if not review or quiz.user_id == user.id: + return + raise HTTPException(403, "Question is private or requires an authorized study/review attempt") diff --git a/backend/app/utils/upload_access.py b/backend/app/utils/upload_access.py new file mode 100644 index 0000000..40e0127 --- /dev/null +++ b/backend/app/utils/upload_access.py @@ -0,0 +1,193 @@ +"""Local upload paths and current-reference ACLs; no external URL fetching.""" +from pathlib import Path +import posixpath +import re +import socket +from urllib.parse import unquote, urlsplit + +from fastapi import HTTPException +from sqlalchemy import or_ +from sqlalchemy.orm import load_only + +from app.config import settings +from app.models.flashcard import Flashcard, FlashcardDeck +from app.models.pdf_document import PDFDocument +from app.models.question import Question +from app.services.quiz_builder import bank_question_predicate +from app.utils.quiz_access import require_question_access + +# These are the existing non-question upload producers in courses.py/certificate_service.py. +# They retain their legacy policy; arbitrary unknown/orphan files do not become public. +LEGACY_LMS_PREFIXES = ('course_files/', 'course_thumbnails/', 'scorm/', 'certificates/') + + +def local_upload_path(value): + if not isinstance(value, str): + raise HTTPException(400, "Invalid image path") + path = value.removeprefix("/uploads/") + if (not path or any(c in path for c in "\\%?#:") or any(ord(c) < 32 for c in path) + or path.endswith(' ') or any(part in ("", ".", "..") for part in path.split("/"))): + raise HTTPException(400, "Unsafe upload path") + return path + + +def upload_file(path): + """Reject even in-root symlink aliases before any ACL lookup.""" + try: + root = Path(settings.UPLOAD_DIR).resolve() + target = root + for part in path.split("/"): + target = target / part + if target.is_symlink(): + raise HTTPException(404, "File not found") + if not target.resolve().is_relative_to(root): + raise HTTPException(404, "File not found") + return target + except (OSError, ValueError): + raise HTTPException(404, "File not found") + + +def _authority(url): + host = unquote(url.hostname or '').rstrip('.').encode('idna').decode('ascii').lower() + try: + host = socket.inet_ntoa(socket.inet_aton(host)) # Numeric IPv4 spellings, no DNS/network. + except OSError: + pass + port = url.port + return host, None if port in (80, 443) else port + + +def stored_upload_path(value): + """Interpret old image strings like the browser, without rewriting stored content.""" + if not isinstance(value, str) or not value: + return None + # Match the existing image URL helper's prefix behavior before URL normalization. + raw = value if re.match(r'^(https?:)?//', value, re.I) or value.startswith('/uploads/') else '/uploads/' + value + raw = raw.replace('\\', '/').replace('\t', '').replace('\r', '').replace('\n', '') + raw = raw.strip(''.join(map(chr, range(33)))) + if raw.startswith('//'): + raw = '//' + raw.lstrip('/') + try: + url = urlsplit(raw) + if url.netloc: + if url.scheme and url.scheme.lower() not in ('http', 'https'): + return None + if _authority(url) != _authority(urlsplit(settings.APP_URL)): + return None + elif url.scheme: + return None + path = posixpath.normpath(unquote(url.path)) + if not path.startswith('/uploads/'): + return None + return local_upload_path(path) + except (ValueError, UnicodeError, HTTPException): + return None + + +def references(db, path): + # ponytail: scan reference metadata, not question bodies. Add indexed canonical + # media keys if library size makes this hot; do not cache stale access grants. + rows = db.query(Question.id, Question.image_path, Question.explanation_image_path).filter( + or_(Question.image_path.isnot(None), Question.explanation_image_path.isnot(None)), + ).all() + ids = [qid for qid, stem, explanation in rows + if stored_upload_path(stem) == path or stored_upload_path(explanation) == path] + if not ids: + return [] + return db.query(Question).options(load_only( + Question.id, Question.source_quiz_id, Question.image_path, Question.explanation_image_path, + )).filter(Question.id.in_(ids)).all() + + +def card_deck_ids(db, path): + return {deck_id for deck_id, image in db.query(Flashcard.deck_id, Flashcard.image_path).filter( + Flashcard.image_path.isnot(None), + ).all() if stored_upload_path(image) == path} + + +def document_for_file(db, path): + for document_id, filename in db.query(PDFDocument.id, PDFDocument.filename).all(): + if stored_upload_path(filename) == path: + return db.get(PDFDocument, document_id) + return None + + +def owns_source(db, path, user, cards): + if user.is_admin: + return True + if path.startswith(f"questions/{user.id}/"): + return True + # Recovery of legacy question drafts is not ownership of private card-only files. + if user.is_moderator and not cards and path.startswith('questions/'): + return True + match = re.fullmatch(r"images/doc_(\d+)/.+", path) + if match and not cards: # Card references stay on card ACL even inside extraction dirs. + document = db.get(PDFDocument, int(match[1])) + return document is not None and (document.user_id == user.id or user.is_moderator) + return False + + +def card_access(db, cards, user): + return bool(cards) and db.query(FlashcardDeck.id).filter( + FlashcardDeck.id.in_(cards), FlashcardDeck.deleted_at.is_(None), + or_(FlashcardDeck.user_id == user.id, FlashcardDeck.is_shared == 1, user.is_admin), + ).first() is not None + + +def can_read_upload(db, path, user, questions, cards, attempt_id=None): + # Sharing a question must never share its original source PDF. + document = document_for_file(db, path) + if document: + return user.is_moderator or document.user_id == user.id + if owns_source(db, path, user, cards) or card_access(db, cards, user): + return True + for question in questions: + try: + require_question_access(db, question, user, attempt_id, + review=stored_upload_path(question.image_path) != path) + return True + except HTTPException: + pass + return False + + +def validate_image_attachments(db, user, values): + """Validate every supplied image before callers mutate any ORM state.""" + result = dict(values) + for field in ("image_path", "explanation_image_path"): + value = result.get(field) + if value is None or value == "": + continue + if not isinstance(value, str): + raise HTTPException(400, "Invalid image path") + if re.match(r"^https?://", value, re.I): + try: + url = urlsplit(value) + if (not url.hostname or url.username or url.password or '\\' in value + or '%' in url.netloc or any(ord(c) < 32 for c in value)): + raise ValueError() + authority = _authority(url) # Also validates ports/hostname encoding. + except (ValueError, UnicodeError): + raise HTTPException(400, "Invalid image URL") + if authority != _authority(urlsplit(settings.APP_URL)) or not url.path.startswith('/uploads/'): + continue # External links are never fetched or granted local-file access. + if url.query or url.fragment: + raise HTTPException(400, "Use the original upload URL without query parameters or fragments") + value = url.path + path = local_upload_path(value) + upload_file(path) + document = document_for_file(db, path) + if document: + allowed = user.is_moderator or document.user_id == user.id + else: + cards = card_deck_ids(db, path) + allowed = owns_source(db, path, user, cards) or card_access(db, cards, user) + if not allowed: + ids = [question.id for question in references(db, path)] + allowed = bool(ids) if user.is_moderator else db.query(Question.id).filter( + Question.id.in_(ids), bank_question_predicate(user), + ).first() is not None + if not allowed: + raise HTTPException(403, "Image is not available for attachment") + result[field] = path + return result diff --git a/backend/tests/test_related_privacy.py b/backend/tests/test_related_privacy.py new file mode 100644 index 0000000..5eef266 --- /dev/null +++ b/backend/tests/test_related_privacy.py @@ -0,0 +1,415 @@ +"""Real routes, disposable files/SQLite, real JWTs; no network/AI calls.""" +from datetime import datetime +from pathlib import Path +from tempfile import TemporaryDirectory +from types import SimpleNamespace +import unittest +from unittest.mock import AsyncMock, Mock, patch + +from sqlalchemy.dialects import postgresql + +import test_quiz_builder as builder +from app.config import settings +from app.models.attempt import QuizAttempt +from app.models.course import CourseEnrollment +from app.models.email_verification import EmailVerification +from app.models.flashcard import Flashcard, FlashcardDeck +from app.models.pdf_document import PDFDocument +from app.models.question import Question +from app.models.quiz import Quiz +from app.routers import teach, uploads, flashcards +from app.utils.auth import create_access_token, get_current_user + + +class PrivacyTests(unittest.TestCase): + def setUp(self): + builder.BuilderTests.setUp(self) + self.tmp = TemporaryDirectory() + self.settings_patch = patch.object(settings, 'UPLOAD_DIR', self.tmp.name) + self.settings_patch.start() + self.client.app.include_router(teach.router, prefix='/teach') + self.client.app.include_router(uploads.router) + self.client.app.include_router(flashcards.router, prefix='/flashcards') + del self.client.app.dependency_overrides[get_current_user] + for q in self.db.query(Question): + q.image_path = f'questions/stem-{q.id}.png' + q.explanation_image_path = f'questions/answer-{q.id}.png' + self.file(q.image_path) + self.file(q.explanation_image_path) + self.db.commit() + self.login(self.owner) + self.quota = patch.object(teach, 'check_rate_limit').start() + self.model = patch.object(teach, '_get_teach_model', return_value=('synthetic', None)).start() + self.find_similar = teach._find_similar_questions + self.similar = patch.object(teach, '_find_similar_questions', return_value=[]).start() + self.ai = patch('litellm.acompletion', new_callable=AsyncMock).start() + self.ai.return_value = SimpleNamespace(choices=[SimpleNamespace(message=SimpleNamespace(content='Tutor reply\n> Follow up'))]) + self.embedding = patch('app.services.embedding_service.embed_question').start() + self.proxy = patch('app.services.ai_service._proxy_model', return_value='synthetic').start() + + def tearDown(self): + patch.stopall() + self.tmp.cleanup() + builder.BuilderTests.tearDown(self) + + def login(self, user, cookie=False): + self.client.headers.pop('authorization', None) + self.client.cookies.clear() + token = create_access_token({'sub': user.email}) + if cookie: + self.client.cookies.set('pedshub_media', token, path='/uploads') + else: + self.client.headers['Authorization'] = f'Bearer {token}' + + def file(self, path, data=b'synthetic-image'): + target = Path(self.tmp.name) / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(data) + return target + + def chat(self, qid, aid=None): + return self.client.post('/teach/chat', json={'question_id': qid, 'attempt_id': aid, 'messages': [{'role': 'user', 'content': 'Explain'}]}) + + def attempt(self, mode='study', completed=False, ids=None, user=1, quiz=2): + a = QuizAttempt(user_id=user, quiz_id=quiz, mode=mode, selected_question_ids=ids, + completed_at=datetime.utcnow() if completed else None) + self.db.add(a) + self.db.commit() + return a + + def enroll(self): + self.db.add(CourseEnrollment(course_id=1, user_id=1)) + self.db.get(Quiz, 2).allow_review = 1 + self.db.commit() + + def test_tutor_denial_precedes_all_external_boundaries(self): + for qid in (4, 5, 999): + self.assertIn(self.chat(qid).status_code, (403, 404)) + for boundary in (self.quota, self.model, self.similar, self.ai): + boundary.assert_not_called() + for qid in (1, 3): + res = self.chat(qid) + self.assertEqual(res.status_code, 200, res.text) + self.assertEqual(self.similar.call_args.args[2].id, 1) + self.login(self.mod) + self.assertEqual(self.chat(4).status_code, 200) + + def test_tutor_course_attempt_modes_pool_and_review(self): + self.enroll() + self.assertEqual(self.chat(5).status_code, 403) + for mode in ('exam', None): + a = self.attempt(mode=mode, ids=[5]) + self.assertEqual(self.chat(5, a.id).status_code, 403) + for a in (self.attempt(ids=[]), self.attempt(ids=[1]), self.attempt(user=2, ids=[5]), self.attempt(quiz=1, ids=[5])): + self.assertEqual(self.chat(5, a.id).status_code, 403) + self.assertEqual(self.chat(5, 999).status_code, 403) + self.ai.assert_not_called() + for a in (self.attempt(ids=[5]), self.attempt(mode='exam', completed=True, ids=[5])): + self.assertEqual(self.chat(5, a.id).status_code, 200) + self.db.get(Quiz, 2).allow_review = 0 + self.db.commit() + self.assertEqual(self.chat(5, a.id).status_code, 403) + self.db.query(CourseEnrollment).delete() + self.db.commit() + self.assertEqual(self.chat(5, a.id).status_code, 403) + + def test_similarity_sql_filters_before_limit_and_full_prompt(self): + q = self.db.get(Question, 1) + q.options = [f'Option {i}' for i in range(8)] + q.question_text = 'Long stem ' * 500 + prompt = teach._build_system_prompt(q, []) + self.assertIn(q.question_text, prompt) + self.assertIn('8) Option 7', prompt) + self.assertIn(q.explanation, prompt) + # Compile actual eligibility/ranking SQL; mock only terminal execution. + q.embedding = [0.1] * Question.__table__.c.embedding.type.dim + captured = [] + with patch('sqlalchemy.orm.Query.all', autospec=True, side_effect=lambda query: captured.append(query) or []): + self.assertEqual(self.find_similar(self.db, q, self.owner), []) + sql = str(captured[-1].statement.compile(dialect=postgresql.dialect())) + self.assertIn('questions.user_id', sql) + self.assertIn('questions.is_shared', sql) + self.assertIn('course_id IS NOT NULL', sql) + self.assertIn('<=>', sql) + self.assertLess(sql.index('WHERE'), sql.index('ORDER BY')) + self.assertLess(sql.index('ORDER BY'), sql.index('LIMIT')) + with patch('sqlalchemy.orm.Query.all', side_effect=RuntimeError('Unavailable embeddings')): + self.assertEqual(self.find_similar(self.db, q, self.owner), []) + q.embedding = None + self.assertEqual(self.find_similar(self.db, q, self.owner), []) + self.db.rollback() + + def test_native_cookie_bearer_revocation_and_api_exclusion(self): + self.client.headers.clear() + self.assertEqual(self.client.get('/uploads/questions/stem-4.png').status_code, 401) + self.login(self.owner, cookie=True) + for path in ('stem-1', 'stem-3', 'answer-1', 'answer-3'): + res = self.client.get(f'/uploads/questions/{path}.png') + self.assertEqual(res.status_code, 200, res.text) + self.assertEqual(res.headers['cache-control'], 'private, no-store') + self.assertEqual(res.headers['vary'], 'Cookie, Authorization') + self.assertEqual(self.client.get('/uploads/questions/stem-4.png').status_code, 404) + self.assertEqual(self.client.patch('/questions/3/share?shared=1').status_code, 401) + self.db.get(Question, 1).is_shared = 0 + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/stem-1.png').status_code, 404) + self.login(self.owner) + self.assertEqual(self.client.get('/uploads/questions/stem-3.png').status_code, 200) + self.db.add(EmailVerification(user_id=1, token='synthetic', expires_at=datetime(2099, 1, 1))) + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/stem-3.png').status_code, 403) + self.assertEqual(self.chat(3).status_code, 403) + + def test_course_stems_answers_and_independent_references(self): + self.assertEqual(self.client.get('/uploads/questions/stem-5.png').status_code, 404) + self.enroll() + self.assertEqual(self.client.get('/uploads/questions/stem-5.png').status_code, 200) + self.assertEqual(self.client.get('/uploads/questions/answer-5.png').status_code, 404) + a = self.attempt(mode='exam', ids=[5]) + url = f'/uploads/questions/answer-5.png?attempt_id={a.id}' + self.assertEqual(self.client.get(url).status_code, 404) + a.mode = 'study' + self.db.commit() + self.assertEqual(self.client.get(url).status_code, 200) + a.selected_question_ids = [] + self.db.commit() + self.assertEqual(self.client.get(url).status_code, 404) + a.selected_question_ids = [5] + self.db.get(Quiz, 2).allow_review = 0 + self.db.commit() + self.assertEqual(self.client.get(url).status_code, 404) + self.db.get(Question, 1).image_path = '/uploads/questions/answer-5.png' + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/answer-5.png').status_code, 200) + + def test_extraction_original_pdf_unattached_and_cards(self): + self.db.add_all([PDFDocument(id=1, user_id=2, filename='private.pdf', original_filename='source.pdf'), + PDFDocument(id=2, user_id=1, filename='own.pdf', original_filename='own.pdf')]) + for path in ('private.pdf', 'own.pdf', 'images/doc_1/shared.png', 'images/doc_1/adjacent.png', 'images/doc_2/own.png', 'questions/1/draft.png', 'questions/2/draft.png', 'questions/legacy.png'): + self.file(path) + self.db.get(Question, 1).image_path = 'images/doc_1/shared.png' + self.db.get(Question, 2).image_path = 'private.pdf' + self.db.commit() + for path, code in [('private.pdf', 404), ('own.pdf', 200), ('images/doc_1/shared.png', 200), ('images/doc_1/adjacent.png', 404), ('images/doc_2/own.png', 200), ('questions/1/draft.png', 200), ('questions/2/draft.png', 404), ('questions/legacy.png', 404)]: + self.assertEqual(self.client.get('/uploads/' + path).status_code, code, path) + deck = FlashcardDeck(user_id=2, title='Cards', is_shared=0) + self.db.add(deck) + self.db.flush() + self.db.add(Flashcard(deck_id=deck.id, front='front', back='back', image_path='questions/2/draft.png')) + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/2/draft.png').status_code, 404) + deck.is_shared = 1 + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/2/draft.png').status_code, 200) + deck.deleted_at = datetime.utcnow() + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/2/draft.png').status_code, 404) + self.login(self.mod) + self.assertEqual(self.client.get('/uploads/questions/legacy.png').status_code, 200) + + def test_attachment_forgery_atomic_and_chooser(self): + payload = {'question_text': 'Created', 'options': ['yes', 'no'], 'correct_answer': 'yes'} + before = self.db.query(Question).count() + self.enroll() + for field in ('image_path', 'explanation_image_path'): + for path in ('questions/stem-4.png', '/uploads/questions/answer-4.png', 'questions/stem-5.png', 'questions/2/draft.png', '../escape', '/uploads/questions/../escape', '//other/asset'): + res = self.client.post('/questions/create', json={**payload, field: path}) + self.assertIn(res.status_code, (400, 403), res.text) + self.assertEqual(self.db.query(Question).count(), before) + res = self.client.patch('/questions/3', json={'question_text': 'Forged', field: path}) + self.assertIn(res.status_code, (400, 403), res.text) + self.assertEqual(self.db.get(Question, 3).question_text, 'Question 3') + res = self.client.post('/questions/create', json={**payload, 'image_path': '/uploads/questions/stem-1.png'}) + self.assertEqual(res.status_code, 200, res.text) + self.assertEqual(res.json()['image_path'], 'questions/stem-1.png') + paths = [p['image_path'] for p in self.client.get('/questions/images').json()] + self.assertEqual(paths, sorted(set(paths))) + self.assertIn('questions/answer-3.png', paths) + self.assertNotIn('questions/stem-4.png', paths) + self.assertNotIn('questions/stem-5.png', paths) + self.login(self.mod) + for field in ('image_path', 'explanation_image_path'): + for value in ([], {}, 17, '../escape'): + res = self.client.patch('/quizzes/1/questions/1', json={'question_text': 'Forged', field: value}) + self.assertEqual(res.status_code, 400, res.text) + self.assertEqual(self.db.get(Question, 1).question_text, 'Question 1') + self.embedding.assert_called_once() + + def test_same_site_urls_share_path_authorization_and_legacy_references(self): + with patch.object(settings, 'APP_URL', 'https://app.example.com'): + payload = {'question_text': 'Absolute image', 'options': ['yes', 'no'], 'correct_answer': 'yes'} + count = self.db.query(Question).count() + for url in ('https://app.example.com/uploads/questions/stem-4.png', + 'http://app.example.com/uploads/questions/answer-4.png'): + response = self.client.post('/questions/create', json={**payload, 'image_path': url}) + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(self.db.query(Question).count(), count) + response = self.client.patch('/questions/3', json={'question_text': 'Forged', 'explanation_image_path': url}) + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(self.db.get(Question, 3).question_text, 'Question 3') + response = self.client.post('/questions/create', json={**payload, + 'image_path': 'https://app.example.com/uploads/questions/stem-1.png'}) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()['image_path'], 'questions/stem-1.png') + for url in ('https://external.example:bad/a.png', 'https://external.example:99999/a.png'): + response = self.client.patch('/questions/3', json={'image_path': url}) + self.assertEqual(response.status_code, 400, response.text) + response = self.client.patch('/questions/3', json={'image_path': 'https://external.example/figure.png'}) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(response.json()['image_path'], 'https://external.example/figure.png') + self.db.get(Question, 2).image_path = 'https://app.example.com/uploads/legacy-public.svg' + self.file('legacy-public.svg') + self.db.commit() + self.client.headers.clear() + self.assertEqual(self.client.get('/uploads/legacy-public.svg').status_code, 401) + self.login(self.owner) + self.assertEqual(self.client.get('/uploads/legacy-public.svg').status_code, 200) + self.db.get(Question, 2).is_shared = 0 + self.db.commit() + self.assertEqual(self.client.get('/uploads/legacy-public.svg').status_code, 404) + self.enroll() + self.db.get(Question, 5).image_path = 'http://app.example.com/uploads/questions/stem-5.png' + self.db.commit() + self.assertEqual(self.client.get('/uploads/questions/stem-5.png').status_code, 200) + + def test_card_only_moderator_denial_admin_success_and_question_grant(self): + admin = builder.User(id=4, name='Admin', email='admin@example.com', hashed_password='unused', role='admin') + deck = FlashcardDeck(user_id=2, title='Private peer cards', is_shared=0) + self.db.add_all([admin, deck]) + self.db.flush() + card = Flashcard(deck_id=deck.id, front='Private', back='Private', image_path='cards/private.png') + self.db.add(card) + self.db.commit() + payload = {'question_text': 'Copy', 'options': ['yes', 'no'], 'correct_answer': 'yes'} + for path in ('cards/private.png', 'questions/2/card-only.png'): + card.image_path = path + self.file(path) + self.db.commit() + self.login(self.mod) + self.assertEqual(self.client.get(f'/flashcards/{deck.id}').status_code, 403) + self.assertEqual(self.client.get('/uploads/' + path).status_code, 404) + before = self.db.query(Question).count() + response = self.client.post('/questions/create', json={**payload, 'image_path': path}) + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(self.db.query(Question).count(), before) + response = self.client.patch('/questions/1', json={'explanation_image_path': path, 'question_text': 'Forged'}) + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(self.db.get(Question, 1).question_text, 'Question 1') + self.login(admin) + self.assertEqual(self.client.get(f'/flashcards/{deck.id}').status_code, 200) + self.assertEqual(self.client.get('/uploads/' + path).status_code, 200) + self.assertEqual(self.client.post('/questions/create', json={**payload, 'image_path': path}).status_code, 200) + # Even inside extraction directories, a card reference stays on card ACL. + self.login(self.mod) + card.image_path = 'images/doc_1/card-only.png' + self.file(card.image_path) + self.db.commit() + self.assertEqual(self.client.get('/uploads/images/doc_1/card-only.png').status_code, 404) + self.assertEqual(self.client.patch('/questions/1', json={'image_path': card.image_path}).status_code, 403) + self.login(admin) + self.assertEqual(self.client.get('/uploads/images/doc_1/card-only.png').status_code, 200) + # A real question reference independently gives its moderator access. + self.login(self.mod) + self.db.get(Question, 4).image_path = 'cards/moderated-question.png' + card.image_path = 'cards/moderated-question.png' + self.file(card.image_path) + self.db.commit() + self.assertEqual(self.client.get('/uploads/cards/moderated-question.png').status_code, 200) + self.assertEqual(self.client.post('/questions/create', json={**payload, 'image_path': card.image_path}).status_code, 200) + + def test_legacy_aliases_are_canonical_for_classification_and_permissions(self): + with patch.object(settings, 'APP_URL', 'https://app.example.com'): + self.file('legacy-private.svg') + self.file('course_files/card-only.svg') + question = self.db.get(Question, 3) + variants = [ + 'https://APP.EXAMPLE.COM/uploads/legacy-private.svg?v=1#view', + 'http://app.example.com:80/uploads/legacy-private.svg#part', + '/uploads/legacy-private.svg?size=1', + 'legacy-private.svg#part', + 'https://app%2Eexample.com/uploads/legacy-private.svg', + '/uploads/images/../legacy-private.svg', + 'https://app.example.com/uploads/%6Cegacy-private.svg', + r'https://app.example.com\uploads\legacy-private.svg?v=1', + ] + for value in variants: + question.image_path = value + self.db.commit() + self.client.headers.clear() + self.client.cookies.clear() + self.assertEqual(self.client.get('/uploads/legacy-private.svg?v=1').status_code, 401, value) + self.login(self.owner) + self.assertEqual(self.client.get('/uploads/legacy-private.svg?v=1').status_code, 200, value) + self.login(self.peer) + self.assertEqual(self.client.get('/uploads/legacy-private.svg').status_code, 404, value) + self.login(self.owner) + bank_paths = self.client.get('/questions/images').json() + self.assertIn({'image_path': 'legacy-private.svg', 'url': '/uploads/legacy-private.svg'}, bank_paths) + # Parsing a browser-normalized URL must not bypass checks on new writes. + self.login(self.peer) + for value in ('https://app%2Eexample.com/uploads/legacy-private.svg', + r'https://app.example.com\uploads\legacy-private.svg'): + response = self.client.patch('/questions/4', json={'image_path': value}) + self.assertEqual(response.status_code, 400, response.text) + # Even a legacy-LMS directory becomes protected when a private card references it. + deck = FlashcardDeck(user_id=1, title='Private owner cards', is_shared=0) + self.db.add(deck) + self.db.flush() + self.db.add(Flashcard(deck_id=deck.id, front='Private', back='Private', + image_path='https://APP.EXAMPLE.COM/uploads/course_files/card-only.svg?v=1#card')) + self.db.commit() + self.client.headers.clear() + self.assertEqual(self.client.get('/uploads/course_files/card-only.svg').status_code, 401) + self.login(self.owner) + self.assertEqual(self.client.get('/uploads/course_files/card-only.svg').status_code, 200) + self.login(self.mod) + self.assertEqual(self.client.get('/uploads/course_files/card-only.svg').status_code, 404) + self.assertEqual(self.client.patch('/questions/1', json={'image_path': 'course_files/card-only.svg'}).status_code, 403) + + def test_orphaned_files_do_not_become_anonymous_after_reference_deletion(self): + self.file('orphan.svg') + question = self.db.get(Question, 3) + question.image_path = 'orphan.svg' + self.db.commit() + self.assertEqual(self.client.get('/uploads/orphan.svg').status_code, 200) + self.db.delete(question) + self.db.commit() + self.client.headers.clear() + self.assertEqual(self.client.get('/uploads/orphan.svg').status_code, 401) + self.login(self.peer) + self.assertEqual(self.client.get('/uploads/orphan.svg').status_code, 404) + for prefix in ('course_files', 'course_thumbnails', 'scorm', 'certificates'): + self.file(f'{prefix}/unrelated.bin') + self.client.headers.clear() + self.assertEqual(self.client.get(f'/uploads/{prefix}/unrelated.bin').status_code, 200) + + def test_paths_svg_head_range_upload_and_legacy_policy(self): + res = self.client.post('/questions/upload-image', files={'file': ('test.svg', b'', 'image/svg+xml')}) + self.assertEqual(res.status_code, 200, res.text) + self.assertTrue(res.json()['image_path'].startswith('questions/1/')) + url = res.json()['url'] + res = self.client.get(url) + self.assertEqual(res.headers['x-content-type-options'], 'nosniff') + self.assertIn('sandbox', res.headers['content-security-policy']) + head = self.client.head(url) + self.assertEqual(head.status_code, 200) + self.assertEqual(head.content, b'') + self.assertEqual(head.headers['content-length'], str(len(res.content))) + ranged = self.client.get(url, headers={'Range': 'bytes=0-3'}) + # HTTP permits ignoring Range; pinned Starlette returns a full 200 here. + # Production byte-range delivery is checked through Nginx's native filter. + self.assertIn(ranged.status_code, (200, 206)) + self.assertEqual(ranged.content, res.content[:4] if ranged.status_code == 206 else res.content) + Path(self.tmp.name, 'questions', 'alias.png').symlink_to(Path(self.tmp.name, 'questions', 'stem-4.png')) + Path(self.tmp.name, 'questions', 'outside.png').symlink_to('/etc/passwd') + for path in ('questions/alias.png', 'questions/outside.png', 'questions/%2e%2e/escape', 'questions/%252e%252e/escape', 'questions/%5cescape', 'questions/missing.png', 'questions/' + 'x' * 300): + self.assertEqual(self.client.get('/uploads/' + path).status_code, 404, path) + self.file('scorm/test/index.html', b'Legacy SCORM') + self.client.headers.clear() + legacy = self.client.get('/uploads/scorm/test/index.html') + self.assertEqual(legacy.status_code, 200) + self.assertEqual(legacy.headers['cache-control'], 'private, no-store') + + +if __name__ == '__main__': + unittest.main() diff --git a/docs/quiz-revamp-progress.md b/docs/quiz-revamp-progress.md index 4c520f2..5b5f20f 100644 --- a/docs/quiz-revamp-progress.md +++ b/docs/quiz-revamp-progress.md @@ -55,6 +55,18 @@ Browser checks used a loopback-only fixture with synthetic accounts/questions an Proof images: [desktop](quiz-revamp/desktop-question.png), [study feedback](quiz-revamp/study-feedback.png), [mobile width](quiz-revamp/mobile-question.png). These are synthetic previews, not screenshots of deployed clinical content. +## Related-content privacy (under verification) + +Tutor authorization now precedes quota/model/AI work, attempt context must own the selected question, course answers require an authorized study/completed attempt unless privileged, and similarity context is filtered in SQL before ranking/LIMIT. Full prompts retain long stems and more than five options. + +Uploaded media moved from a public static mount to a permission-aware boundary. Browser/Markdown/Milkdown image loading continues through a Path=/uploads media-only cookie mirroring the existing JavaScript-readable token (not HttpOnly/XSS-hardening). API mutations remain bearer-only; stale refresh/error responses cannot switch or resurrect accounts. + +References authorize bytes: bank visibility, course stem/review rules, per-uploader drafts, document/extraction ownership, card owner/admin/shared rules, and moderator handling that respects card-only privacy. Legacy local URL spellings are canonicalized for classification and permissions. Attachment validation runs before mutation on create and both edit paths, so private files cannot be republished into shared content. New uploads live under questions/{user_id}/. SVG gets a sandbox CSP; Nginx no longer caches uploads and provides native byte ranges. + +Verification: 37 backend tests in the exact deployed image (new card-moderator, legacy-alias, orphan and same-site URL regressions), 69 frontend tests and production build, real Nginx 206/HEAD/416 with no-store/Vary/sandbox through the deployed frontend image, real pgvector eligibility-before-LIMIT, and synthetic browser owner/peer/logout/quiz-image checks. Follow-up independent review resolved both earlier blockers and accepted the work; see [privacy-review.md](quiz-revamp/privacy-review.md). + +Known boundaries: only course_files/course_thumbnails/scorm/certificates retain legacy LMS policy; already-cached public responses cannot be recalled by new no-store headers; downloaded offline content likewise. Privacy work is not committed or deployed yet. + ## Next Continue with the Orthobullets-inspired runner/results UI, question navigation and study tools; then article/subsection reading, linked flashcards, educator AI authoring and moderated comments. Complete related-content privacy work and end-to-end desktop/mobile validation before deployment. diff --git a/docs/quiz-revamp/privacy-logout.png b/docs/quiz-revamp/privacy-logout.png new file mode 100644 index 0000000..3f6e70b Binary files /dev/null and b/docs/quiz-revamp/privacy-logout.png differ diff --git a/docs/quiz-revamp/privacy-owner.png b/docs/quiz-revamp/privacy-owner.png new file mode 100644 index 0000000..f762e87 Binary files /dev/null and b/docs/quiz-revamp/privacy-owner.png differ diff --git a/docs/quiz-revamp/privacy-peer.png b/docs/quiz-revamp/privacy-peer.png new file mode 100644 index 0000000..faaad34 Binary files /dev/null and b/docs/quiz-revamp/privacy-peer.png differ diff --git a/docs/quiz-revamp/privacy-player.png b/docs/quiz-revamp/privacy-player.png new file mode 100644 index 0000000..5191cdb Binary files /dev/null and b/docs/quiz-revamp/privacy-player.png differ diff --git a/docs/quiz-revamp/privacy-review.md b/docs/quiz-revamp/privacy-review.md new file mode 100644 index 0000000..c599b2d --- /dev/null +++ b/docs/quiz-revamp/privacy-review.md @@ -0,0 +1,11 @@ +## Review — follow-up validation (static, read-only) + +- **Finding 1 — RESOLVED.** `owns_source` (`backend/app/utils/upload_access.py:115–127`) no longer treats every file as moderator-owned. The moderator grant is now `user.is_moderator and not cards and path.startswith('questions/')` (line 121), so a live card reference (`cards` non-empty from `card_deck_ids`) forces the card ACL path: `card_access` (lines 130–134) requires `user_id == user.id` / `is_shared == 1` / admin — a non-owner moderator fails. Reads (`can_read_upload`, lines 137–152) and attachments (`validate_image_attachments`, lines 183–188) both fall through to `references()` only when the `questions/` and `images/doc_` grants do not apply, so moderator attachment of a card-only file returns 403. Admin still succeeds via `owns_source:116`. An independent question reference grants moderator access because `require_question_access` (`backend/app/utils/quiz_access.py:69–90`) returns for moderators, and `validate_image_attachments:186–187` uses `bool(ids)` for moderators. Covered by `test_card_only_moderator_denial_admin_success_and_question_grant` (`backend/tests/test_related_privacy.py`). + +- **Finding 2 — RESOLVED.** `stored_upload_path` (`upload_access.py:60–85`) canonicalizes case (`_authority` lowercases host), default port 80/443, query/fragment (dropped via `urlsplit`), percent (`unquote` on path/host), backslash (`replace('\\','/')`), and `..` (`posixpath.normpath`), and rejects anything that does not normalize back under `/uploads/` — so hostile out-of-root paths return `None`. Both classification (`references`/`card_deck_ids`/`document_for_file` compare canonicalized stored values to the route path) and permission checks use this. Anonymous access hits `get_current_user("", db)` → 401 (`uploads.py:37–41`); owner success and peer 404 covered by `test_legacy_aliases_are_canonical_for_classification_and_permissions`. New-write strictness is unchanged: `validate_image_attachments:168–175` rejects query/fragment, `%` in netloc, backslash, and bad ports on absolute URLs, and `local_upload_path:24–31` rejects `\ % ? # :`, control chars, and `..`/empty parts. The image chooser dedupes via `stored_upload_path(row.path) or row.path` in a set (`questions.py:445–447`). + +- **Orphan/LMS handling — RESOLVED.** `protected` (`uploads.py:35–36`) defaults non-LMS-prefix paths to private, and `document_for_file`/`questions`/`cards` force protection even inside a legacy prefix. Unreferenced `course_files|course_thumbnails|scorm|certificates` files stay anonymous (test `test_orphaned_files...`). Hostile paths can't escape: `upload_file:34–47` rejects symlinks and resolves `is_relative_to(root)`; `stored_upload_path` normalizes `..` before the `/uploads/` prefix check. + +- **Note (not a blocker):** `owns_source:123–126` grants moderators the `images/doc_N/` branch without checking `cards`, unlike the card-aware `questions/` clause. Currently unreachable as a leak because no code path writes `Flashcard.image_path` (generation at `tasks/quiz_tasks.py:655–660` sets none; `CardEdit` has only front/back), and moderators already hold document access. One-line hardening when cards get images: `if match and not cards:`. + +- **Residual risks (unverified by me):** I ran no tests/commands/live DB — static only. Parent re-verified 37 backend tests, 69 frontend tests/build, Nginx 206/HEAD/416 with no-store/Vary/SVG sandbox, and pgvector eligibility-before-LIMIT; those remain parent-attested, not re-run here. Full-table scans in `references`/`card_deck_ids`/`document_for_file` per request are an acknowledged perf ceiling (`upload_access.py:88`). The `pedshub_media` cookie is not HttpOnly (deliberate, `frontend/src/utils/token.js`). No blockers remain for the two P1 findings. \ No newline at end of file diff --git a/frontend/nginx.conf b/frontend/nginx.conf index 01e06f0..a86f22d 100644 --- a/frontend/nginx.conf +++ b/frontend/nginx.conf @@ -42,8 +42,9 @@ server { proxy_pass $backend; proxy_set_header Host $host; proxy_hide_header X-Frame-Options; - proxy_hide_header Content-Security-Policy; - add_header Cache-Control "public, max-age=86400"; + proxy_force_ranges on; # Pinned backend FileResponse does not implement byte ranges. + # Local add_header prevents inheriting the app CSP; preserve backend SVG sandbox. + add_header X-Content-Type-Options "nosniff" always; } # Uploaded images proxy to backend @@ -52,8 +53,9 @@ server { set $backend http://backend:8000; proxy_pass $backend; proxy_set_header Host $host; - expires 7d; - add_header Cache-Control "public, immutable"; + # Backend owns private/no-store and Vary; never cache protected media. + proxy_force_ranges on; + add_header X-Content-Type-Options "nosniff" always; } # SPA fallback diff --git a/frontend/src/api/client.js b/frontend/src/api/client.js index 20476ba..14559d0 100644 --- a/frontend/src/api/client.js +++ b/frontend/src/api/client.js @@ -1,4 +1,7 @@ import axios from 'axios' +import { setToken, syncMediaToken } from '../utils/token' + +syncMediaToken() const api = axios.create({ baseURL: '/api', @@ -12,34 +15,31 @@ api.interceptors.request.use((config) => { return config }) +function isCurrentRequest(config) { + const token = localStorage.getItem('token') + return !!token && config?.headers?.Authorization === `Bearer ${token}` +} + api.interceptors.response.use( (response) => { - // Sliding token expiration: if server sends new token, update localStorage const newToken = response.headers['x-new-token'] - if (newToken) { - localStorage.setItem('token', newToken) - } + if (newToken && isCurrentRequest(response.config)) setToken(newToken) return response }, (error) => { - // Check for token refresh even on error responses - const newToken = error.response?.headers?.['x-new-token'] - if (newToken) { - localStorage.setItem('token', newToken) - } - const url = error.config?.url || '' - // Don't redirect to login from the login/register/verify endpoints themselves + // Don't redirect from login/register/verify endpoints themselves. const isAuthEndpoint = url.includes('/auth/login') || url.includes('/auth/register') || url.includes('/auth/verify') || url.includes('/auth/reset') || url.includes('/auth/forgot') - if (error.response?.status === 401 && !isAuthEndpoint) { - localStorage.removeItem('token') - window.location.href = '/login' - } - // Unverified email — clear token and redirect to login (will show resend option) - if (error.response?.status === 403 && error.response?.headers?.['x-unverified'] === 'true') { - localStorage.removeItem('token') - window.location.href = '/login' + if (isCurrentRequest(error.config)) { + if ((error.response?.status === 401 && !isAuthEndpoint) || + (error.response?.status === 403 && error.response?.headers?.['x-unverified'] === 'true')) { + setToken(null) + window.location.href = '/login' + } else { + const newToken = error.response?.headers?.['x-new-token'] + if (newToken) setToken(newToken) + } } return Promise.reject(error) } diff --git a/frontend/src/api/client.test.js b/frontend/src/api/client.test.js new file mode 100644 index 0000000..991d324 --- /dev/null +++ b/frontend/src/api/client.test.js @@ -0,0 +1,77 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import { setToken, syncMediaToken } from '../utils/token' +import api from './client' + +beforeEach(() => { + localStorage.clear() + vi.restoreAllMocks() +}) + +it('sets, clears and bootstraps the media-only cookie with the stored token', () => { + const cookie = vi.spyOn(document, 'cookie', 'set') + setToken('token-a') + expect(localStorage.getItem('token')).toBe('token-a') + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('pedshub_media=token-a; Path=/uploads; SameSite=Strict')) + expect(cookie.mock.calls.at(-1)[0].includes('; Secure')).toBe(window.location.protocol === 'https:') + setToken(null) + expect(localStorage.getItem('token')).toBeNull() + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('Max-Age=0')) + localStorage.setItem('token', 'existing') + syncMediaToken() + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('pedshub_media=existing;')) +}) + +function deferredRequest() { + let finish, started + const ready = new Promise(resolve => { started = resolve }) + const promise = api.get('/synthetic', { adapter: config => new Promise(resolve => { + finish = (status = 200, headers = {}) => resolve({ status, headers, data: {}, config }) + started() + }) }) + return { promise, ready, finish: (...args) => finish(...args) } +} + +it('refreshes both stores only for the response original bearer', async () => { + const cookie = vi.spyOn(document, 'cookie', 'set') + setToken('old') + const req = deferredRequest() + await req.ready + req.finish(200, { 'x-new-token': 'new' }) + await req.promise + expect(localStorage.getItem('token')).toBe('new') + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('pedshub_media=new;')) +}) + +it.each([null, 'different-account'])('late refresh cannot restore credentials after switching to %s', async current => { + setToken('old') + const req = deferredRequest() + await req.ready + setToken(current) + req.finish(200, { 'x-new-token': 'stale-refresh' }) + await req.promise + expect(localStorage.getItem('token')).toBe(current) +}) + +it.each([401, 403, 500])('late error %s cannot refresh or clear a new account', async status => { + setToken('current') + const error = { config: { url: '/synthetic', headers: { Authorization: 'Bearer old' } }, + response: { status, headers: { 'x-new-token': 'stale', 'x-unverified': 'true' } } } + const rejected = api.interceptors.response.handlers[0].rejected + await expect(rejected(error)).rejects.toBe(error) + expect(localStorage.getItem('token')).toBe('current') + setToken(null) + await expect(rejected(error)).rejects.toBe(error) + expect(localStorage.getItem('token')).toBeNull() +}) + +it('keeps login redirect exclusions and refreshes an authenticated ordinary error', async () => { + setToken('current') + const rejected = api.interceptors.response.handlers[0].rejected + const error = { config: { url: '/auth/login', headers: { Authorization: 'Bearer current' } }, response: { status: 401, headers: {} } } + await expect(rejected(error)).rejects.toBe(error) + expect(localStorage.getItem('token')).toBe('current') + error.config.url = '/synthetic' + error.response = { status: 500, headers: { 'x-new-token': 'new' } } + await expect(rejected(error)).rejects.toBe(error) + expect(localStorage.getItem('token')).toBe('new') +}) diff --git a/frontend/src/components/TeachChat.jsx b/frontend/src/components/TeachChat.jsx index c082502..98e7daa 100644 --- a/frontend/src/components/TeachChat.jsx +++ b/frontend/src/components/TeachChat.jsx @@ -3,6 +3,7 @@ import ReactMarkdown from 'react-markdown' import remarkGfm from 'remark-gfm' import rehypeRaw from 'rehype-raw' import api from '../api/client' +import { markdownImageUrl } from '../utils/uploads' /** * TeachChat — slide-in AI tutor drawer for study mode. @@ -14,7 +15,7 @@ import api from '../api/client' * Mobile — bottom sheet, 70vh * Desktop — right-side panel inside quiz-layout, 320px wide */ -export default function TeachChat({ question, elevated = false }) { +export default function TeachChat({ question, attemptId, elevated = false }) { const [open, setOpen] = useState(false) const [messages, setMessages] = useState([]) // {role, content, suggestions?} const [input, setInput] = useState('') @@ -39,7 +40,7 @@ export default function TeachChat({ question, elevated = false }) { setMessages([]) setInput('') setNoModel(false) - }, [question?.id]) + }, [question?.id, attemptId]) // Scroll to bottom when messages change useEffect(() => { @@ -64,6 +65,7 @@ export default function TeachChat({ question, elevated = false }) { try { const res = await api.post('/teach/chat', { question_id: question.id, + attempt_id: attemptId ?? null, messages: next, model_id: selectedModelId || null, }) @@ -201,6 +203,7 @@ export default function TeachChat({ question, elevated = false }) { }}> {m.role === 'assistant' ? , p: ({children}) =>

{children}

, ul: ({children}) => , ol: ({children}) =>
    {children}
, diff --git a/frontend/src/components/TeachChat.test.jsx b/frontend/src/components/TeachChat.test.jsx new file mode 100644 index 0000000..0d6d728 --- /dev/null +++ b/frontend/src/components/TeachChat.test.jsx @@ -0,0 +1,25 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import TeachChat from './TeachChat' +import api from '../api/client' + +vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn() } })) +beforeEach(() => { + vi.resetAllMocks() + Element.prototype.scrollIntoView = vi.fn() + api.get.mockResolvedValue({ data: [] }) + api.post.mockResolvedValue({ data: { reply: '![Local](/uploads/questions/answer.png) ![External](https://external.example/a.png)', suggestions: [] } }) +}) + +it.each([undefined, 50])('sends optional attempt %s and renders local markdown media with context', async attemptId => { + render() + await userEvent.click(screen.getByTitle('Ask AI tutor')) + await userEvent.type(screen.getByPlaceholderText(/Ask a question/), 'Explain this') + await userEvent.click(screen.getByRole('button', { name: 'Send' })) + await waitFor(() => expect(api.post).toHaveBeenCalledWith('/teach/chat', { + question_id: 5, attempt_id: attemptId ?? null, messages: [{ role: 'user', content: 'Explain this' }], model_id: null, + })) + expect(await screen.findByAltText('Local')).toHaveAttribute('src', `/uploads/questions/answer.png${attemptId ? '?attempt_id=50' : ''}`) + expect(screen.getByAltText('External')).toHaveAttribute('src', 'https://external.example/a.png') +}) diff --git a/frontend/src/context/AuthContext.jsx b/frontend/src/context/AuthContext.jsx index 986cea5..32023ab 100644 --- a/frontend/src/context/AuthContext.jsx +++ b/frontend/src/context/AuthContext.jsx @@ -1,5 +1,6 @@ import { createContext, useContext, useState, useEffect } from 'react' import api from '../api/client' +import { setToken } from '../utils/token' const AuthContext = createContext(null) @@ -12,7 +13,7 @@ export function AuthProvider({ children }) { if (token) { api.get('/auth/me') .then(res => setUser(res.data)) - .catch(() => localStorage.removeItem('token')) + .catch(() => { if (localStorage.getItem('token') === token) setToken(null) }) .finally(() => setLoading(false)) } else { setLoading(false) @@ -21,21 +22,21 @@ export function AuthProvider({ children }) { const login = async (email, password) => { const res = await api.post('/auth/login', { email, password }) - localStorage.setItem('token', res.data.access_token) + setToken(res.data.access_token) const me = await api.get('/auth/me') setUser(me.data) return me.data } const loginWithToken = async (token) => { - localStorage.setItem('token', token) + setToken(token) const me = await api.get('/auth/me') setUser(me.data) return me.data } const logout = () => { - localStorage.removeItem('token') + setToken(null) setUser(null) } diff --git a/frontend/src/context/AuthContext.test.jsx b/frontend/src/context/AuthContext.test.jsx new file mode 100644 index 0000000..d05a150 --- /dev/null +++ b/frontend/src/context/AuthContext.test.jsx @@ -0,0 +1,38 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { AuthProvider, useAuth } from './AuthContext' +import api from '../api/client' + +vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn() } })) +beforeEach(() => { + localStorage.clear() + vi.restoreAllMocks() + api.get.mockResolvedValue({ data: { id: 1, name: 'Synthetic' } }) + api.post.mockResolvedValue({ data: { access_token: 'password-token' } }) +}) +function Controls() { + const { login, loginWithToken, logout, user } = useAuth() + return <>{user?.name} +} +it.each(['Password', 'SSO'])('mirrors %s login and synchronously clears logout', async method => { + const cookie = vi.spyOn(document, 'cookie', 'set') + render() + await userEvent.click(screen.getByText(method)) + await screen.findByText('Synthetic') + const token = method === 'Password' ? 'password-token' : 'sso-token' + expect(localStorage.getItem('token')).toBe(token) + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining(`pedshub_media=${token}; Path=/uploads`)) + await userEvent.click(screen.getByText('Logout')) + expect(localStorage.getItem('token')).toBeNull() + expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('Max-Age=0')) +}) +it('bootstrap failure cannot clear a switched account', async () => { + let reject + localStorage.setItem('token', 'bootstrap-token') + api.get.mockImplementationOnce(() => new Promise((_, fail) => { reject = fail })) + render() + localStorage.setItem('token', 'other-account') + reject(new Error('Old request failed')) + await waitFor(() => expect(localStorage.getItem('token')).toBe('other-account')) +}) diff --git a/frontend/src/pages/QuizEditPage.jsx b/frontend/src/pages/QuizEditPage.jsx index 27b1b65..27ffb38 100644 --- a/frontend/src/pages/QuizEditPage.jsx +++ b/frontend/src/pages/QuizEditPage.jsx @@ -1,16 +1,11 @@ import { useState, useEffect } from 'react' import { useParams, useNavigate, Link } from 'react-router-dom' import api from '../api/client' +import { uploadUrl } from '../utils/uploads' import ConfirmButton from '../components/ConfirmButton' const LETTERS = ['A', 'B', 'C', 'D', 'E', 'F'] -function uploadUrl(path) { - if (!path) return '' - if (/^https?:\/\//i.test(path) || path.startsWith('/uploads/')) return path - return `/uploads/${path}` -} - function ImagePreview({ label, path }) { if (!path) return null return ( diff --git a/frontend/src/pages/QuizPage.jsx b/frontend/src/pages/QuizPage.jsx index fe528c3..13769db 100644 --- a/frontend/src/pages/QuizPage.jsx +++ b/frontend/src/pages/QuizPage.jsx @@ -1,3 +1,4 @@ +import { uploadUrl } from '../utils/uploads' import { useState, useEffect, useRef, useCallback, lazy, Suspense } from 'react' import { useParams, useNavigate, useSearchParams, Link } from 'react-router-dom' import { useAuth } from '../context/AuthContext' @@ -1174,7 +1175,7 @@ const timerStarted = timeLeft !== null {current.image_path && ( )} @@ -1191,7 +1192,7 @@ const timerStarted = timeLeft !== null
e.stopPropagation()}> Expanded question illustrationExplanation: {current.explanation}} {current.explanation_image_path && ( )} @@ -1318,7 +1319,7 @@ const timerStarted = timeLeft !== null {/* AI tutor — only in study mode, lazy-loaded */} {isStudy && current && ( - + )}
diff --git a/frontend/src/pages/QuizPage.test.jsx b/frontend/src/pages/QuizPage.test.jsx index ab3333b..f0c89e0 100644 --- a/frontend/src/pages/QuizPage.test.jsx +++ b/frontend/src/pages/QuizPage.test.jsx @@ -8,7 +8,7 @@ import api from '../api/client' vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn(), delete: vi.fn() } })) vi.mock('../context/AuthContext', () => ({ useAuth: () => ({ user: { id: 1, name: 'Learner', role: 'user' } }) })) vi.mock('../components/MyNote', () => ({ default: () => null })) -vi.mock('../components/TeachChat', () => ({ default: () =>
Study tutor
})) +vi.mock('../components/TeachChat', () => ({ default: ({ attemptId }) =>
Study tutor {attemptId}
})) const questions = [ { id: 1, question_text: 'Full first clinical question.', question_type: 'mcq', options: ['First answer', 'Second answer'], category_breadcrumbs: [{ id: 10, name: 'Pediatrics' }, { id: 11, name: 'Neonatology' }] }, @@ -148,6 +148,24 @@ describe('quiz player', () => { await waitFor(() => expect(api.post).toHaveBeenCalledWith('/favorites', { question_id: 1 })) }) + it('passes its attempt to tutor, question, expanded and explanation images', async () => { + const originalGet = api.get.getMockImplementation() + api.get.mockImplementation(async (...args) => { + const res = await originalGet(...args) + if (res.data?.questions) res.data.questions[0] = { ...res.data.questions[0], image_path: 'questions/stem.png', explanation_image_path: '/uploads/questions/answer.png' } + return res + }) + await begin() + expect(await screen.findByTestId('tutor-context')).toHaveTextContent('50') + expect(screen.getByAltText('Question illustration')).toHaveAttribute('src', '/uploads/questions/stem.png?attempt_id=50') + await userEvent.click(screen.getByAltText('Question illustration')) + expect(screen.getByAltText('Expanded question illustration')).toHaveAttribute('src', '/uploads/questions/stem.png?attempt_id=50') + await userEvent.click(screen.getByRole('button', { name: 'Close expanded image' })) + fireEvent.keyDown(window, { key: '1' }) + fireEvent.keyDown(window, { key: 'Enter' }) + expect(await screen.findByAltText('Explanation illustration')).toHaveAttribute('src', '/uploads/questions/answer.png?attempt_id=50') + }) + it('offers a functional calculator without hijacking input shortcuts', async () => { await begin() await userEvent.click(screen.getByRole('button', { name: /Calculator/ })) diff --git a/frontend/src/pages/ResultsPage.jsx b/frontend/src/pages/ResultsPage.jsx index 9d918ec..dc2a42d 100644 --- a/frontend/src/pages/ResultsPage.jsx +++ b/frontend/src/pages/ResultsPage.jsx @@ -1,3 +1,4 @@ +import { uploadUrl } from '../utils/uploads' import { useState, useEffect } from 'react' import { useParams, useNavigate, useLocation, useSearchParams, Link } from 'react-router-dom' import api from '../api/client' @@ -178,7 +179,7 @@ export default function ResultsPage() { - {ans.image_path && Question illustration} + {ans.image_path && Question illustration} {/* Numbered answers */} {ans.options && ans.options.length > 0 && (
@@ -234,7 +235,7 @@ export default function ResultsPage() { {ans.explanation &&
{ans.explanation}
} {ans.explanation_image_path && (
- Explanation illustration e.currentTarget.style.display = 'none'} />
diff --git a/frontend/src/utils/token.js b/frontend/src/utils/token.js new file mode 100644 index 0000000..0b00924 --- /dev/null +++ b/frontend/src/utils/token.js @@ -0,0 +1,10 @@ +// Media-only mirror of the existing JS-readable token, NOT HttpOnly/XSS hardening. +export function setToken(token) { + if (token) localStorage.setItem('token', token) + else localStorage.removeItem('token') + document.cookie = `pedshub_media=${token ? encodeURIComponent(token) : ''}; Path=/uploads; SameSite=Strict${window.location.protocol === 'https:' ? '; Secure' : ''}${token ? '' : '; Max-Age=0'}` +} + +export function syncMediaToken() { + setToken(localStorage.getItem('token')) +} diff --git a/frontend/src/utils/uploads.js b/frontend/src/utils/uploads.js new file mode 100644 index 0000000..41211ad --- /dev/null +++ b/frontend/src/utils/uploads.js @@ -0,0 +1,21 @@ +// Keep the editor's relative/absolute URL behavior; attempt context is local only. +export function uploadUrl(path, attemptId) { + if (!path) return '' + const value = /^(https?:)?\/\//i.test(path) || path.startsWith('/uploads/') ? path : `/uploads/${path}` + let url + try { url = new URL(value, window.location.origin) } + catch { return value } // Legacy invalid URLs must not crash the question/player. + if (attemptId != null && url.origin === window.location.origin && url.pathname.startsWith('/uploads/')) { + url.searchParams.set('attempt_id', attemptId) + return `${url.pathname}${url.search}${url.hash}` + } + return value +} + +export function markdownImageUrl(src, attemptId) { + if (!src) return src + let url + try { url = new URL(src, window.location.origin) } + catch { return src } + return url.origin === window.location.origin && url.pathname.startsWith('/uploads/') ? uploadUrl(src, attemptId) : src +} diff --git a/frontend/src/utils/uploads.test.js b/frontend/src/utils/uploads.test.js new file mode 100644 index 0000000..30639d9 --- /dev/null +++ b/frontend/src/utils/uploads.test.js @@ -0,0 +1,24 @@ +import { expect, it } from 'vitest' +import { uploadUrl, markdownImageUrl } from './uploads' + +it('leaves malformed legacy image URLs to normal browser image-error handling', () => { + for (const url of ['https://', 'https://host:bad/a.png', 'http://[invalid']) { + expect(uploadUrl(url, 12)).toBe(url) + expect(markdownImageUrl(url, 12)).toBe(url) + } +}) + +it('uses the existing upload format and adds attempt context only to same-origin uploads', () => { + expect(uploadUrl('questions/test.png')).toBe('/uploads/questions/test.png') + expect(uploadUrl('/uploads/questions/test.png', 12)).toBe('/uploads/questions/test.png?attempt_id=12') + expect(uploadUrl('questions/test.png?size=1#view', 12)).toBe('/uploads/questions/test.png?size=1&attempt_id=12#view') + expect(uploadUrl(`${window.location.origin}/uploads/test.png?attempt_id=1`, 12)).toBe('/uploads/test.png?attempt_id=12') + for (const url of ['https://external.example/uploads/test.png', '//external.example/uploads/test.png']) { + expect(uploadUrl(url, 12)).toBe(url) + expect(markdownImageUrl(url, 12)).toBe(url) + } + expect(markdownImageUrl('/uploads/test.png', 12)).toBe('/uploads/test.png?attempt_id=12') + expect(markdownImageUrl('/other/test.png', 12)).toBe('/other/test.png') + expect(markdownImageUrl('https://external.example/test.png', 12)).toBe('https://external.example/test.png') + expect(uploadUrl(null, 12)).toBe('') +})