From 6c85f8b4a93325a116097b7375af0c0b5923822a Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 7 Sep 2026 16:44:07 +0200 Subject: [PATCH] fix: resolve article review findings Escape raw HTML in article markdown, honor section deep links, filter card link listings by bank visibility and publication status, validate source sections. 44 backend and 77 frontend tests pass. --- backend/app/routers/articles.py | 17 ++++----- backend/app/routers/flashcards.py | 10 ++++-- backend/tests/test_articles_cards.py | 45 ++++++++++++++++++++++++ frontend/src/pages/ArticlesPage.jsx | 35 +++++++++++------- frontend/src/pages/ArticlesPage.test.jsx | 28 +++++++++++++++ 5 files changed, 112 insertions(+), 23 deletions(-) diff --git a/backend/app/routers/articles.py b/backend/app/routers/articles.py index 97040e5..6ede145 100644 --- a/backend/app/routers/articles.py +++ b/backend/app/routers/articles.py @@ -1,6 +1,5 @@ """Topic/article library with stable subsection links and card/question associations.""" import re -import uuid from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel, field_validator @@ -10,6 +9,7 @@ from app.database import get_db from app.models.article import Article, QuestionArticleLink from app.models.flashcard import Flashcard, FlashcardDeck, FlashcardArticleLink from app.models.question import Question +from app.models.section import Section from app.models.user import User from app.services.quiz_builder import bank_question_predicate from app.services.quiz_builder import category_breadcrumbs @@ -82,6 +82,11 @@ def _section_ids(article): return {section["id"] for section in (article.sections or [])} +def _validate_source_section(db, section_id): + if section_id is not None and not db.get(Section, section_id): + raise HTTPException(400, "Source section not found") + + def _article_json(article: Article) -> dict: return { "id": article.id, @@ -106,7 +111,7 @@ def list_articles( db: Session = Depends(get_db), current_user: User = Depends(get_current_user), ): - """Published articles for everyone; educators additionally see their own drafts.""" + """Published articles for everyone; educators additionally see drafts.""" query = db.query(Article) if category_id: query = query.filter(Article.category_id == category_id) @@ -127,6 +132,7 @@ def create_article( _validate_sections(data.sections) if data.category_id and not db.get(QuestionCategory, data.category_id): raise HTTPException(400, "Category not found") + _validate_source_section(db, data.section_id) if db.query(Article.id).filter(Article.slug == data.slug).first(): raise HTTPException(400, "Slug is already in use") article = Article( @@ -245,6 +251,7 @@ def update_article( _validate_sections(data.sections) if data.category_id and not db.get(QuestionCategory, data.category_id): raise HTTPException(400, "Category not found") + _validate_source_section(db, data.section_id) if db.query(Article.id).filter(Article.slug == data.slug, Article.id != article.id).first(): raise HTTPException(400, "Slug is already in use") article.slug, article.title, article.summary, article.content = data.slug, data.title, data.summary, data.content @@ -334,9 +341,3 @@ def unlink_question( question_id=question_id, article_id=article_id, section_id=section_id, ).delete(synchronize_session=False) db.commit() - - -@router.post("/{article_id}/section-id") -def new_section_id(): - """Clients may mint stable section IDs offline; this helper exists for parity.""" - return {"id": uuid.uuid4().hex} diff --git a/backend/app/routers/flashcards.py b/backend/app/routers/flashcards.py index c2dee5e..268ad47 100644 --- a/backend/app/routers/flashcards.py +++ b/backend/app/routers/flashcards.py @@ -578,10 +578,16 @@ def list_card_links( deck = db.get(FlashcardDeck, card.deck_id) if deck and deck.user_id != current_user.id and not deck.is_shared and not current_user.is_admin: raise HTTPException(status_code=403, detail="Not your card") - questions = db.query(Question.id, Question.question_text).join( + questions = db.query(Question.id, Question.question_text) + if not current_user.is_moderator: + questions = questions.filter(bank_question_predicate(current_user)) + questions = questions.join( FlashcardQuestionLink, FlashcardQuestionLink.question_id == Question.id, ).filter(FlashcardQuestionLink.flashcard_id == card_id).all() - articles = db.query(Article.id, Article.title, Article.status, FlashcardArticleLink.article_section_id).join( + articles = db.query(Article.id, Article.title, Article.status, FlashcardArticleLink.article_section_id) + if not current_user.is_moderator: + articles = articles.filter(Article.status == 'published') + articles = articles.join( FlashcardArticleLink, FlashcardArticleLink.article_id == Article.id, ).filter(FlashcardArticleLink.flashcard_id == card_id).all() return { diff --git a/backend/tests/test_articles_cards.py b/backend/tests/test_articles_cards.py index 8ce98eb..65a5484 100644 --- a/backend/tests/test_articles_cards.py +++ b/backend/tests/test_articles_cards.py @@ -156,5 +156,50 @@ class ArticlesCardsTests(unittest.TestCase): self.assertEqual(self.bank.db.query(QuestionArticleLink).count(), 0) + def test_card_links_hide_private_questions_and_draft_articles(self): + deck = FlashcardDeck(user_id=3, title='Shared educator deck', is_shared=1) + self.bank.db.add(deck) + self.bank.db.flush() + card = Flashcard(deck_id=deck.id, front='Private front', back='Private back') + self.bank.db.add(card) + self.bank.db.flush() + self.bank.user = self.bank.mod + article = self.create().json() # Draft, owned by the moderator. + self.client.put(f"/flashcards/cards/{card.id}/links/question", json={'question_id': 1}) + self.client.put(f"/flashcards/cards/{card.id}/links/article", json={'article_id': article['id']}) + # Drafts stay hidden from learners even though the shared deck exposes the card. + self.bank.user = self.bank.owner + links = self.client.get(f"/flashcards/cards/{card.id}/links").json() + self.assertEqual([q['id'] for q in links['questions']], [1]) + self.assertEqual(links['articles'], []) + # Making the question private and publishing the article flips both filters. + self.bank.db.get(Question, 1).is_shared = 0 + self.bank.db.commit() + self.bank.user = self.bank.mod + self.client.post(f"/articles/{article['id']}/publish", json={'published': True}) + self.bank.user = self.bank.owner + links = self.client.get(f"/flashcards/cards/{card.id}/links").json() + self.assertEqual(links['questions'], []) + self.assertEqual([a['id'] for a in links['articles']], [article['id']]) + self.bank.user = self.bank.peer + links = self.client.get(f"/flashcards/cards/{card.id}/links").json() + self.assertEqual(links['questions'], []) + self.assertEqual([a['id'] for a in links['articles']], [article['id']]) + self.bank.user = self.bank.mod + links = self.client.get(f"/flashcards/cards/{card.id}/links").json() + self.assertEqual([q['id'] for q in links['questions']], [1]) + self.assertEqual([a['id'] for a in links['articles']], [article['id']]) + + def test_source_section_validation_rejects_unknown_ids(self): + self.bank.user = self.bank.mod + response = self.create(section_id=999) + self.assertEqual(response.status_code, 400, response.text) + article = self.create().json() + response = self.client.patch(f"/articles/{article['id']}", json={ + "title": "Topic article", "slug": "topic-article", "content": "Intro", + "sections": [], "section_id": 999}) + self.assertEqual(response.status_code, 400, response.text) + + if __name__ == '__main__': unittest.main() diff --git a/frontend/src/pages/ArticlesPage.jsx b/frontend/src/pages/ArticlesPage.jsx index 9a7bd64..0172307 100644 --- a/frontend/src/pages/ArticlesPage.jsx +++ b/frontend/src/pages/ArticlesPage.jsx @@ -1,8 +1,7 @@ -import { useState, useEffect, useRef, useCallback } from 'react' +import { useState, useEffect, useCallback } from 'react' import { Link, useNavigate, useParams, useSearchParams } from 'react-router-dom' import ReactMarkdown from 'react-markdown' import remarkGfm from 'remark-gfm' -import rehypeRaw from 'rehype-raw' import api from '../api/client' import { useAuth } from '../context/AuthContext' import RichEditor from '../components/RichEditor' @@ -12,8 +11,9 @@ import './ArticlesPage.css' const sectionId = () => Array.from(crypto.getRandomValues(new Uint8Array(16)), b => b.toString(16).padStart(2, '0')).join('') function Markdown({ children, attemptId }) { + // Educator content renders as Markdown only; raw HTML is escaped, not executed. return ( - , a: ({ node, ...props }) => , }}> @@ -134,18 +134,27 @@ export function ArticlePage() { const load = useCallback(() => { api.get(`/articles/${id}`).then(res => { + const from = searchParams.get('section') + const sections = res.data.sections || [] setArticle(res.data) - if (!activeSection && res.data.sections?.length) setActiveSection(res.data.sections[0].id) - setForm({ title: res.data.title, slug: res.data.slug, summary: res.data.summary || '', content: res.data.content || '', sections: res.data.sections || [] }) + setActiveSection(prev => { + if (from && sections.some(s => s.id === from)) return from + return sections.some(s => s.id === prev) ? prev : (sections[0]?.id || '') + }) + setForm({ title: res.data.title, slug: res.data.slug, summary: res.data.summary || '', content: res.data.content || '', sections }) }).catch(err => setError(err.response?.status === 404 ? 'Article not found' : 'Could not load article')).finally(() => setLoading(false)) api.get(`/articles/${id}/questions`).then(res => setQuestions(res.data)).catch(() => setQuestions([])) api.get(`/articles/${id}/cards`).then(res => setCards(res.data)).catch(() => setCards([])) - }, [id, activeSection]) + }, [id, searchParams]) useEffect(() => { load() }, [load]) const section = article?.sections?.find(s => s.id === activeSection) - const chosenSection = searchParams.get('section') || '' + + const openSection = (secId) => { + setActiveSection(secId) + setSearchParams(secId ? { section: secId } : {}) + } const save = async (publish = null) => { setSaving(true) @@ -256,22 +265,22 @@ export function ArticlePage() { ) : (
- -