fix: address Copilot review comments on PR 357
- Escape all interpolated text in renderDiscoverSyncCard innerHTML to prevent XSS - Replace inline onclick/onchange handlers with addEventListener bindings - Remove familiar_favorites hardcoded track_count of 0, use pool_count like other personalized playlists - Guard request.get_json() against None in manage_discover_auto_update POST handler - Set push status to skipped when playlist_name is missing in _push_playlist_to_server
This commit is contained in:
parent
f3357cb90d
commit
5ce2030655
2 changed files with 35 additions and 16 deletions
|
|
@ -32773,6 +32773,8 @@ def _push_playlist_to_server(batch_id, batch):
|
||||||
playlist_id = batch.get('playlist_id', '')
|
playlist_id = batch.get('playlist_id', '')
|
||||||
playlist_name = batch.get('playlist_name', '')
|
playlist_name = batch.get('playlist_name', '')
|
||||||
if not playlist_name:
|
if not playlist_name:
|
||||||
|
logger.info(f"[PlaylistPush] No playlist_name for batch {batch_id} - skipping server push")
|
||||||
|
database.update_sync_history_push_status(batch_id, 'skipped')
|
||||||
return
|
return
|
||||||
|
|
||||||
database.update_sync_history_push_status(batch_id, 'pushing')
|
database.update_sync_history_push_status(batch_id, 'pushing')
|
||||||
|
|
@ -44211,10 +44213,7 @@ def get_discover_synced_playlists():
|
||||||
track_count = 0
|
track_count = 0
|
||||||
else:
|
else:
|
||||||
# Personalized playlists come from the discovery pool
|
# Personalized playlists come from the discovery pool
|
||||||
# familiar_favorites is not implemented — always report 0
|
if pool_count > 0:
|
||||||
if ptype == 'familiar_favorites':
|
|
||||||
track_count = 0
|
|
||||||
elif pool_count > 0:
|
|
||||||
track_count = min(50, pool_count)
|
track_count = min(50, pool_count)
|
||||||
else:
|
else:
|
||||||
track_count = 0
|
track_count = 0
|
||||||
|
|
@ -44310,10 +44309,13 @@ def manage_discover_auto_update():
|
||||||
settings[key] = bool(val)
|
settings[key] = bool(val)
|
||||||
return jsonify({"success": True, "settings": settings})
|
return jsonify({"success": True, "settings": settings})
|
||||||
|
|
||||||
data = request.get_json()
|
data = request.get_json(silent=True) or {}
|
||||||
playlist_type = data.get('playlist_type')
|
playlist_type = data.get('playlist_type')
|
||||||
enabled = data.get('enabled', False)
|
enabled = data.get('enabled', False)
|
||||||
|
|
||||||
|
if not playlist_type:
|
||||||
|
return jsonify({"success": False, "error": "Missing playlist_type"}), 400
|
||||||
|
|
||||||
is_lb_type = playlist_type and playlist_type.startswith('listenbrainz_')
|
is_lb_type = playlist_type and playlist_type.startswith('listenbrainz_')
|
||||||
if playlist_type not in valid_types and not is_lb_type:
|
if playlist_type not in valid_types and not is_lb_type:
|
||||||
return jsonify({"success": False, "error": f"Invalid playlist type: {playlist_type}"}), 400
|
return jsonify({"success": False, "error": f"Invalid playlist type: {playlist_type}"}), 400
|
||||||
|
|
|
||||||
|
|
@ -9110,17 +9110,17 @@ function renderDiscoverSyncCard(playlist, container, sourceLabel) {
|
||||||
const trackLabel = isEmpty ? 'No tracks yet' : `${playlist.track_count} tracks`;
|
const trackLabel = isEmpty ? 'No tracks yet' : `${playlist.track_count} tracks`;
|
||||||
|
|
||||||
card.innerHTML = `
|
card.innerHTML = `
|
||||||
<div class="discover-sync-card-icon">${playlist.icon}</div>
|
<div class="discover-sync-card-icon">${_esc(playlist.icon)}</div>
|
||||||
<div class="discover-sync-card-info">
|
<div class="discover-sync-card-info">
|
||||||
<div class="discover-sync-card-name">${playlist.name}
|
<div class="discover-sync-card-name">${_esc(playlist.name)}
|
||||||
<span class="discover-sync-card-meta-inline">
|
<span class="discover-sync-card-meta-inline">
|
||||||
<span class="discover-sync-source-badge">${sourceLabel || 'unknown'}</span>
|
<span class="discover-sync-source-badge">${_esc(sourceLabel || 'unknown')}</span>
|
||||||
<span class="discover-sync-separator">\u00b7</span>
|
<span class="discover-sync-separator">\u00b7</span>
|
||||||
<span class="discover-sync-track-count">${trackLabel}</span>
|
<span class="discover-sync-track-count">${_esc(trackLabel)}</span>
|
||||||
<span class="discover-sync-separator">\u00b7</span>
|
<span class="discover-sync-separator">\u00b7</span>
|
||||||
<span class="discover-sync-status ${statusClass}">${statusText}</span>
|
<span class="discover-sync-status ${statusClass}">${_esc(statusText)}</span>
|
||||||
<span class="discover-sync-separator">\u00b7</span>
|
<span class="discover-sync-separator">\u00b7</span>
|
||||||
<span class="discover-sync-last-synced">${lastSyncedText}</span>
|
<span class="discover-sync-last-synced">${_esc(lastSyncedText)}</span>
|
||||||
</span>
|
</span>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
@ -9128,26 +9128,43 @@ function renderDiscoverSyncCard(playlist, container, sourceLabel) {
|
||||||
<div class="discover-sync-toggle-wrapper" title="${isEmpty ? 'No tracks available — visit Discover first' : 'Keep this playlist updated automatically'}">
|
<div class="discover-sync-toggle-wrapper" title="${isEmpty ? 'No tracks available — visit Discover first' : 'Keep this playlist updated automatically'}">
|
||||||
<label class="discover-sync-toggle-label">Keep updated</label>
|
<label class="discover-sync-toggle-label">Keep updated</label>
|
||||||
<label class="discover-sync-toggle">
|
<label class="discover-sync-toggle">
|
||||||
<input type="checkbox" ${playlist.auto_update ? 'checked' : ''} ${isEmpty ? 'disabled' : ''}
|
<input type="checkbox" class="discover-auto-update-toggle" ${playlist.auto_update ? 'checked' : ''} ${isEmpty ? 'disabled' : ''}>
|
||||||
onchange="toggleDiscoverAutoUpdate('${playlist.type}', this.checked)">
|
|
||||||
<span class="discover-sync-toggle-slider"></span>
|
<span class="discover-sync-toggle-slider"></span>
|
||||||
</label>
|
</label>
|
||||||
</div>
|
</div>
|
||||||
<div class="discover-sync-toggle-wrapper" title="Coming soon: download any available quality for this batch, even if it's below your global quality profile. Useful for rotating discover playlists where quantity matters more than quality.">
|
<div class="discover-sync-toggle-wrapper" title="Coming soon: download any available quality for this batch, even if it's below your global quality profile. Useful for rotating discover playlists where quantity matters more than quality.">
|
||||||
<label class="discover-sync-toggle-label" style="opacity:0.5">Any Quality</label>
|
<label class="discover-sync-toggle-label" style="opacity:0.5">Any Quality</label>
|
||||||
<label class="discover-sync-toggle" style="opacity:0.5;cursor:not-allowed">
|
<label class="discover-sync-toggle" style="opacity:0.5;cursor:not-allowed">
|
||||||
<input type="checkbox" id="discover-any-quality-${playlist.type}" disabled>
|
<input type="checkbox" class="discover-any-quality-toggle" disabled>
|
||||||
<span class="discover-sync-toggle-slider"></span>
|
<span class="discover-sync-toggle-slider"></span>
|
||||||
</label>
|
</label>
|
||||||
</div>
|
</div>
|
||||||
<button class="discover-sync-btn" id="discover-sync-btn-${playlist.type}"
|
<button class="discover-sync-btn"
|
||||||
onclick="syncDiscoverPlaylistFromTab('${playlist.type}', '${playlist.name}')"
|
|
||||||
${playlist.sync_status === 'syncing' || isEmpty ? 'disabled' : ''}>
|
${playlist.sync_status === 'syncing' || isEmpty ? 'disabled' : ''}>
|
||||||
\u27f3 Sync Now
|
\u27f3 Sync Now
|
||||||
</button>
|
</button>
|
||||||
</div>
|
</div>
|
||||||
`;
|
`;
|
||||||
|
|
||||||
|
// Bind event listeners instead of inline handlers (avoids XSS from playlist names)
|
||||||
|
const autoUpdateToggle = card.querySelector('.discover-auto-update-toggle');
|
||||||
|
if (autoUpdateToggle) {
|
||||||
|
autoUpdateToggle.addEventListener('change', function() {
|
||||||
|
toggleDiscoverAutoUpdate(playlist.type, this.checked);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
const anyQualityToggle = card.querySelector('.discover-any-quality-toggle');
|
||||||
|
if (anyQualityToggle) {
|
||||||
|
anyQualityToggle.id = `discover-any-quality-${playlist.type}`;
|
||||||
|
}
|
||||||
|
|
||||||
|
const syncButton = card.querySelector('.discover-sync-btn');
|
||||||
|
if (syncButton) {
|
||||||
|
syncButton.id = `discover-sync-btn-${playlist.type}`;
|
||||||
|
syncButton.addEventListener('click', () => syncDiscoverPlaylistFromTab(playlist.type, playlist.name));
|
||||||
|
}
|
||||||
|
|
||||||
// Make the icon + info area clickable to view tracks
|
// Make the icon + info area clickable to view tracks
|
||||||
if (!isEmpty) {
|
if (!isEmpty) {
|
||||||
const clickArea = card.querySelector('.discover-sync-card-info');
|
const clickArea = card.querySelector('.discover-sync-card-info');
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue