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.
This commit is contained in:
Daniel
parent
d5269d7772
commit
6c85f8b4a9
5 changed files with 112 additions and 23 deletions
|
|
@ -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}
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<ReactMarkdown remarkPlugins={[remarkGfm]} rehypePlugins={[rehypeRaw]} components={{
|
||||
<ReactMarkdown remarkPlugins={[remarkGfm]} components={{
|
||||
img: ({ node, src, ...props }) => <img {...props} src={markdownImageUrl(src, attemptId)} />,
|
||||
a: ({ node, ...props }) => <a {...props} target="_blank" rel="noopener noreferrer" />,
|
||||
}}>
|
||||
|
|
@ -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() {
|
|||
</div>
|
||||
) : (
|
||||
<div className="article-layout">
|
||||
<button className="article-drawer-toggle" onClick={() => setDrawerOpen(v => !v)} aria-expanded={drawerOpen}>
|
||||
<button className="article-drawer-toggle" onClick={() => setDrawerOpen(v => !v)} aria-expanded={drawerOpen} aria-controls="article-sections">
|
||||
{drawerOpen ? '✕ Close sections' : '☰ Sections'}
|
||||
</button>
|
||||
<aside className={`article-sections ${drawerOpen ? 'open' : ''}`}>
|
||||
<aside id="article-sections" className={`article-sections ${drawerOpen ? 'open' : ''}`}>
|
||||
<h4>In this article</h4>
|
||||
<ul>
|
||||
<li>
|
||||
<button className={!chosenSection && !activeSection ? 'section-link active' : 'section-link'}
|
||||
onClick={() => { setActiveSection(''); setSearchParams({}) }}>
|
||||
<button className={!activeSection ? 'section-link active' : 'section-link'}
|
||||
onClick={() => openSection('')}>
|
||||
Overview
|
||||
</button>
|
||||
</li>
|
||||
{(article.sections || []).map(sec => (
|
||||
<li key={sec.id}>
|
||||
<button className={activeSection === sec.id && !chosenSection ? 'section-link active' : 'section-link'}
|
||||
onClick={() => { setActiveSection(sec.id); setSearchParams({}) }}>
|
||||
<button className={activeSection === sec.id ? 'section-link active' : 'section-link'}
|
||||
onClick={() => openSection(sec.id)}>
|
||||
{sec.title}
|
||||
</button>
|
||||
</li>
|
||||
|
|
|
|||
|
|
@ -56,4 +56,32 @@ describe('topic reading', () => {
|
|||
render(<MemoryRouter><QuestionReadingLinks questionId={9} /></MemoryRouter>)
|
||||
expect(screen.queryByTestId('question-reading')).not.toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('renders educator markdown without executing raw HTML', async () => {
|
||||
api.get.mockImplementation(url => {
|
||||
if (url === '/articles/1') return Promise.resolve({ data: { ...article, content: '<img src=x onerror="alert(1)">Intro', sections: [] } })
|
||||
if (url === '/articles/1/questions') return Promise.resolve({ data: [] })
|
||||
if (url === '/articles/1/cards') return Promise.resolve({ data: [] })
|
||||
return Promise.resolve({ data: [] })
|
||||
})
|
||||
const { container } = render(<MemoryRouter initialEntries={['/articles/1']}><Routes><Route path="/articles/:id" element={<ArticlePage />} /></Routes></MemoryRouter>)
|
||||
expect(await screen.findByText(/Intro/)).toBeInTheDocument()
|
||||
expect(container.querySelector('img')).toBeNull()
|
||||
})
|
||||
|
||||
it('honors a linked section deep link and marks the active nav item', async () => {
|
||||
const twoSections = { ...article, sections: [
|
||||
{ id: 'a'.repeat(32), slug: 'first', title: 'First section', content: 'First body' },
|
||||
{ id: 'b'.repeat(32), slug: 'second', title: 'Second section', content: 'Second body' },
|
||||
] }
|
||||
api.get.mockImplementation(url => {
|
||||
if (url === '/articles/1') return Promise.resolve({ data: twoSections })
|
||||
if (url === '/articles/1/questions') return Promise.resolve({ data: [] })
|
||||
if (url === '/articles/1/cards') return Promise.resolve({ data: [] })
|
||||
return Promise.resolve({ data: [] })
|
||||
})
|
||||
render(<MemoryRouter initialEntries={[`/articles/1?section=${'b'.repeat(32)}`]}><Routes><Route path="/articles/:id" element={<ArticlePage />} /></Routes></MemoryRouter>)
|
||||
expect(await screen.findByText('Second body')).toBeInTheDocument()
|
||||
expect(screen.getByRole('button', { name: 'Second section' })).toHaveClass('active')
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Reference in a new issue