Split points by documents #120
Conversation
|
next i want to add an index of documents at the top, maybe with a count of points for each classification per doc |
|
Can we perhaps make this a setting? I think most users would prefer the way we show the points right now - all in one place |
Yea, should we move it to a separate module for clarity? |
|
IMO it improves the clarity and makes the list easier to read because you get that little bit more context. |
|
BTW I believe the settings button at the bottom broke for whatever reason ;D |
|
WalkthroughAdds a new pointListStyle preference ("docCategories" | "unified"), persists it during install and via Settings, exposes a getter in popup state, and updates popup rendering to branch between a document-categorized view and a unified list with new helpers, types, DOM sections, and CSS selectors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant LocalStorage
participant Popup
participant Service
User->>Settings: open settings
Settings->>LocalStorage: read pointListStyle
LocalStorage-->>Settings: return pointListStyle
Settings->>User: show current selection
User->>Settings: change Summary Style
Settings->>LocalStorage: save pointListStyle
LocalStorage-->>Settings: persisted
User->>Popup: open popup
Popup->>LocalStorage: hydrate preferences (pointListStyle)
LocalStorage-->>Popup: return pointListStyle
Popup->>Service: request service points (+ documents)
Service-->>Popup: ServiceResponse (points, documents)
alt pointListStyle == "docCategories"
Popup->>Popup: populateListDocCategories(points, documents)
Popup->>Popup: build document index, per-document sections, unlinked points
else pointListStyle == "unified"
Popup->>Popup: populateListUnified(points)
Popup->>Popup: filter & render grouped unified list
end
Popup->>User: render popup list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/views/style/popup.css (1)
187-191: Fix the typo in the closing div tag.Line 190 has a typo:
/div>should be</div>.Apply this diff:
<div class=""> <div id="pointList" class="pointList"> <a style="display: none">...</a> - /div> + </div> </div>`src/scripts/views/popup/service.ts (1)
183-210: Critical: XSS vulnerability and syntax error in populateListUnified.This function has two issues:
- Line 190: Syntax error -
/div>should be</div>- Line 192: Using
innerHTMLwithout sanitization is a potential XSS vector, though the template is static in this case.Apply this diff to fix the syntax error:
const temp = ` <div class=""> <div id="pointList" class="pointList"> <a style="display: none">...</a> - /div> + </div> </div>`The innerHTML usage here is safe since
tempcontains only static HTML, but consider documenting this or using a safer approach for consistency.
🧹 Nitpick comments (6)
src/scripts/background/install.ts (1)
16-16: Consider adding type safety for the default value.The string literal
"docCategories"is not type-checked against the union type"docCategories" | "unified"defined insrc/scripts/views/popup/state.ts. A typo here would only be caught at runtime.Consider extracting this as a typed constant or adding a type assertion:
+const DEFAULT_POINT_LIST_STYLE: "docCategories" | "unified" = "docCategories"; + export async function handleExtensionInstalled(): Promise<void> { const donationAllowed = donationReminderAllowed(navigator.userAgent); await setLocal({ themeHeader: true, sentry: false, displayDonationReminder: { active: false, allowedPlattform: donationAllowed, }, - pointListStyle: "docCategories" + pointListStyle: DEFAULT_POINT_LIST_STYLE });src/scripts/views/settings/state.ts (1)
60-62: Add validation when reading pointListStyle from storage.The code uses
String(result['pointListStyle'])which could produce invalid values if storage is corrupted or manually edited.Consider validating the stored value:
if (elements.pointListStyle) { - elements.pointListStyle.value = String(result['pointListStyle']); + const storedValue = result['pointListStyle']; + if (storedValue === "docCategories" || storedValue === "unified") { + elements.pointListStyle.value = storedValue; + } else { + elements.pointListStyle.value = "docCategories"; + } }src/scripts/views/popup/service.ts (4)
11-11: Consider making document_id nullable explicitly.Using
document_id?: numbermeans the property can be missing orundefined, but line 219 and 258 check fornull. Consider usingdocument_id?: number | nullfor clarity.
216-216: Use for...of instead of for...in for arrays.Line 216 uses
for (let i of documents)which iterates over values, but the variable is namedi(suggesting an index). This is confusing.Apply this diff for clarity:
- for (let i of documents) { - const element = i; + for (const element of documents) {
303-316: Simplify createSortedPoints logic.The function has unnecessary
ifchecks since the arrays will always exist (even if empty) after proper typing.-function createSortedPoints(sortedPoints:any,pointsList:HTMLElement) { - if (sortedPoints.blocker) { - createPointList(sortedPoints.blocker, pointsList, false); - } - if (sortedPoints.bad) { - createPointList(sortedPoints.bad, pointsList, false); - } - if (sortedPoints.good) { - createPointList(sortedPoints.good, pointsList, false); - } - if (sortedPoints.neutral) { - createPointList(sortedPoints.neutral, pointsList, true); - } +function createSortedPoints(sortedPoints: FilteredPoints, pointsList: HTMLElement) { + createPointList(sortedPoints.blocker, pointsList, false); + createPointList(sortedPoints.bad, pointsList, false); + createPointList(sortedPoints.good, pointsList, false); + createPointList(sortedPoints.neutral, pointsList, true); }
322-322: Non-null assertion may be unsafe.Line 322 uses
pointsFiltered[i]!which assumes the array access is always valid. While the loop bounds ensure this, combining it with optional chaining forcasesuggests potential undefined values.Consider defensive coding:
- const rawTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const point = pointsFiltered[i]; + if (!point) continue; + const rawTitle = point.case?.localized_title ?? point.title;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/views/settings/icons/FormatListBulleted.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
src/scripts/background/install.ts(1 hunks)src/scripts/views/popup/service.ts(4 hunks)src/scripts/views/popup/state.ts(3 hunks)src/scripts/views/settings/handlers.ts(2 hunks)src/scripts/views/settings/state.ts(3 hunks)src/views/popup.html(2 hunks)src/views/settings/settings.html(1 hunks)src/views/style/popup.css(3 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
src/scripts/views/popup/service.ts
[warning] 192-192: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 192-192: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 233-233: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 252-252: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 269-269: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 329-329: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 233-233: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 252-252: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 269-269: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 329-329: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (11)
src/views/popup.html (2)
102-110: LGTM! Document-based structure aligns with new rendering modes.The changes from
pointListtodocumentListand the addition of adocsWithoutPointssection properly support the new document-categorized display mode.
120-120: Good addition of thedeferattribute.The
deferattribute is appropriate for module scripts and ensures proper loading order.src/views/style/popup.css (3)
4-5: Font path update looks correct.The addition of
../prefix to font URLs suggests proper relative path adjustment for the CSS file location.
273-292: Good refactor from ID to class selectors.Changing from
#pointListto.pointListenables multiple point list containers on the page, which is essential for the document-categorized display mode.
294-312: Document header styles look appropriate.The flex layout and styling for document headers support the new document-categorized view effectively.
src/views/settings/settings.html (1)
78-95: LGTM! Settings UI is well-structured and consistent.The new "Summary Style" control follows the existing pattern and provides clear options for users. The values match the types defined in the state module.
src/scripts/views/settings/handlers.ts (1)
11-11: LGTM! Element retrieval follows established pattern.Consistent with other setting elements in the function.
src/scripts/views/settings/state.ts (2)
16-16: LGTM! Added to storage fetch list.Consistent with other settings.
76-76: LGTM! Element collection updated.Consistent with other form elements.
src/scripts/views/popup/service.ts (2)
63-69: LGTM! Clear branching logic based on display style.The conditional logic properly routes to different rendering functions based on the selected style, with appropriate error logging for unsupported values.
7-12: No changes needed—caseis correctly marked as required.The type definition properly marks
caseas required. Evidence from the codebase shows consistent access tocasewithout optional chaining across lines 289, 292, 295, 298, 325, and 326. Line 322's optional chaining oncase(.case?.localized_title) is an outlier and appears to be unnecessary defensive coding. The property is accessed directly as required everywhere else, confirming the type definition is accurate andcaseis always provided in the API response.
| function populateListDocCategories(allPoints: ServicePoint[], documents: ServiceDocument[]) { | ||
| const documentList = document.getElementById('documentList'); | ||
| // Split points by Document and display them seperatly | ||
| for (let i of documents) { | ||
| const element = i; | ||
|
|
||
| const docPoints = allPoints.filter((point:ServicePoint) => point.document_id === element.id) | ||
| const sortedPoints = filterPoints(docPoints) | ||
|
|
||
| if (sortedPoints.blocker.length + sortedPoints.bad.length + sortedPoints.neutral.length + sortedPoints.good.length > 0) { | ||
| const doc = document.createElement('div'); | ||
| const temp = ` | ||
| <div class=""> | ||
| <div class="documentHeader"> | ||
| <h3 class="documentTitle">${element.name}</h3> | ||
| <a href="${element.url}" target="_blank">Read Original></a> | ||
| </div> | ||
| <div id="pointList_${element.id}" class="pointList"> | ||
| <a style="display: none">...</a> | ||
| </div> | ||
| </div>`; | ||
| doc.innerHTML = temp.trim(); | ||
| documentList!.appendChild(doc.firstChild!); | ||
|
|
||
| const pointsList = document.getElementById(`pointList_${element.id}`)! | ||
|
|
||
| createSortetPoints(sortedPoints,pointsList) | ||
| } else { //documents without points | ||
| const docsWithoutPointsWraper = document.getElementById('docsWithoutPointsWraper') | ||
| const docsWithoutPoints = document.getElementById('docsWithoutPoints') | ||
|
|
||
| if (docsWithoutPoints?.style.display === "none") { | ||
| docsWithoutPoints.style.display = "block" | ||
| } | ||
| const doc = document.createElement('div'); | ||
| const temp = ` | ||
| <div class="documentHeader"> | ||
| <h3 class="documentTitle">${element.name}</h3> | ||
| <a href="${element.url}" target="_blank">Read Original></a> | ||
| </div>`; | ||
| doc.innerHTML = temp.trim(); | ||
| docsWithoutPointsWraper!.appendChild(doc.firstChild!); | ||
| } | ||
| } | ||
| //display points not liked to a document | ||
| const noDocPoints = allPoints.filter((point: ServicePoint) => point.document_id === null) | ||
| if (noDocPoints.length > 0) { | ||
| const doc = document.createElement('div'); | ||
| const temp = ` | ||
| <div class=""> | ||
| <div class="documentHeader"> | ||
| <h3 class="documentTitle">Points not linked to a Document</h3> | ||
| </div> | ||
| `.trim(); | ||
| if (wrapper.firstChild) { | ||
| container.appendChild(wrapper.firstChild as HTMLElement); | ||
| <div id="pointList_unlinkedPoints" class="pointList"> | ||
| <a style="display: none">...</a> | ||
| </div> | ||
| </div>`; | ||
| doc.innerHTML = temp.trim(); | ||
| documentList!.appendChild(doc.firstChild!); | ||
| const sortedPoints = filterPoints(noDocPoints) | ||
| const pointsList = document.getElementById(`pointList_unlinkedPoints`)! | ||
| createSortetPoints(sortedPoints,pointsList) | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Multiple XSS vulnerabilities in populateListDocCategories.
Lines 227-228 and 250-251 insert unsanitized data from the API (element.name and element.url) directly into HTML via innerHTML. This is a critical XSS vulnerability if the API response is compromised or contains malicious content.
The static analysis correctly flags this as CWE-79. You must sanitize or escape the content:
+function escapeHtml(text: string): string {
+ const div = document.createElement('div');
+ div.textContent = text;
+ return div.innerHTML;
+}
+
+function escapeUrl(url: string): string {
+ const a = document.createElement('a');
+ a.href = url;
+ return a.href; // Browser normalizes and validates
+}
function populateListDocCategories(allPoints: ServicePoint[], documents: ServiceDocument[]) {
// ... existing code ...
if (sortedPoints.blocker.length + sortedPoints.bad.length + sortedPoints.neutral.length + sortedPoints.good.length > 0) {
const doc = document.createElement('div');
const temp = `
<div class="">
<div class="documentHeader">
- <h3 class="documentTitle">${element.name}</h3>
- <a href="${element.url}" target="_blank">Read Original></a>
+ <h3 class="documentTitle">${escapeHtml(element.name)}</h3>
+ <a href="${escapeUrl(element.url)}" target="_blank">Read Original></a>
</div>Apply the same escaping to lines 250-251 in the documents-without-points section.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 233-233: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 252-252: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 269-269: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 233-233: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 252-252: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 269-269: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
In src/scripts/views/popup/service.ts around lines 213-277, the code injects
unsanitized API values (element.name and element.url) directly into innerHTML
causing XSS; fix by building DOM nodes without innerHTML: create elements (div,
h3, a) with document.createElement, set the title text via textContent (or safe
escaping) and assign the link via element.setAttribute('href', safeUrl) after
validating/normalizing the URL (allow only http/https, or prepend https if
missing), add rel="noopener noreferrer" and keep target="_blank", then append
those nodes; apply the exact same change to the documents-without-points block
(lines ~250-251) so both sections use textContent and safe href assignment
instead of string interpolation into innerHTML.
| function createPointList(pointsFiltered: ServicePoint[], pointsList: HTMLElement, last: boolean) { | ||
| let added = 0; | ||
| for (let i = 0; i < pointsFiltered.length; i++) { | ||
| const point = document.createElement('div'); | ||
| const pointTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; | ||
|
|
||
| let temp = ` | ||
| <div class="point ${pointsFiltered[i]!.case.classification}"> | ||
| <img src="icons/${pointsFiltered[i]!.case.classification}.svg"> | ||
| <p>${pointTitle}</p> | ||
| ${renderCuratorTag(pointsFiltered[i]!.status)} | ||
| </div>`; | ||
| point.innerHTML = temp.trim(); | ||
| pointsList.appendChild(point.firstChild!); | ||
| added++; | ||
| if (i !== pointsFiltered.length - 1) { | ||
| const divider = document.createElement('hr'); | ||
| container.appendChild(divider); | ||
| pointsList.appendChild(divider); | ||
| } | ||
| }); | ||
|
|
||
| if (added > 0 && !isLastGroup) { | ||
| } | ||
| if (added !== 0 && !last) { | ||
| const divider = document.createElement('hr'); | ||
| divider.classList.add('group'); | ||
| container.appendChild(divider); | ||
| pointsList.appendChild(divider); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: XSS vulnerability in createPointList.
Line 327 directly inserts pointTitle (derived from API data) into HTML via innerHTML on line 330. This is a critical XSS vulnerability.
Even though pointTitle uses a fallback to localized_title or title, both come from the API and must be sanitized:
function createPointList(pointsFiltered: ServicePoint[], pointsList: HTMLElement, last: boolean) {
let added = 0;
for (let i = 0; i < pointsFiltered.length; i++) {
const point = document.createElement('div');
- const pointTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title;
+ const rawTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title;
+ const pointTitle = escapeHtml(rawTitle);
let temp = `
<div class="point ${pointsFiltered[i]!.case.classification}">Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 329-329: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 329-329: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
|
|
||
| const darkmode = Boolean(result['darkmode']); | ||
| const storedCuratorMode = Boolean(result['curatorMode']); | ||
| pointListStyle = result['pointListStyle'] as "docCategories" | "unified" |
There was a problem hiding this comment.
Add validation for type-asserted storage value.
The type assertion as "docCategories" | "unified" is unsafe because storage could contain any value. This could lead to runtime errors if the stored value is corrupted.
Apply this diff:
- pointListStyle = result['pointListStyle'] as "docCategories" | "unified"
+ const storedStyle = result['pointListStyle'];
+ pointListStyle = (storedStyle === "docCategories" || storedStyle === "unified")
+ ? storedStyle
+ : "docCategories";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pointListStyle = result['pointListStyle'] as "docCategories" | "unified" | |
| const storedStyle = result['pointListStyle']; | |
| pointListStyle = (storedStyle === "docCategories" || storedStyle === "unified") | |
| ? storedStyle | |
| : "docCategories"; |
🤖 Prompt for AI Agents
In src/scripts/views/popup/state.ts around line 45, the code unsafely
type-asserts a storage value to "docCategories" | "unified"; validate the value
at runtime instead of using a direct type assertion. Read the raw value, check
it against an allowed set (e.g., ["docCategories","unified"]), and if it matches
assign it, otherwise assign a safe default (or remove the key) and optionally
log/debug the unexpected value; ensure the variable ends up typed correctly
without relying on an unchecked "as" cast.
| if (pointListStyleSelect) { | ||
| pointListStyleSelect.addEventListener('change', () => { | ||
| const normalized = pointListStyleSelect.value ?? 'docCategories'; | ||
| if (pointListStyleSelect.value !== normalized) { | ||
| pointListStyleSelect.value = normalized; | ||
| } | ||
| void setLocal({ pointListStyle: normalized }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve type safety and validation for pointListStyle.
The current implementation has two issues:
- The normalization logic (
pointListStyleSelect.value ?? 'docCategories') won't catch invalid values already in the select element, unlike the language handler which usesnormalizeLanguage(). - The
normalizedvariable and the persisted value lack type safety.
Consider adding proper validation:
+function normalizePointListStyle(value: string): "docCategories" | "unified" {
+ if (value === "docCategories" || value === "unified") {
+ return value;
+ }
+ return "docCategories";
+}
+
if (pointListStyleSelect) {
pointListStyleSelect.addEventListener('change', () => {
- const normalized = pointListStyleSelect.value ?? 'docCategories';
+ const normalized = normalizePointListStyle(pointListStyleSelect.value);
if (pointListStyleSelect.value !== normalized) {
pointListStyleSelect.value = normalized;
}
void setLocal({ pointListStyle: normalized });
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pointListStyleSelect) { | |
| pointListStyleSelect.addEventListener('change', () => { | |
| const normalized = pointListStyleSelect.value ?? 'docCategories'; | |
| if (pointListStyleSelect.value !== normalized) { | |
| pointListStyleSelect.value = normalized; | |
| } | |
| void setLocal({ pointListStyle: normalized }); | |
| }); | |
| } | |
| function normalizePointListStyle(value: string): "docCategories" | "unified" { | |
| if (value === "docCategories" || value === "unified") { | |
| return value; | |
| } | |
| return "docCategories"; | |
| } | |
| if (pointListStyleSelect) { | |
| pointListStyleSelect.addEventListener('change', () => { | |
| const normalized = normalizePointListStyle(pointListStyleSelect.value); | |
| if (pointListStyleSelect.value !== normalized) { | |
| pointListStyleSelect.value = normalized; | |
| } | |
| void setLocal({ pointListStyle: normalized }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/scripts/views/settings/handlers.ts around lines 58 to 66, the change
handler uses pointListStyleSelect.value ?? 'docCategories' which neither
validates against allowed values nor preserves type safety; replace this with a
validation/normalization step that checks pointListStyleSelect.value against the
known allowed pointListStyle values (e.g., an enum or const array), map invalid
or missing values to the default 'docCategories', and narrow the type with a
type guard or typed cast before calling setLocal; ensure the persisted value is
of the declared pointListStyle type and update the handler to use that validated
value only.
|
@shadowwwind want me to take over and finish the implementation? |
i am not entirely happy with the table of contents, but i dont see a better solutions for service like brave iwth so many docs |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/scripts/views/popup/state.ts (2)
36-38: Fix inconsistent naming convention.The getter is named
getpointListStyle(lowercase 'p') while other getters use camelCase (e.g.,getApiUrl,getLanguage). This inconsistency could lead to confusion.Apply this diff:
-export function getpointListStyle() { - return pointListStyle -} +export function getPointListStyle() { + return pointListStyle; +}Note: This will require updating the import in
src/scripts/views/popup/service.ts(line 1).
45-45: Add validation for type-asserted storage value.The type assertion
as "docCategories" | "unified"is unsafe because storage could contain any value. This could lead to runtime errors if the stored value is corrupted.Apply this diff:
- pointListStyle = result['pointListStyle'] as "docCategories" | "unified" + const storedStyle = result['pointListStyle']; + pointListStyle = (storedStyle === "docCategories" || storedStyle === "unified") + ? storedStyle + : DEFAULT_LIST_STYLE;src/scripts/views/popup/service.ts (4)
1-1: Update import to use consistent naming.The import uses
getpointListStylewhich doesn't follow the camelCase convention. This should be updated after the function name is corrected instate.ts.
222-282: Critical: Multiple XSS vulnerabilities in populateListDocCategories.Lines 253-254 and 276-277 insert unsanitized data from the API (
element.nameandelement.url) directly into HTML viainnerHTML. This is a critical XSS vulnerability if the API response is compromised or contains malicious content.You must sanitize or escape the content:
+function escapeHtml(text: string): string { + const div = document.createElement('div'); + div.textContent = text; + return div.innerHTML; +} + +function sanitizeUrl(url: string): string { + try { + const parsed = new URL(url); + if (parsed.protocol === 'http:' || parsed.protocol === 'https:') { + return parsed.href; + } + } catch { + // Invalid URL + } + return ''; +} function populateListDocCategories(allPoints: ServicePoint[], documents: ServiceDocument[]) { // ... existing code ... if (sortedPoints.blocker.length + sortedPoints.bad.length + sortedPoints.neutral.length + sortedPoints.good.length > 0) { const doc = document.createElement('div'); const temp = ` <div class=""> <div class="documentHeader"> - <h3 id="documents_${element.id}" class="documentTitle" >${element.name}</h3> - <a href="${element.url}" target="_blank">Read Original></a> + <h3 id="documents_${element.id}" class="documentTitle">${escapeHtml(element.name)}</h3> + <a href="${sanitizeUrl(element.url)}" target="_blank" rel="noopener noreferrer">Read Original></a> </div>Apply the same escaping to lines 276-277 in the documents-without-points section.
334-347: Fix typo in function name.
createSortetPointsshould becreateSortedPoints.Apply this diff:
-function createSortetPoints(sortedPoints:FilteredPoints,pointsList:HTMLElement) { +function createSortedPoints(sortedPoints: FilteredPoints, pointsList: HTMLElement) {Update all call sites (lines 265 and 300) as well.
349-374: Critical: XSS vulnerability in createPointList.Line 353 constructs
pointTitlefrom API data (pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title), and line 361 directly inserts it into HTML viainnerHTML. This is a critical XSS vulnerability.Both title values come from the API and must be sanitized:
function createPointList(pointsFiltered: ServicePoint[], pointsList: HTMLElement, last: boolean) { let added = 0; for (let i = 0; i < pointsFiltered.length; i++) { const point = document.createElement('div'); - const pointTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const rawTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const pointTitle = escapeHtml(rawTitle); let temp = ` <div class="point ${pointsFiltered[i]!.case.classification}">Use the
escapeHtmlhelper function suggested in the previous comment.
🧹 Nitpick comments (1)
src/scripts/constants.ts (1)
17-17: Optional: Simplify the type annotation.The
as constassertion is redundant when you've already explicitly typed the variable as"docCategories" | "unified". You can simplify this to just the explicit type.Apply this diff if you prefer cleaner syntax:
-export const DEFAULT_LIST_STYLE :"docCategories" | "unified" = "docCategories" as const; +export const DEFAULT_LIST_STYLE: "docCategories" | "unified" = "docCategories";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/scripts/background/install.ts(2 hunks)src/scripts/constants.ts(1 hunks)src/scripts/views/popup/service.ts(5 hunks)src/scripts/views/popup/state.ts(4 hunks)src/views/popup.html(2 hunks)src/views/style/popup.css(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/background/install.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/scripts/views/popup/service.ts (1)
src/scripts/views/popup/state.ts (2)
getPointListStyle(36-38)isCuratorMode(20-22)
src/scripts/views/popup/state.ts (2)
src/scripts/lib/language.ts (1)
SupportedLanguage(3-3)src/scripts/lib/chromeStorage.ts (1)
getLocal(1-9)
🪛 ast-grep (0.40.0)
src/scripts/views/popup/service.ts
[warning] 201-201: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 201-201: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 259-259: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 278-278: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 295-295: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 360-360: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 259-259: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 278-278: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 295-295: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 360-360: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (13)
src/views/style/popup.css (4)
4-5: LGTM! Font paths correctly updated.The relative paths now correctly point to
../fonts/which aligns with the updated asset layout.
152-160: LGTM! Migration to class-based selectors.Changing from
#titleto.titleclass selector aligns with the new header structure and improves reusability.
273-292: LGTM! Consistent class-based approach.Migrating
.pointListand related selectors from IDs to classes supports the new document-based rendering where multiple point lists can exist.
294-348: LGTM! Well-structured document layout CSS.The new
.documentHeader,.documentIndex, and.indexSummaryWraperstyles properly support the document-categorized view with clear visual hierarchy and color-coded status indicators.src/views/popup.html (3)
101-104: LGTM! New document-based structure.The new
documentIndexanddocumentListelements properly support the document-categorized rendering mode introduced in the popup service.
105-110: LGTM! Documents without points section.The dedicated section for documents without points provides a clear UX for incomplete document coverage.
120-120: LGTM! Script loading optimization.The
deferattribute ensures the script executes after the DOM is parsed, improving page load performance.src/scripts/views/popup/service.ts (6)
7-11: LGTM! ServicePoint.case is now required.Making the
caseproperty required (removing the?) improves type safety and aligns with the actual usage throughout the code wherepoint.case.classificationis accessed without optional chaining.
14-18: LGTM! Well-defined ServiceDocument interface.The new interface clearly defines the structure for documents with appropriate types for id, name, and url.
34-39: LGTM! FilteredPoints interface replaces unsafeany.This interface provides proper type safety for the filtered points structure used throughout the rendering logic.
70-78: LGTM! Clean branching logic for rendering modes.The conditional rendering based on
pointListStylecleanly separates the two display modes with appropriate error logging for unsupported values.
192-219: LGTM! Unified list rendering path.The
populateListUnifiedfunction provides a clean implementation of the single consolidated view. Note that XSS concerns in the sharedcreatePointListhelper (line 353) should be addressed separately.
375-422: LGTM! Document index creation with summary.The
addDocumentToIndexfunction properly creates index entries with per-classification summaries. The nested helper functions are well-organized. Note thatinnerText(line 381) is safe for setting text content, unlikeinnerHTML.
| let curatorMode = false; | ||
| let apiUrl = DEFAULT_API_URL; | ||
| let language: SupportedLanguage = 'en'; | ||
| let pointListStyle:"docCategories" | "unified" = "unified" |
There was a problem hiding this comment.
Inconsistent default value with constants.
The variable is initialized to "unified" but DEFAULT_LIST_STYLE in src/scripts/constants.ts is set to "docCategories". This inconsistency could lead to confusion about the intended default behavior.
Apply this diff to align with the constant:
+import { DEFAULT_API_URL, DEFAULT_LIST_STYLE } from '../../constants';
-let pointListStyle:"docCategories" | "unified" = "unified"
+let pointListStyle: "docCategories" | "unified" = DEFAULT_LIST_STYLE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let pointListStyle:"docCategories" | "unified" = "unified" | |
| import { DEFAULT_API_URL, DEFAULT_LIST_STYLE } from '../../constants'; | |
| let pointListStyle: "docCategories" | "unified" = DEFAULT_LIST_STYLE; |
🤖 Prompt for AI Agents
In src/scripts/views/popup/state.ts around line 11, the variable pointListStyle
is initialized to the literal "unified" which contradicts DEFAULT_LIST_STYLE in
src/scripts/constants.ts (set to "docCategories"); update the file to import
DEFAULT_LIST_STYLE from src/scripts/constants.ts (or reference it from the
existing import) and initialize pointListStyle to DEFAULT_LIST_STYLE so the
default value is consistent with the project constant.
|
what do we want the default to be? |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/scripts/views/popup/service.ts (3)
221-302: CRITICAL: XSS vulnerabilities still not fixed.Lines 252-253 and 275-276 inject unsanitized API data (
element.nameandelement.url) directly into HTML viainnerHTML. This is a critical XSS vulnerability that was flagged in previous reviews but remains unaddressed.Replace innerHTML with safe DOM construction:
+function escapeHtml(text: string): string { + const div = document.createElement('div'); + div.textContent = text; + return div.innerHTML; +} + function populateListDocCategories(allPoints: ServicePoint[], documents: ServiceDocument[]) { // ... existing code ... if (sortedPoints.blocker.length + sortedPoints.bad.length + sortedPoints.neutral.length + sortedPoints.good.length > 0) { - const doc = document.createElement('div'); - const temp = ` - <div class=""> - <div class="documentHeader"> - <h3 id="documents_${element.id}" class="documentTitle" >${element.name}</h3> - <a href="${element.url}" target="_blank">Read Original</a> - </div> - <div id="pointList_${element.id}" class="pointList"> - <a style="display: none">...</a> - </div> - </div>`; - doc.innerHTML = temp.trim(); - documentList!.appendChild(doc.firstChild!); + const wrapper = document.createElement('div'); + const header = document.createElement('div'); + header.className = 'documentHeader'; + + const title = document.createElement('h3'); + title.id = `documents_${element.id}`; + title.className = 'documentTitle'; + title.textContent = element.name; + + const link = document.createElement('a'); + link.href = element.url; + link.target = '_blank'; + link.rel = 'noopener noreferrer'; + link.textContent = 'Read Original'; + + header.appendChild(title); + header.appendChild(link); + + const pointsDiv = document.createElement('div'); + pointsDiv.id = `pointList_${element.id}`; + pointsDiv.className = 'pointList'; + + wrapper.appendChild(header); + wrapper.appendChild(pointsDiv); + documentList!.appendChild(wrapper);Apply the same fix to lines 272-279 in the documents-without-points section.
333-346: Fix typo and improve conditional checks.
- Function name has typo:
createSortetPointsshould becreateSortedPoints(flagged in previous review).- Lines 334, 337, 340, 343 check array truthiness, but arrays always exist per
FilteredPoints. Check.length > 0instead for clarity.-function createSortedPoints(sortedPoints:FilteredPoints,pointsList:HTMLElement) { - if (sortedPoints.blocker) { +function createSortedPoints(sortedPoints: FilteredPoints, pointsList: HTMLElement) { + if (sortedPoints.blocker.length > 0) { createPointList(sortedPoints.blocker, pointsList, false); } - if (sortedPoints.bad) { + if (sortedPoints.bad.length > 0) { createPointList(sortedPoints.bad, pointsList, false); } - if (sortedPoints.good) { + if (sortedPoints.good.length > 0) { createPointList(sortedPoints.good, pointsList, false); } - if (sortedPoints.neutral) { + if (sortedPoints.neutral.length > 0) { createPointList(sortedPoints.neutral, pointsList, true); } }
348-373: CRITICAL: XSS vulnerability in point title rendering.Lines 352, 357, and 360 inject unsanitized API data (
pointTitle) directly into HTML viainnerHTML. This is a critical XSS vulnerability flagged in previous reviews but still unaddressed.Replace innerHTML with safe DOM construction:
function createPointList(pointsFiltered: ServicePoint[], pointsList: HTMLElement, last: boolean) { let added = 0; for (let i = 0; i < pointsFiltered.length; i++) { - const point = document.createElement('div'); const pointTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const classification = pointsFiltered[i]!.case.classification; + + const pointDiv = document.createElement('div'); + pointDiv.className = `point ${classification}`; + + const img = document.createElement('img'); + img.src = `icons/${classification}.svg`; + + const p = document.createElement('p'); + p.textContent = pointTitle; + + pointDiv.appendChild(img); + pointDiv.appendChild(p); + + const curatorTagHtml = renderCuratorTag(pointsFiltered[i]!.status); + if (curatorTagHtml) { + const tagDiv = document.createElement('div'); + tagDiv.innerHTML = curatorTagHtml; + pointDiv.appendChild(tagDiv.firstChild!); + } - let temp = ` - <div class="point ${pointsFiltered[i]!.case.classification}"> - <img src="icons/${pointsFiltered[i]!.case.classification}.svg"> - <p>${pointTitle}</p> - ${renderCuratorTag(pointsFiltered[i]!.status)} - </div>`; - point.innerHTML = temp.trim(); - pointsList.appendChild(point.firstChild!); + pointsList.appendChild(pointDiv); added++; if (i !== pointsFiltered.length - 1) { const divider = document.createElement('hr'); pointsList.appendChild(divider); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scripts/views/popup/service.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/scripts/views/popup/service.ts (1)
src/scripts/views/popup/state.ts (2)
getPointListStyle(36-38)isCuratorMode(20-22)
🪛 ast-grep (0.40.0)
src/scripts/views/popup/service.ts
[warning] 200-200: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 200-200: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 258-258: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 277-277: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 294-294: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 359-359: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 258-258: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 277-277: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 294-294: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 359-359: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (3)
src/scripts/views/popup/service.ts (3)
4-39: LGTM! Well-structured type definitions.The interface updates properly support the new document-categorized view:
- Making
caserequired eliminates potential runtime errorsdocument_idcorrectly enables point-to-document linkingFilteredPointsreplaces the previously flaggedanytype with proper typing
192-218: LGTM! Safe innerHTML usage with static content.The unified list rendering is well-structured. Line 200's innerHTML usage is safe because it contains only static HTML with no user-supplied data.
303-331: LGTM! Clean filtering logic.The function properly filters by status and partitions points by classification using the well-typed
FilteredPointsinterface.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/scripts/views/popup/service.ts (4)
389-391: Fix typos and use classList.add().Multiple typos and unconventional classList assignment:
- function documentIndexPointSummary(points:FilteredPoints) { //adds the number of points of each classification as a summary below the document index entry - const pointsSummanry = document.createElement("div") - pointsSummanry.classList = "indexSummaryWraper" + function documentIndexPointSummary(points: FilteredPoints) { //adds the number of points of each classification as a summary below the document index entry + const pointsSummary = document.createElement("div") + pointsSummary.classList.add("indexSummaryWrapper")Update all references to
pointsSummanry→pointsSummaryand fix "Wraper" → "Wrapper" in CSS as well.
248-260: Critical: XSS vulnerability persists in document rendering.Lines 252-253 interpolate
element.nameandelement.urldirectly into innerHTML. This was flagged in a previous review but remains unaddressed. Malicious API data could execute arbitrary JavaScript.Add escaping functions and use them:
+function escapeHtml(text: string): string { + const div = document.createElement('div'); + div.textContent = text; + return div.innerHTML; +} // In populateListDocCategories: const temp = ` <div class=""> <div class="documentHeader"> - <h3 id="documents_${element.id}" class="documentTitle" >${element.name}</h3> - <a href="${element.url}" target="_blank">Read Original</a> + <h3 id="documents_${element.id}" class="documentTitle" >${escapeHtml(element.name)}</h3> + <a href="${encodeURI(element.url)}" target="_blank" rel="noopener noreferrer">Read Original</a>Apply the same fix to lines 275-276.
282-284: Filter may miss points withnulldocument_id.The filter checks only for
undefined, but API data might returnnull. Use loose equality to catch both:- const noDocPoints = allPoints.filter((point: ServicePoint) => point.document_id === undefined) + const noDocPoints = allPoints.filter((point: ServicePoint) => point.document_id == null)
348-373: Critical: XSS vulnerability in point title rendering.Line 357 interpolates
pointTitle(from API data) directly into innerHTML without sanitization. This was flagged previously but remains unaddressed.function createPointList(pointsFiltered: ServicePoint[], pointsList: HTMLElement, last: boolean) { let added = 0; for (let i = 0; i < pointsFiltered.length; i++) { const point = document.createElement('div'); - const pointTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const rawTitle = pointsFiltered[i]!.case?.localized_title ?? pointsFiltered[i]!.title; + const pointTitle = escapeHtml(rawTitle); let temp = ` <div class="point ${pointsFiltered[i]!.case.classification}">Where
escapeHtmlis the helper suggested earlier.
🧹 Nitpick comments (6)
src/scripts/views/popup/service.ts (6)
70-78: Missing fallback for unsupported pointListStyle.When
pointListStyleis neither "docCategories" nor "unified", the code only logs an error but leaves the user with no points displayed. Consider falling back to the unified view:if (pointListStyle === "docCategories") { populateListDocCategories(data.points, data.documents); } else if (pointListStyle === "unified") { populateListUnified(data.points) } else { - console.error("Unsupported pointListStyle", pointListStyle); + console.error("Unsupported pointListStyle", pointListStyle); + populateListUnified(data.points); // Fallback to unified view }
192-218: Add null checks for DOM elements.Lines 202 uses non-null assertions (
!) that will throw if the elements are missing. Consider adding guards:- doc.innerHTML = temp.trim(); - documentList!.appendChild(doc.firstChild!); + doc.innerHTML = temp.trim(); + if (documentList && doc.firstChild) { + documentList.appendChild(doc.firstChild); + }Note: The static analysis XSS warning on line 200 is a false positive here since the template contains only static HTML with no user input.
233-234: Use proper style property assignment.Line 234 assigns to
styledirectly as a string, which is unconventional and may cause TypeScript issues:- indexElement.style = "display:block" + indexElement.style.display = "block";
303-331: Consider single-pass filtering for efficiency.The function makes five passes over the array (one for status filtering, four for classification). A single pass would be more efficient, especially for services with many points:
function filterPoints(points: ServicePoint[]): FilteredPoints { - if (isCuratorMode()) { - points = points.filter( - (point) => - point.status === 'approved' || point.status === 'pending' - ); - } else { - points = points.filter((point) => point.status === 'approved'); - } - let filteredPoints: FilteredPoints = { + const filteredPoints: FilteredPoints = { blocker: [], bad: [], good: [], neutral: [] - } - filteredPoints.blocker = points.filter( - (point) => point.case.classification === 'blocker' - ); - // ... similar for bad, good, neutral + }; + const curatorMode = isCuratorMode(); + for (const point of points) { + const validStatus = curatorMode + ? (point.status === 'approved' || point.status === 'pending') + : point.status === 'approved'; + if (!validStatus) continue; + + const classification = point.case.classification; + if (classification && classification in filteredPoints) { + filteredPoints[classification as keyof FilteredPoints].push(point); + } + } return filteredPoints; }
333-346: Truthy checks on arrays are always true.The conditions like
if (sortedPoints.blocker)always pass since empty arrays are truthy. The code works becausecreatePointListhandles empty arrays, but the intent would be clearer with length checks:-function createSortedPoints(sortedPoints:FilteredPoints,pointsList:HTMLElement) { - if (sortedPoints.blocker) { - createPointList(sortedPoints.blocker, pointsList, false); - } - // ... similar for others +function createSortedPoints(sortedPoints: FilteredPoints, pointsList: HTMLElement) { + createPointList(sortedPoints.blocker, pointsList, false); + createPointList(sortedPoints.bad, pointsList, false); + createPointList(sortedPoints.good, pointsList, false); + createPointList(sortedPoints.neutral, pointsList, true); }Since
createPointListhandles empty arrays gracefully, the conditionals add no value.
423-426: Remove excessive blank lines.Four consecutive blank lines between
addDocumentToIndexandrenderCuratorTagcan be reduced to one or two for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scripts/views/popup/service.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/scripts/views/popup/service.ts (1)
src/scripts/views/popup/state.ts (2)
getPointListStyle(36-38)isCuratorMode(20-22)
🪛 ast-grep (0.40.0)
src/scripts/views/popup/service.ts
[warning] 200-200: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 200-200: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 258-258: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 277-277: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 294-294: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 359-359: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 258-258: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 277-277: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 294-294: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: doc.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 359-359: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: point.innerHTML = temp.trim()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (2)
src/scripts/views/popup/service.ts (2)
1-39: LGTM! Interfaces are well-defined.The addition of
FilteredPoints,ServiceDocument, and thedocument_idfield onServicePointproperly types the new document-based rendering flow.
380-381: Validate URL protocol to prevent javascript: injection.Line 381 sets
hrefdirectly from API data. A malicious API could returnjavascript:alert(1)as the URL. Validate the protocol:item.innerText = serviceDocument.name - item.href = `#documents_${serviceDocument.id}` + item.href = `#documents_${serviceDocument.id}` // Internal anchor, safeWait - this is an internal anchor link, not
serviceDocument.url. This is actually safe. However, consider addingrel="noopener"to external links in lines 253 and 276 whereelement.urlis used.
Like it is right now
Make a popup or navigate to a different page/make it collapsable? That way it wont take too much room. Needless to say, I think I'll take a look and do some UI work before we merge this. Let me know when your implementation is finished @shadowwwind |
2 participants
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.