From d05e9cde321c98d5e8b3111294ac9036a56b0bda Mon Sep 17 00:00:00 2001 From: GGThed Date: Fri, 7 Aug 2026 19:56:27 -0400 Subject: [PATCH] fix(security): supprimer le XSS stocke du calendrier SEC-XSS-001. Chaine complete : un nom d'utilisateur libre arrivait dans le DOM d'un coach ou d'un administrateur, en meme origine, avec sa session. La CSP autorisant 'unsafe-inline', rien ne l'arretait. Cote serveur - la cause /matches/api/events construisait de la presentation dans un champ JSON : match_desc = participants_str + f"
{match.description}" Le navigateur deposait cette valeur telle quelle dans innerHTML. Les noms de joueurs y transitaient sans echappement -- et il ne pouvait pas y en avoir : c'est du JSON, pas du HTML. Les deux valeurs etaient deja des cles distinctes du payload. La concatenation faisait donc aussi afficher les participants deux fois dans le modal : une fois dans "Teams", une fois en tete de "Description". Corriger la faille corrige l'affichage. Cote navigateur - le sink showEventModal assemblait une chaine HTML puis l'affectait a innerHTML. Remplace par une construction de noeuds : makeEl / detailItem / multilineNode / teamNode passent tout texte par textContent. Les retours a la ligne d'une description restent rendus, via des
crees en dur. Les deux listes deroulantes concatenaient egalement titres de tryout et noms d'equipe dans innerHTML. Remplacees par new Option(), dont le premier argument est pose en texte. Verification 5 tests sur le contrat de l'API, dont un avec un nom d'utilisateur hostile ecrit directement en base -- ce que la validation refuse desormais, mais que des lignes anterieures peuvent contenir. Le JavaScript inline extrait passe `node --check`. Reste ouvert : la CSP autorise toujours 'unsafe-inline' (SEC-WEB-001), donc la defense en profondeur manque encore. Suivi en OPS-010. Co-Authored-By: Claude Opus 5 --- app/routes/matches.py | 13 ++- app/templates/pages/calendar.html | 156 ++++++++++++++++++++---------- tests/test_calendar_api.py | 145 +++++++++++++++++++++++++++ 3 files changed, 259 insertions(+), 55 deletions(-) create mode 100644 tests/test_calendar_api.py diff --git a/app/routes/matches.py b/app/routes/matches.py index a9dc0cf..6c42699 100644 --- a/app/routes/matches.py +++ b/app/routes/matches.py @@ -48,7 +48,12 @@ def api_events(): for tryout in tryouts: for match in tryout.matches: match_color = '#10b981' if match.match_type == 'team_vs_team' else '#f59e0b' - match_desc = match.description or '' + # 'description' used to be participants_str + '
' + description. + # Building presentation markup inside a JSON field is what carried + # the stored XSS: the browser dropped it straight into innerHTML, + # and player usernames travelled through it unescaped. The two + # values are already separate keys, so the concatenation also made + # the modal show the participants twice. participants_str = '' if match.match_type == 'team_vs_team': teams = [] @@ -56,14 +61,12 @@ def api_events(): teams.append(match.team1.name) if match.team2: teams.append(match.team2.name) - participants_str = f"{' vs '.join(teams)}" - match_desc = participants_str + (f"
{match.description}" if match.description else '') + participants_str = ' vs '.join(teams) else: player_names = [] for p in match.participants.all(): player_names.append(p.player.username if p.player else 'Unknown Player') participants_str = ', '.join(player_names) if player_names else 'No players' - match_desc = participants_str + (f"
{match.description}" if match.description else '') start_time_str = match.start_time.strftime('%H:%M') if match.start_time else None end_time_str = match.end_time.strftime('%H:%M') if match.end_time else None @@ -79,7 +82,7 @@ def api_events(): 'type': 'match', 'color': match_color, 'extendedProps': { 'location': match.location or tryout.location or 'TBD', - 'status': match.status, 'description': match_desc, + 'status': match.status, 'description': match.description or '', 'match_type': match.match_type, 'tryout_id': tryout.id, 'match_id': match.id, 'start_time': start_time_str, 'end_time': end_time_str, diff --git a/app/templates/pages/calendar.html b/app/templates/pages/calendar.html index b984ad0..2290230 100644 --- a/app/templates/pages/calendar.html +++ b/app/templates/pages/calendar.html @@ -205,14 +205,16 @@ function fetchTryoutOptions() { fetch('/matches/api/manageable-tryouts') .then(function(r) { return r.json(); }) .then(function(data) { + // new Option() sets the label as text; concatenating it into + // innerHTML let a tryout title carry markup into the page. var sel = document.getElementById('createTryoutSelect'); - sel.innerHTML = ''; + sel.replaceChildren(new Option('-- Select a tryout --', '')); var today = new Date().toISOString().split('T')[0]; data.forEach(function(t) { // Only show tryouts that haven't ended var tryoutEndDate = t.end_date || t.date; if (tryoutEndDate >= today) { - sel.innerHTML += ''; + sel.appendChild(new Option(t.title + ' (' + t.date + ')', t.id)); } }); }) @@ -224,70 +226,124 @@ function fetchTeamOptions() { .then(function(r) { return r.json(); }) .then(function(data) { var sel = document.getElementById('createTeamSelect'); - sel.innerHTML = ''; + sel.replaceChildren(new Option('-- Select a team --', '')); data.forEach(function(t) { - sel.innerHTML += ''; + sel.appendChild(new Option(t.name, t.id)); }); }) .catch(function() {}); } +// --------------------------------------------------------------------------- +// Safe DOM builders +// --------------------------------------------------------------------------- +// Everything rendered in the event modal originates from a JSON endpoint, +// where no HTML escaping applies. Text therefore goes through textContent, +// never through innerHTML. + +function makeEl(tag, className, text) { + var node = document.createElement(tag); + if (className) { node.className = className; } + if (text !== undefined && text !== null) { node.textContent = text; } + return node; +} + +function detailItem(label, valueNode, fullWidth) { + var item = makeEl('div', 'detail-item' + (fullWidth ? ' full-width' : '')); + item.appendChild(makeEl('span', 'detail-label', label)); + var value = makeEl('span', 'detail-value'); + value.appendChild(valueNode); + item.appendChild(value); + return item; +} + +// Renders newlines as
without letting any other markup through. +function multilineNode(text) { + var fragment = document.createDocumentFragment(); + String(text).split('\n').forEach(function(line, index) { + if (index > 0) { fragment.appendChild(document.createElement('br')); } + fragment.appendChild(document.createTextNode(line)); + }); + return fragment; +} + +// A team block: its name, plus an optional list of player names. +function teamNode(name, players) { + var team = makeEl('div', 'match-team'); + team.appendChild(makeEl('span', 'team-name', name)); + if (players) { + var list = makeEl('ul', 'team-players-list'); + players.forEach(function(player) { + list.appendChild(makeEl('li', null, player)); + }); + team.appendChild(list); + } + return team; +} + +function versusNode(left, right) { + var wrap = makeEl('div', 'match-teams'); + wrap.appendChild(left); + wrap.appendChild(makeEl('div', 'match-vs', 'vs')); + wrap.appendChild(right); + return wrap; +} + +function buildTeamsNode(props) { + var sides = props.participants.split(' vs '); + + if (props.match_type === 'team_vs_team' && sides.length >= 2) { + return versusNode(teamNode(sides[0]), teamNode(sides[1])); + } + + if (props.match_type === 'player_vs_player' && sides.length >= 2) { + return versusNode( + teamNode('Team 1', sides[0].split(', ')), + teamNode('Team 2', sides[1].split(', ')) + ); + } + + return document.createTextNode(props.participants); +} + function showEventModal(event) { var props = event.extendedProps; var title = event.title; var type = props.type; var date = event.start ? event.start.toDateString() : ''; - var content = '
'; - content += '
Type'; - content += ''; - content += (props.match_type === 'team_vs_team' ? 'Team Match' : (props.match_type === 'player_vs_player' ? 'Player Match' : 'Player Scrim')) + ''; - content += '
'; - content += '
Title' + title + '
'; - content += '
Date' + date + '
'; - content += '
Location' + (props.location || 'TBD') + '
'; - content += '
Status'; - content += '' + (props.status || 'scheduled') + ''; - content += '
'; - - // Add team separation for matches + // Built as DOM nodes rather than concatenated HTML. Every value below — + // match title, location, description, and above all the participant list, + // which is made of user-chosen usernames — comes from a JSON API and has + // never been HTML-escaped. Assigning it to innerHTML executed it. + var grid = makeEl('div', 'detail-grid'); + + var typeLabel = props.match_type === 'team_vs_team' ? 'Team Match' + : (props.match_type === 'player_vs_player' ? 'Player Match' : 'Player Scrim'); + var typeBadgeClass = (props.match_type === 'team_vs_team' || props.match_type === 'player_vs_player') + ? 'success' : 'warning'; + + grid.appendChild(detailItem('Type', makeEl('span', 'badge badge-' + typeBadgeClass, typeLabel))); + grid.appendChild(detailItem('Title', document.createTextNode(title))); + grid.appendChild(detailItem('Date', document.createTextNode(date))); + grid.appendChild(detailItem('Location', document.createTextNode(props.location || 'TBD'))); + + var status = props.status || 'scheduled'; + grid.appendChild(detailItem('Status', makeEl('span', 'badge badge-' + status, status))); + if (type === 'match' && props.participants) { - content += '
Teams'; - if (props.match_type === 'team_vs_team') { - var teams = props.participants.split(' vs '); - if (teams.length >= 2) { - content += '
'; - content += '
' + teams[0] + '
'; - content += '
vs
'; - content += '
' + teams[1] + '
'; - content += '
'; - } else { - content += props.participants; - } - } else if (props.match_type === 'player_vs_player') { - var parts = props.participants.split(' vs '); - if (parts.length >= 2) { - content += '
'; - content += '
Team 1
  • ' + parts[0].split(', ').join('
  • ') + '
