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']