fix: use innerHTML instead of textContent in createElement helper#92
fix: use innerHTML instead of textContent in createElement helper#92simgunz wants to merge 3 commits into
Conversation
| const el = document.createElement(tag); | ||
| if (className) el.className = className; | ||
| if (innerHTML) el.textContent = innerHTML; | ||
| if (innerHTML) el.innerHTML = innerHTML; |
There was a problem hiding this comment.
Appreciate this! One thing to note, now that this uses innerHTML, a couple of the call sites pass interpolated data without escaping:
updateNonMatchesTable(line 379) -row.doc_id,row.field_path, etc. go straight into<td>tagsloadPDF(line 541) -docIdgets interpolated intoonclickhandlers, which could allow string breakout
There's already an escapeHtml helper at line 394, would you mind wrapping those interpolations with it? Low risk since it's a local report, but good to close the door on it while we're here. Really appreciate it!
There was a problem hiding this comment.
Nice, thanks for adding the escaping! One more thing, the escapeHtml is defined locally inside updateDocumentFiles here, but it's now called from updateNonMatchesTable (line 380) and loadPDF (line 541) too, which are separate functions. Those calls will hit a ReferenceError at runtime since it's not in scope there. Would you mind hoisting this up to module scope near the other helpers (around line 29 by createElement)? That way every call site can reach it. Rest of the changes look great!
There was a problem hiding this comment.
Updated. It should be ok now.
|
Hi @simgunz, appreciate the PR! Any questions or update on the requested changes? |
The createElement helper named its third parameter 'innerHTML' but assigned it via textContent, causing HTML strings (filter controls, table rows, PDF navigation) to render as escaped text instead of being interpreted as HTML.
Move escapeHtml from inside updateDocumentFiles to module scope next to createElement, making it accessible to all functions.
Wrap user-controlled values with escapeHtml in updateNonMatchesTable and loadPDF to prevent cross-site scripting via doc_id, field_path, and other interpolated fields.
simgunz
force-pushed
the
fix/createElement-innerHTML-rendering
branch
from
b2dc37f to
5f88ec3
Compare
adiadd
left a comment
adiadd
left a comment
There was a problem hiding this comment.
Taking another look on this, the textContent → innerHTML flip and the table-row escaping look great. The two spots I'd like to nail down before merging are both in the PDF nav area (left inline comments). Once those are sorted this is good to go. Really appreciate you sticking with the back-and-forth on this one!
| const safeDocId = escapeHtml(docId); | ||
| const navControls = createElement('div', 'pdf-navigation', ` | ||
| <button class="btn btn-primary pdf-nav-btn" id="prev-${docId}" onclick="navigatePDF('${docId}', -1)">← Previous</button> | ||
| <span class="pdf-page-info" id="page-info-${docId}">Page 1 of ${pdf.numPages}</span> | ||
| <button class="btn btn-primary pdf-nav-btn" id="next-${docId}" onclick="navigatePDF('${docId}', 1)">Next →</button> | ||
| <button class="btn btn-primary pdf-nav-btn" id="prev-${safeDocId}" onclick="navigatePDF('${safeDocId}', -1)">← Previous</button> | ||
| <span class="pdf-page-info" id="page-info-${safeDocId}">Page 1 of ${pdf.numPages}</span> | ||
| <button class="btn btn-primary pdf-nav-btn" id="next-${safeDocId}" onclick="navigatePDF('${safeDocId}', 1)">Next →</button> |
There was a problem hiding this comment.
appreciate the escaping pass, one thing - the escapeHtml helper only covers &, <, > (since it round-trips through textContent → innerHTML). It doesn't escape " or ', which means in this block:
<button ... id="prev-${safeDocId}" onclick="navigatePDF('${safeDocId}', -1)">
a docId with a " in it can break out of the id attribute, and a docId with a ' can break out of the onclick JS string. So the door's still cracked open here.
i think the cleanest fix imo is to ditch the HTML-string template for the nav and build it as DOM + addEventListener, same pattern we're already using in updateDocumentFiles:
const navControls = createElement('div', 'pdf-navigation');
const prev = document.createElement('button');
prev.className = 'btn btn-primary pdf-nav-btn';
prev.id = `prev-${docId}`;
prev.textContent = '← Previous';
prev.addEventListener('click', () => navigatePDF(docId, -1));
// …same for the page-info span and next button, then append all three…
this way there's no HTML parse on user-controlled data at all and we don't need escapeHtml here. ff you'd rather keep the template and do the minimal patch, we'd need to extend escapeHtml to also cover " and ', lmk which you prefer. thanks!
|
|
||
| const pdfContainer = createElement('div', 'pdf-container'); | ||
| const canvas = document.createElement('canvas'); | ||
| canvas.id = `pdf-canvas-${escapeHtml(docId)}`; |
There was a problem hiding this comment.
escapeHtml shouldn't be used on these since .id is a DOM property assignment, not an HTML parse. if a docId ever contains & or <, this stores the id as e.g. pdf-canvas-foo&bar literally, but then line 430 / 575 / 601-602 look it up using the raw docId and getElement returns null → PDF never renders and nav buttons don't bind.
should just be raw docId here, same as before:
canvas.id = `pdf-canvas-${docId}`;
loading.id = `pdf-loading-${docId}`;
error.id = `pdf-error-${docId}`;
escaping only matters where the value gets parsed as HTML/attr/JS
| canvas.id = `pdf-canvas-${escapeHtml(docId)}`; | ||
| canvas.className = 'pdf-canvas'; | ||
| const loading = createElement('div', 'pdf-loading'); | ||
| loading.id = `pdf-loading-${escapeHtml(docId)}`; |
| loading.id = `pdf-loading-${escapeHtml(docId)}`; | ||
| loading.textContent = 'Loading PDF...'; | ||
| const error = createElement('div', 'pdf-error'); | ||
| error.id = `pdf-error-${escapeHtml(docId)}`; |
There was a problem hiding this comment.
ditto as the canvas.id and the loading.id
|
Hey @simgunz, once again thank you so much for the PR and apologies for the multiple back-and-forth comments! Any questions or update on the requested changes? |
2 participants
Issue #, if available: N/A
Description of changes:
The
createElementutility inmain.jsnames its third parameterinnerHTMLbut assigns it viael.textContent, which escapes HTML. This causes dynamically injected elements (report filter controls, non-match table rows, PDF navigation buttons) to render as raw HTML strings instead of interactive UI components.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Before the fix:
