-
Notifications
You must be signed in to change notification settings - Fork 9
feat: Sds3 #386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: pre-staging
Are you sure you want to change the base?
feat: Sds3 #386
Conversation
|
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
| entitiesList.innerHTML = newEntities | ||
| .map( | ||
| (entity) => ` | ||
| <div class="swal-file-row px-2"> | ||
| <span class="swal-file-text">${entity}</span> | ||
| <button class="delete-button btn btn-sm btn-outline-danger" data-entity-name="${entity}">Delete</button> | ||
| </div> | ||
| ` | ||
| ) | ||
| .join(""); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this vulnerability, we must ensure that any user-controlled data interpolated into HTML is properly escaped so that it is interpreted as text, not as markup. The best way to do this is to escape HTML meta-characters (<, >, &, ", ', and `) in the entity value before inserting it into the template string. This can be done by creating a utility function (e.g., escapeHtml) that replaces these characters with their corresponding HTML entities. We should use this function when interpolating entity into the HTML in renderEntitiesInSwal (lines 99 and 100).
What to change:
- Add an
escapeHtmlfunction to the file. - In
renderEntitiesInSwal, useescapeHtml(entity)instead ofentitywhen interpolating into the template string for both the text span and thedata-entity-nameattribute.
What is needed:
- A new
escapeHtmlfunction definition. - Update the template string in
renderEntitiesInSwalto useescapeHtml(entity).
-
Copy modified lines R13-R23 -
Copy modified lines R110-R111
| @@ -12,2 +12,13 @@ | ||
| } from "../../../stores/slices/datasetEntityStructureSlice"; | ||
| // Utility function to escape HTML special characters | ||
| function escapeHtml(str) { | ||
| return String(str) | ||
| .replace(/&/g, "&") | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/"/g, """) | ||
| .replace(/'/g, "'") | ||
| .replace(/`/g, "`"); | ||
| } | ||
|
|
||
| export const guidedOpenEntityAdditionSwal = async ({ entityType, subjectId, sampleId }) => { | ||
| @@ -98,4 +109,4 @@ | ||
| <div class="swal-file-row px-2"> | ||
| <span class="swal-file-text">${entity}</span> | ||
| <button class="delete-button btn btn-sm btn-outline-danger" data-entity-name="${entity}">Delete</button> | ||
| <span class="swal-file-text">${escapeHtml(entity)}</span> | ||
| <button class="delete-button btn btn-sm btn-outline-danger" data-entity-name="${escapeHtml(entity)}">Delete</button> | ||
| </div> |
| $(".bf-dataset-span").html(bfDataset); | ||
| $("#current-ps-dataset").text(bfDataset); | ||
| $("#current-ps-dataset-generate").text(bfDataset); | ||
| $(".ps-dataset-span").html(bfDataset); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this issue, avoid using .html(bfDataset) when inserting user-controllable or DOM-derived string data into the page, as it will be interpreted as HTML and expose the app to XSS. Instead, use .text(bfDataset) to safely insert the value as plain text, ensuring that any HTML meta-characters in bfDataset are properly escaped. Specifically, in src/renderer/src/scripts/globals.js, replace $(".ps-dataset-span").html(bfDataset); on line 1673 with $(".ps-dataset-span").text(bfDataset);. No new imports or major code changes are needed; simply update the jQuery method used to render the dataset name.
-
Copy modified line R1673
| @@ -1670,7 +1670,7 @@ | ||
| } | ||
| $("#current-ps-dataset").text(bfDataset); | ||
| $("#current-ps-dataset-generate").text(bfDataset); | ||
| $(".ps-dataset-span").html(bfDataset); | ||
| $(".ps-dataset-span").text(bfDataset); | ||
| confirm_click_function(); | ||
| // $("#button-refresh-publishing-status").removeClass("hidden"); | ||
| $("#button-refresh-publishing-status").addClass("fa-spin"); |
| @@ -406,9 +407,9 @@ | |||
|
|
|||
| window.log.info("Dataset rename success"); | |||
| window.defaultBfDataset = renamedDatasetName; | |||
| $(".bf-dataset-span").html(renamedDatasetName); | |||
| $(".ps-dataset-span").html(renamedDatasetName); | |||
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this issue, we should avoid inserting untrusted user input as HTML. Instead of using .html(renamedDatasetName), which interprets the string as HTML, we should use .text(renamedDatasetName), which safely inserts the string as plain text, escaping any HTML meta-characters. This change should be made on line 410 in src/renderer/src/scripts/manage-dataset/manage-dataset.js. No additional imports or dependencies are required, as jQuery's .text() method is already available.
-
Copy modified line R410
| @@ -409,3 +409,3 @@ | ||
| window.defaultBfDataset = renamedDatasetName; | ||
| $(".ps-dataset-span").html(renamedDatasetName); | ||
| $(".ps-dataset-span").text(renamedDatasetName); | ||
| window.refreshDatasetList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request #386 has too many files changed.
We can only review pull requests with up to 300 changed files, and this pull request has 1007.
|
|
||
| await window.importLocalDataset(folderPath); | ||
| await window.importLocalDataset(folderPath); | ||
| } |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix this vulnerability, we need to prevent untrusted text from being interpreted as HTML. The specific sink is the assignment to innerHTML, which interprets its value as HTML. The remedy is to use textContent instead, which sets the element's text without parsing as HTML.
Detailed steps:
- Locate the assignment to
innerHTML(document.getElementById("org-dataset-folder-path").innerHTML = folderPath;on line 650 ofsrc/renderer/src/scripts/organize-dataset/curate-functions.js). - Replace
innerHTMLwithtextContentso that any value infolderPath, even if it contains HTML or scripts, will be rendered as plain text. - No additional libraries or imports are necessary for this change.
- No other code changes are required, and functionality (displaying the folder path) remains unchanged and safe.
-
Copy modified line R650
| @@ -647,7 +647,7 @@ | ||
| } | ||
|
|
||
| if (moveForward) { | ||
| document.getElementById("org-dataset-folder-path").innerHTML = folderPath; | ||
| document.getElementById("org-dataset-folder-path").textContent = folderPath; | ||
| document.getElementById("nextBtn").disabled = false; | ||
| } | ||
| }; |
| }, | ||
| }; | ||
|
|
||
| // Set the default Pennsieve account and dataset | ||
| window.sodaJSONObj["bf-account-selected"]["account-name"] = window.defaultBfAccount; | ||
| window.sodaJSONObj["bf-dataset-selected"]["dataset-name"] = selectedDataset; | ||
| window.sodaJSONObj["ps-account-selected"]["account-name"] = window.defaultBfAccount; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this problem, we need to ensure that all backslashes in the string are replaced with forward slashes, not just the first one. The best way to do this in JavaScript is to use a regular expression with the global flag: replace(/\\/g, "/"). This will replace every backslash in the string. The change should be made on line 1541 of src/renderer/src/scripts/others/tab-effects.js. No new imports or dependencies are needed, as this is standard JavaScript functionality.
-
Copy modified line R1541
| @@ -1538,7 +1538,7 @@ | ||
| } | ||
| } | ||
| } else if (extension == ".csv") { | ||
| let temp_current_file_path = current_file_path.replace("\\", "/"); | ||
| let temp_current_file_path = current_file_path.replace(/\\/g, "/"); | ||
| let relative_path = temp_current_file_path.replace(root_folder_path + "/", ""); | ||
| for (item in window.sodaJSONObj["starting-point"][high_level_folder]["manifest"]) { | ||
| if ( |
| @@ -1546,7 +1465,7 @@ | |||
| folders: {}, | |||
| files: {}, | |||
| path: current_file_path, | |||
| type: "local", | |||
| location: "local", | |||
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the problem, the code on line 1387 should replace all occurrences of backslash (\) in current_file_path with forward slash (/). This is best achieved by using the replace method with a regular expression /\\/g, which matches all backslashes globally. The change should be made only to line 1387 in the file src/renderer/src/scripts/others/tab-effects.js. No new imports or definitions are needed, as this is a standard JavaScript operation.
-
Copy modified line R1387
| @@ -1384,7 +1384,7 @@ | ||
| } | ||
| } | ||
| } else if (extension == ".csv") { | ||
| let temp_current_file_path = current_file_path.replace("\\", "/"); | ||
| let temp_current_file_path = current_file_path.replace(/\\/g, "/"); | ||
| let relative_path = temp_current_file_path.replace(root_folder_path + "/", ""); | ||
| for (item in window.sodaJSONObj["starting-point"][high_level_folder]["manifest"]) { | ||
| if ( |
fix: Hotfix ud UI
| "<tr id='row-current-" + | ||
| keyword + | ||
| newRowIndex + | ||
| "' class='row-" + | ||
| type + | ||
| "'><td class='contributor-table-row'>" + | ||
| indexNumber + | ||
| "</td><td>" + | ||
| newID + | ||
| "</td><td><div class='ui small basic icon buttons contributor-helper-buttons' style='display: flex'><button class='ui button' onclick='window.edit_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='pen icon' style='color: var(--tagify-dd-color-primary)'></i></button><button class='ui button' onclick='window.copy_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='fas fa-copy' style='color: orange'></i></button><button class='ui button' onclick='window.delete_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='trash alternate outline icon' style='color: red'></i></button></div></td></tr>"); | ||
| "_id(this)'><i class='trash alternate outline icon' style='color: red'></i></button></div></td></tr>"; |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the vulnerability, we must explicitly escape any user-controlled values before interpolating them into HTML strings. This especially applies to the newID variable embedded both as the content of a <td> and possibly within an attribute (though not directly in a way that would break out of the attribute context here). The safest way is to encode newID for HTML context using a trusted escaping function.
We will:
- Escape
newIDbefore it is inserted into the HTML string inaddNewIDToTable. - Use a well-known library, such as
he(HTML Entities), to encode it properly since this is a common approach and avoids subtle mistakes in writing a custom escaper. - Update only the relevant code in
src/renderer/src/scripts/metadata-files/subjects-samples.jsby importingheand using it to encodenewIDin the HTML row construction.
-
Copy modified line R8 -
Copy modified line R320
| @@ -5,7 +5,7 @@ | ||
| import introJs from "intro.js"; | ||
| import { clientError } from "../others/http-error-handler/error-handler"; | ||
| import client from "../client"; | ||
|
|
||
| import he from "he"; | ||
| while (!window.baseHtmlLoaded) { | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| } | ||
| @@ -317,7 +317,7 @@ | ||
| "'><td class='contributor-table-row'>" + | ||
| indexNumber + | ||
| "</td><td>" + | ||
| newID + | ||
| he.encode(newID) + | ||
| "</td><td><div class='ui small basic icon buttons contributor-helper-buttons' style='display: flex'><button class='ui button' onclick='window.edit_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='pen icon' style='color: var(--tagify-dd-color-primary)'></i></button><button class='ui button' onclick='window.copy_current_" + |
-
Copy modified lines R94-R95
| @@ -91,7 +91,8 @@ | ||
| "validator": "^13.11.0", | ||
| "vite-plugin-commonjs-externals": "^0.1.3", | ||
| "xlsx": "^0.18.5", | ||
| "zustand": "^4.5.2" | ||
| "zustand": "^4.5.2", | ||
| "he": "^1.2.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@electron-toolkit/eslint-config": "^1.0.1", |
| Package | Version | Security advisories |
| he (npm) | 1.2.0 | None |
| "<tr id='row-current-" + | ||
| keyword + | ||
| newRowIndex + | ||
| "' class='row-" + | ||
| type + | ||
| "'><td class='contributor-table-row'>" + | ||
| indexNumber + | ||
| "</td><td>" + | ||
| secondaryID + | ||
| "</td><td>" + | ||
| newID + | ||
| "</td><td><div class='ui small basic icon buttons contributor-helper-buttons' style='display: flex'><button class='ui button' onclick='window.edit_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='pen icon' style='color: var(--tagify-dd-color-primary)'></i></button><button class='ui button' onclick='window.copy_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='fas fa-copy' style='color: orange'></i></button><button class='ui button' onclick='window.delete_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='trash alternate outline icon' style='color: red'></i></button></div></td></tr>"); | ||
| "_id(this)'><i class='trash alternate outline icon' style='color: red'></i></button></div></td></tr>"; |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best way to fix this problem is to ensure all user-provided (or otherwise tainted) data included in HTML strings is properly escaped for HTML context before concatenation. Rather than inserting these values directly into the HTML via string interpolation, escape each value using a robust HTML-escaping function. This prevents HTML meta-characters from being interpreted by the browser, rendering them as harmless text instead.
Specifically:
- In the function
addNewIDToTable, prior to concatenation in line 329–347, escapenewIDandsecondaryID(as well as any other user-influenced variable, includingindexNumberif it is user-controlled, though it looks auto-assigned). - You may either use a well-known library (like lodash's
_.escape, DOMPurify, or a hand-written escape function), or implement your own basic HTML escape for the small cases needed. - Add the import if using an external library (prefer lodash if allowed, or implement a minimal local function if not).
- Replace direct string interpolation in the vulnerable line(s) to use the escaped values.
Files/regions to change:
- In
src/renderer/src/scripts/metadata-files/subjects-samples.js:- Define an HTML-escape utility function (if not already imported/available).
- In
addNewIDToTable, escapenewIDandsecondaryIDat the point of use.
-
Copy modified lines R284-R294 -
Copy modified lines R340-R342 -
Copy modified line R352 -
Copy modified line R354
| @@ -281,6 +281,17 @@ | ||
| } | ||
| }; | ||
|
|
||
| // Simple HTML escape utility to prevent XSS when inserting user data into HTML | ||
| function escapeHtml(str) { | ||
| if (typeof str !== "string") return str; | ||
| return str | ||
| .replace(/&/g, "&") | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/"/g, """) | ||
| .replace(/'/g, "'"); | ||
| } | ||
|
|
||
| const addNewIDToTable = (newID, secondaryID, type) => { | ||
| var message = ""; | ||
| if (type === "subjects") { | ||
| @@ -326,6 +337,9 @@ | ||
| keyword + | ||
| "_id(this)'><i class='trash alternate outline icon' style='color: red'></i></button></div></td></tr>"; | ||
| } else if (type === "samples") { | ||
| // escape newID and secondaryID before inserting as HTML | ||
| const escapedNewID = escapeHtml(newID); | ||
| const escapedSecondaryID = escapeHtml(secondaryID); | ||
| table.insertRow(rowIndex).outerHTML = | ||
| "<tr id='row-current-" + | ||
| keyword + | ||
| @@ -335,9 +349,9 @@ | ||
| "'><td class='contributor-table-row'>" + | ||
| indexNumber + | ||
| "</td><td>" + | ||
| secondaryID + | ||
| escapedSecondaryID + | ||
| "</td><td>" + | ||
| newID + | ||
| escapedNewID + | ||
| "</td><td><div class='ui small basic icon buttons contributor-helper-buttons' style='display: flex'><button class='ui button' onclick='window.edit_current_" + | ||
| keyword + | ||
| "_id(this)'><i class='pen icon' style='color: var(--tagify-dd-color-primary)'></i></button><button class='ui button' onclick='window.copy_current_" + |
No description provided.