docs: Add Announcement Banner Configuration#3532
Conversation
Fix missing newline at the end of migration-banner.js
There was a problem hiding this comment.
Code Review
This pull request replaces the hardcoded migration banner with a configurable announcement banner that includes a close button and session-based persistence. Feedback on these changes highlights a potential DOM-based XSS vulnerability in custom-layout.js due to the use of innerHTML with configuration values, suggesting a refactor to use safer DOM APIs like textContent. Additionally, it is recommended to add disableBanner = true to the Cloudflare configuration to preserve the previous behavior of keeping the banner disabled by default on Cloudflare Pages.
| var wrapper = document.createElement('div'); | ||
| wrapper.id = 'banner-wrapper'; | ||
| wrapper.className = 'theme-banner-wrapper'; | ||
|
|
||
| var banner = document.createElement('div'); | ||
| banner.className = 'theme-banner'; | ||
|
|
||
| // If the URL starts with http/https, add target="_blank" and security attributes | ||
| var isExternal = BANNER_CONFIG.linkUrl.startsWith('http'); | ||
| var linkAttributes = isExternal ? ' target="_blank" rel="noopener noreferrer"' : ''; | ||
|
|
||
| banner.innerHTML = ` | ||
| <div class="theme-banner-content"> | ||
| <strong>${BANNER_CONFIG.title}</strong> ${BANNER_CONFIG.message} | ||
| <a href="${BANNER_CONFIG.linkUrl}"${linkAttributes}>${BANNER_CONFIG.linkText}</a>. | ||
| </div> | ||
| <button id="close-general-banner" class="close-btn" aria-label="Close">×</button> | ||
| `; | ||
| wrapper.appendChild(banner); | ||
|
|
||
| var contentArea = document.querySelector('.td-content') || document.querySelector('main'); | ||
| if (contentArea) { | ||
| contentArea.prepend(wrapper); | ||
| } | ||
| } | ||
|
|
||
| // Create the Wrapper | ||
| var wrapper = document.createElement('div'); | ||
| wrapper.id = 'migration-banner-wrapper'; | ||
| wrapper.className = 'theme-banner-wrapper'; | ||
|
|
||
| // Create the Banner | ||
| var banner = document.createElement('div'); | ||
| banner.className = 'theme-migration-banner'; | ||
| banner.innerHTML = '⚠️ <strong>Archived Docs:</strong> Visit <a href="https://mcp-toolbox.dev/">mcp-toolbox.dev</a> for the latest version.'; | ||
| wrapper.appendChild(banner); | ||
|
|
||
| // Inject the wrapper into the center information column | ||
| var contentArea = document.querySelector('.td-content') || document.querySelector('main'); | ||
| if (contentArea) { | ||
| contentArea.prepend(wrapper); | ||
| } else { | ||
| console.warn("Could not find the main content column to inject the banner."); | ||
| var closeBtn = document.getElementById('close-general-banner'); | ||
| if (closeBtn) { | ||
| closeBtn.addEventListener('click', function() { | ||
| wrapper.style.display = 'none'; | ||
| sessionStorage.setItem(storageKey, "true"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Using innerHTML with template literals containing unescaped configuration values (BANNER_CONFIG.title, BANNER_CONFIG.message, etc.) introduces a potential DOM-based Cross-Site Scripting (XSS) vulnerability. Additionally, performing a DOM lookup with document.getElementById for the close button is redundant since the element is created dynamically in the same scope.
Using safe DOM APIs like textContent and createTextNode completely eliminates the XSS risk and allows direct event listener attachment without subsequent DOM queries.
var wrapper = document.createElement('div');
wrapper.id = 'banner-wrapper';
wrapper.className = 'theme-banner-wrapper';
var banner = document.createElement('div');
banner.className = 'theme-banner';
var contentDiv = document.createElement('div');
contentDiv.className = 'theme-banner-content';
var strong = document.createElement('strong');
strong.textContent = BANNER_CONFIG.title;
contentDiv.appendChild(strong);
contentDiv.appendChild(document.createTextNode(' ' + BANNER_CONFIG.message + ' '));
var link = document.createElement('a');
link.href = BANNER_CONFIG.linkUrl;
link.textContent = BANNER_CONFIG.linkText;
if (BANNER_CONFIG.linkUrl.startsWith('http')) {
link.setAttribute('target', '_blank');
link.setAttribute('rel', 'noopener noreferrer');
}
contentDiv.appendChild(link);
contentDiv.appendChild(document.createTextNode('.'));
var closeBtn = document.createElement('button');
closeBtn.className = 'close-btn';
closeBtn.setAttribute('aria-label', 'Close');
closeBtn.textContent = '\u00D7';
closeBtn.addEventListener('click', function() {
wrapper.style.display = 'none';
sessionStorage.setItem(storageKey, "true");
});
banner.appendChild(contentDiv);
banner.appendChild(closeBtn);
wrapper.appendChild(banner);
var contentArea = document.querySelector('.td-content') || document.querySelector('main');
if (contentArea) {
contentArea.prepend(wrapper);
}| offlineSearch = false | ||
| version_menu = "Releases" | ||
| disableMigrationBanner = true | ||
| releases_url = "/releases.releases" |
There was a problem hiding this comment.
The previous configuration disableMigrationBanner = true was removed, but it was not replaced with disableBanner = true. This will cause the new announcement banner to be enabled on Cloudflare Pages by default. If the intention is to keep the banner disabled on Cloudflare Pages (matching the previous behavior), please add disableBanner = true.
| releases_url = "/releases.releases" | |
| disableBanner = true | |
| releases_url = "/releases.releases" |
This PR adds a general banner configuration with a sessionStorage to make the banner closable for a session for a user.
Before merging, please alter the title, message, linkText and linkURL