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"<br>{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 <br> 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 <[email protected]>
This commit is contained in:
GGThed
@@ -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 + '<br>' + 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"<br>{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"<br>{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,
|
||||
|
||||
@@ -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 = '<option value="">-- Select a tryout --</option>';
|
||||
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 += '<option value="' + t.id + '">' + t.title + ' (' + t.date + ')</option>';
|
||||
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 = '<option value="">-- Select a team --</option>';
|
||||
sel.replaceChildren(new Option('-- Select a team --', ''));
|
||||
data.forEach(function(t) {
|
||||
sel.innerHTML += '<option value="' + t.id + '">' + t.name + '</option>';
|
||||
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 <br> 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 = '<div class="detail-grid">';
|
||||
content += '<div class="detail-item"><span class="detail-label">Type</span><span class="detail-value">';
|
||||
content += '<span class="badge badge-' + (props.match_type === 'team_vs_team' || props.match_type === 'player_vs_player' ? 'success' : 'warning') + '">';
|
||||
content += (props.match_type === 'team_vs_team' ? 'Team Match' : (props.match_type === 'player_vs_player' ? 'Player Match' : 'Player Scrim')) + '</span>';
|
||||
content += '</span></div>';
|
||||
content += '<div class="detail-item"><span class="detail-label">Title</span><span class="detail-value">' + title + '</span></div>';
|
||||
content += '<div class="detail-item"><span class="detail-label">Date</span><span class="detail-value">' + date + '</span></div>';
|
||||
content += '<div class="detail-item"><span class="detail-label">Location</span><span class="detail-value">' + (props.location || 'TBD') + '</span></div>';
|
||||
content += '<div class="detail-item"><span class="detail-label">Status</span><span class="detail-value">';
|
||||
content += '<span class="badge badge-' + (props.status || 'scheduled') + '">' + (props.status || 'scheduled') + '</span>';
|
||||
content += '</span></div>';
|
||||
|
||||
// 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 += '<div class="detail-item full-width"><span class="detail-label">Teams</span><span class="detail-value">';
|
||||
if (props.match_type === 'team_vs_team') {
|
||||
var teams = props.participants.split(' vs ');
|
||||
if (teams.length >= 2) {
|
||||
content += '<div class="match-teams">';
|
||||
content += '<div class="match-team"><span class="team-name">' + teams[0] + '</span></div>';
|
||||
content += '<div class="match-vs">vs</div>';
|
||||
content += '<div class="match-team"><span class="team-name">' + teams[1] + '</span></div>';
|
||||
content += '</div>';
|
||||
} else {
|
||||
content += props.participants;
|
||||
}
|
||||
} else if (props.match_type === 'player_vs_player') {
|
||||
var parts = props.participants.split(' vs ');
|
||||
if (parts.length >= 2) {
|
||||
content += '<div class="match-teams">';
|
||||
content += '<div class="match-team"><span class="team-name">Team 1</span><ul class="team-players-list"><li>' + parts[0].split(', ').join('</li><li>') + '</li></ul></div>';
|
||||
content += '<div class="match-vs">vs</div>';
|
||||
content += '<div class="match-team"><span class="team-name">Team 2</span><ul class="team-players-list"><li>' + parts[1].split(', ').join('</li><li>') + '</li></ul></div>';
|
||||
content += '</div>';
|
||||
} else {
|
||||
content += props.participants;
|
||||
}
|
||||
} else {
|
||||
content += props.participants;
|
||||
}
|
||||
content += '</span></div>';
|
||||
grid.appendChild(detailItem('Teams', buildTeamsNode(props), true));
|
||||
}
|
||||
|
||||
|
||||
if (props.description) {
|
||||
content += '<div class="detail-item full-width"><span class="detail-label">Description</span><span class="detail-value">' + props.description + '</span></div>';
|
||||
grid.appendChild(detailItem('Description', multilineNode(props.description), true));
|
||||
}
|
||||
content += '</div>';
|
||||
|
||||
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';
|
||||
|
||||
@@ -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"<br>{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 <br> 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 '<br>' 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 = '<img src=x onerror=alert(1)>'
|
||||
|
||||
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']
|
||||
Reference in New Issue
Block a user