Alternative MPD live to live sample page #144
Conversation
Summary of ChangesHello @cotid-qualabs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a new interactive sample page to demonstrate DASH.js's ability to manage live-to-live alternative media presentations. The sample provides a user-friendly interface for configuring and testing dynamic 'replace events' within a live streaming context, allowing users to define when and how an alternative live stream should temporarily replace the main live stream. This enhancement helps showcase advanced live streaming capabilities and provides a practical tool for developers to experiment with alternative content switching. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new sample page for live-to-live alternative media presentations. The implementation is well-structured, but I've identified a few areas for improvement concerning JavaScript best practices, code quality, and security. Specifically, there's a potential Cross-Site Scripting (XSS) vulnerability in the manifest viewer that should be addressed. I've also included suggestions to refactor the event display logic for better maintainability and to clean up some redundant code in the initialization function.
| let highlighted = manifestXml | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/(<\/?)(\w+)(.*?)(\/?>)/g, (match, open, tagName, attrs, close) => { | ||
| let isAltMpd = (tagName === "ReplacePresentation"); | ||
| let coloredTag = `<span class="token.tag${isAltMpd ? ' token.altmpd' : ''}">${open}${tagName}</span>`; | ||
| if (attrs.trim().length > 0) { | ||
| attrs = attrs.replace(/(\w+)="(.*?)"/g, `<span class="token.attr-name">$1</span>="<span class="token.attr-value">$2</span>"`); | ||
| coloredTag += attrs; | ||
| } | ||
| coloredTag += `<span class="token.tag${isAltMpd ? ' token.altmpd' : ''}">${close}</span>`; | ||
| return coloredTag; | ||
| }); | ||
|
|
||
| viewer.innerHTML = highlighted; |
There was a problem hiding this comment.
The updateManifestViewer function is vulnerable to Cross-Site Scripting (XSS). It constructs HTML using innerHTML and includes user-provided data from the 'Alternative Live Stream URL' input field. The regex-based syntax highlighting does not properly sanitize attribute values, allowing a malicious user to inject arbitrary HTML attributes and event handlers (e.g., onmouseover). For example, an input like http://a.com" onmouseover="alert('XSS') would result in executable JavaScript.
To fix this, you should avoid building HTML with string concatenation. Instead, create DOM elements programmatically and set user-provided content using textContent to ensure it's treated as plain text.
| var video = document.getElementById('video-element'); | ||
| var alternativeVideo = document.getElementById('alternative-video-element'); | ||
|
|
There was a problem hiding this comment.
The variables video and alternativeVideo are declared but never used within the init function. They are retrieved again later in the loadPlayer function. To improve code clarity and avoid redundancy, these unused variable declarations should be removed. Additionally, var is used here, while the rest of the code uses let and const. Removing these lines will also improve consistency.
| eventDiv.innerHTML = ` | ||
| <div> | ||
| <strong>Replace Event #${event.id + 1}</strong><br> | ||
| <small>Time: ${timeString} (${event.timeOffset}s from now)<br> | ||
| Duration: ${event.duration}ms | Return Offset: ${event.returnOffset}ms</small> | ||
| </div> | ||
| <button class="btn btn-sm btn-danger" onclick="removeEvent(${index})">Remove</button> | ||
| `; |
There was a problem hiding this comment.
Using innerHTML with an inline onclick event handler is not a recommended practice. It couples the HTML tightly with the JavaScript logic, can be blocked by Content Security Policies (CSP), and requires the removeEvent function to be globally accessible. A better approach is to create the elements programmatically using document.createElement and attach event listeners using addEventListener. This makes the code cleaner, safer, and more compliant with modern web standards.
| eventDiv.innerHTML = ` | |
| <div> | |
| <strong>Replace Event #${event.id + 1}</strong><br> | |
| <small>Time: ${timeString} (${event.timeOffset}s from now)<br> | |
| Duration: ${event.duration}ms | Return Offset: ${event.returnOffset}ms</small> | |
| </div> | |
| <button class="btn btn-sm btn-danger" onclick="removeEvent(${index})">Remove</button> | |
| `; | |
| const detailsDiv = document.createElement('div'); | |
| detailsDiv.innerHTML = `<strong>Replace Event #${event.id + 1}</strong><br> | |
| <small>Time: ${timeString} (${event.timeOffset}s from now)<br> | |
| Duration: ${event.duration}ms | Return Offset: ${event.returnOffset}ms</small>`; | |
| const removeButton = document.createElement('button'); | |
| removeButton.className = 'btn btn-sm btn-danger'; | |
| removeButton.textContent = 'Remove'; | |
| removeButton.addEventListener('click', () => removeEvent(index)); | |
| eventDiv.append(detailsDiv, removeButton); |
sebastianpiq
merged commit b68b73f
into
feature/alternative-media-presentations
No description provided.