'; - content += '
vs
'; - content += '
Team 2
  • ' + parts[1].split(', ').join('
  • ') + '
'; - content += '
'; - } else { - content += props.participants; - } - } else { - content += props.participants; - } - content += '
'; + grid.appendChild(detailItem('Teams', buildTeamsNode(props), true)); } - + if (props.description) { - content += '
Description' + props.description + '
'; + grid.appendChild(detailItem('Description', multilineNode(props.description), true)); } - content += '
'; - - document.getElementById('modalTitle').textContent = 'Match Details'; - document.getElementById('modalContent').innerHTML = content; + + var modalContent = document.getElementById('modalTitle'); + modalContent.textContent = 'Match Details'; + var target = document.getElementById('modalContent'); + target.textContent = ''; + target.appendChild(grid); // Reset buttons document.getElementById('deleteMatchBtn').style.display = 'none'; diff --git a/tests/test_calendar_api.py b/tests/test_calendar_api.py new file mode 100644 index 0000000..51427f6 --- /dev/null +++ b/tests/test_calendar_api.py @@ -0,0 +1,145 @@ +"""Contract of the calendar JSON API. + +SEC-XSS-001. /matches/api/events used to build presentation markup into its +`description` field: + + match_desc = participants_str + f"
{match.description}" + +The browser dropped that value straight into innerHTML, so a player username +— free text at the time — reached the DOM of every coach and administrator +who opened the calendar. Escaping cannot be the browser's job alone here; +the API must not emit markup in the first place. + +These tests assert the API contract. The DOM side is fixed in +calendar.html, which now builds nodes with textContent. +""" + +from datetime import date, time + +import pytest + +from app.extensions import db + + +@pytest.fixture +def match_factory(app): + """Create a tryout with one player_scrim match and one participant.""" + from app.models import ( + Match, MatchParticipant, Tryout, TryoutRegistration, + ) + + def _make(owner_id, player_id, *, description=None, username=None): + from app.models import User + + with app.app_context(): + if username is not None: + # Written straight to the column: edit_profile now rejects + # this, but rows predating that fix can still hold anything. + player = db.session.get(User, player_id) + player.username = username + + tryout = Tryout( + title='Spring tryout', game='Valorant', date=date(2030, 5, 1), + created_by=owner_id, status='upcoming', + ) + db.session.add(tryout) + db.session.flush() + + db.session.add(TryoutRegistration( + tryout_id=tryout.id, player_id=player_id)) + + match = Match( + tryout_id=tryout.id, title='Scrim A', description=description, + date=date(2030, 5, 2), start_time=time(18, 0), end_time=time(19, 0), + match_type='player_scrim', created_by=owner_id, + ) + db.session.add(match) + db.session.flush() + db.session.add(MatchParticipant( + match_id=match.id, player_id=player_id)) + db.session.commit() + return tryout.id, match.id + + return _make + + +def _match_event(payload): + return next(e for e in payload if e['id'].startswith('match_')) + + +class TestCalendarEventPayload: + def test_description_is_returned_verbatim( + self, app, client, as_role, make_user, match_factory + ): + player_id = make_user('player') + admin_id = as_role('admin') + match_factory(admin_id, player_id, description='Bring your own peripherals') + + props = _match_event(client.get('/matches/api/events').get_json())['extendedProps'] + + assert props['description'] == 'Bring your own peripherals' + + def test_description_does_not_carry_generated_markup( + self, app, client, as_role, make_user, match_factory + ): + """The
and the duplicated participant list are both gone.""" + player_id = make_user('player') + admin_id = as_role('admin') + match_factory(admin_id, player_id, description='Warm-up first') + + props = _match_event(client.get('/matches/api/events').get_json())['extendedProps'] + + assert '
' not in props['description'] + assert props['participants'] not in props['description'] + + def test_an_empty_description_stays_empty( + self, app, client, as_role, make_user, match_factory + ): + player_id = make_user('player') + admin_id = as_role('admin') + match_factory(admin_id, player_id, description=None) + + props = _match_event(client.get('/matches/api/events').get_json())['extendedProps'] + + assert props['description'] == '' + + def test_participants_remain_available_separately( + self, app, client, as_role, make_user, match_factory + ): + """Removing the concatenation must not lose the participant list: + the modal still renders it, from its own field.""" + from app.models import User + + player_id = make_user('player') + admin_id = as_role('admin') + match_factory(admin_id, player_id, description='x') + + with app.app_context(): + username = db.session.get(User, player_id).username + + props = _match_event(client.get('/matches/api/events').get_json())['extendedProps'] + + assert props['participants'] == username + + +class TestLegacyHostileData: + """Rows written before the username policy can still hold markup.""" + + PAYLOAD = '' + + def test_a_hostile_username_is_confined_to_its_own_field( + self, app, client, as_role, make_user, match_factory + ): + player_id = make_user('player') + admin_id = as_role('admin') + match_factory(admin_id, player_id, + description='Normal text', username=self.PAYLOAD) + + props = _match_event(client.get('/matches/api/events').get_json())['extendedProps'] + + # The payload is data: it may legitimately appear in `participants`, + # which the client renders through textContent. What must never + # happen again is it being spliced into a field the client treats + # as markup. + assert props['description'] == 'Normal text' + assert self.PAYLOAD not in props['description']