perf: replace empty functions with noopQrl #6871
Conversation
wmertens
added
COMP: performance
🦋 Changeset detectedLatest commit: febced5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
wmertens
force-pushed
the
optimizer-noop
branch
from
31dc690 to
1f6ed4b
Compare
|
hmm the e2e aren't happy :( |
wmertens
force-pushed
the
optimizer-noop
branch
from
1f6ed4b to
f80f6be
Compare
chore(v2): add short changelog of major changes for the next upcoming version
fix: calling component as a function
fix(core): add keys to jsxnodes when needed
chore: merge main into v2
perf(serdes): vnode tree shaking
`flags`, `varProps` and `constProps` This assures compatibility with v1 types
they are not necessary and they make the API harder to grasp
chore: make v2 types v1 compatible
chore: merge main into v2
fix: add signal and store effects to the qrl effects for prefetching
fix: rendering attribute value from array of classes from spread props
- fix rust benchmark - mild dep updates
this speeds up `pnpm build --qwik --dev` by a lot
get it from a different qrl instead
also refactor
refactor(core): schedule QRLs instead of direct execution
TODO retain scope captures. The current approach won't work because it strips the code early. TODO handle null qrls in the manifest
wmertens
force-pushed
the
optimizer-noop
branch
from
f6c783a to
febced5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| c.push(`\n/** Qwik Router Entries (${entries.length}) */`); | ||
| for (let i = 0; i < entries.length; i++) { | ||
| const entry = entries[i]; | ||
| c.push(`export const ${entry.id} = () => import(${JSON.stringify(entry.filePath)});`); |
Check warning
Code scanning / CodeQL
Improper code sanitization Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 2 months ago
To fix the problem, we need to ensure that any potentially dangerous characters in entry.filePath are properly escaped before being included in the generated JavaScript code. We can achieve this by implementing a function that escapes these characters and using it to sanitize entry.filePath.
- Implement a function
escapeUnsafeCharsthat escapes potentially dangerous characters. - Use this function to sanitize
entry.filePathbefore including it in the generated code.
| @@ -2,2 +2,20 @@ | ||
|
|
||
| function escapeUnsafeChars(str: string): string { | ||
| const charMap: { [key: string]: string } = { | ||
| '<': '\\u003C', | ||
| '>': '\\u003E', | ||
| '/': '\\u002F', | ||
| '\\': '\\\\', | ||
| '\b': '\\b', | ||
| '\f': '\\f', | ||
| '\n': '\\n', | ||
| '\r': '\\r', | ||
| '\t': '\\t', | ||
| '\0': '\\0', | ||
| '\u2028': '\\u2028', | ||
| '\u2029': '\\u2029' | ||
| }; | ||
| return str.replace(/[<>\b\f\n\r\t\0\u2028\u2029]/g, x => charMap[x]); | ||
| } | ||
|
|
||
| export function createEntries(ctx: BuildContext, c: string[]) { | ||
| @@ -25,3 +43,3 @@ | ||
| const entry = entries[i]; | ||
| c.push(`export const ${entry.id} = () => import(${JSON.stringify(entry.filePath)});`); | ||
| c.push(`export const ${entry.id} = () => import(${escapeUnsafeChars(JSON.stringify(entry.filePath))});`); | ||
| } |
| } | ||
| errorDiv.setAttribute('q:key', '_error_'); | ||
| const journal: VNodeJournal = []; | ||
| vnode_getDOMChildNodes(journal, vHost).forEach((child) => errorDiv.appendChild(child)); |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Copilot Autofix AI about 2 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| } | ||
|
|
||
| if (key === dangerouslySetInnerHTML) { | ||
| element.innerHTML = value as string; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 2 months ago
To fix this issue, we need to ensure that the value assigned to element.innerHTML is properly sanitized to prevent XSS attacks. The best way to do this is to use a function that escapes HTML special characters before assigning the value to innerHTML.
- Import or define an HTML escaping function if not already available.
- Use this function to sanitize
valuebefore assigning it toelement.innerHTML.
| @@ -669,3 +669,3 @@ | ||
| if (key === dangerouslySetInnerHTML) { | ||
| element.innerHTML = value as string; | ||
| element.innerHTML = escapeHTML(value as string); | ||
| element.setAttribute(QContainerAttr, QContainerValue.HTML); |
| } else if (key === 'value' && key in element) { | ||
| (element as any).value = escapeHTML(String(value)); | ||
| } else if (key === dangerouslySetInnerHTML) { | ||
| (element as any).innerHTML = value!; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 2 months ago
To fix the problem, we need to ensure that any value assigned to innerHTML is properly sanitized to prevent XSS attacks. This can be achieved by using a library like DOMPurify to sanitize the HTML content before assigning it to innerHTML.
- Import the
DOMPurifylibrary. - Use
DOMPurify.sanitizeto sanitize thevaluebefore assigning it toinnerHTML.
| @@ -120,2 +120,3 @@ | ||
| import { isDev } from '@qwik.dev/core/build'; | ||
| import DOMPurify from 'dompurify'; | ||
| import { qwikDebugToString } from '../debug'; | ||
| @@ -895,3 +896,3 @@ | ||
| } else if (key === dangerouslySetInnerHTML) { | ||
| (element as any).innerHTML = value!; | ||
| (element as any).innerHTML = DOMPurify.sanitize(value!); | ||
| } else { |
| @@ -10,3 +10,4 @@ | ||
| "dependencies": { | ||
| "csstype": "^3.1" | ||
| "csstype": "^3.1", | ||
| "dompurify": "^3.2.4" | ||
| }, |
| Package | Version | Security advisories |
| dompurify (npm) | 3.2.4 | None |
| const insertBefore = journal[idx++] as Element | Text | null; | ||
| let newChild: any; | ||
| while (idx < length && typeof (newChild = journal[idx]) !== 'number') { | ||
| insertParent.insertBefore(newChild, insertBefore); |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 2 months ago
To fix the problem, we need to ensure that any text content being inserted into the DOM is properly escaped to prevent XSS attacks. This can be achieved by using a function that escapes HTML special characters before inserting the content.
- We will use the
escapeHTMLfunction to escape any text content before it is inserted into the DOM. - Specifically, we will modify the
vnode_applyJournalfunction to escapenewChildbefore it is inserted usinginsertBefore.
| @@ -925,2 +925,5 @@ | ||
| while (idx < length && typeof (newChild = journal[idx]) !== 'number') { | ||
| if (newChild.nodeType === Node.TEXT_NODE) { | ||
| newChild.nodeValue = escapeHTML(newChild.nodeValue); | ||
| } | ||
| insertParent.insertBefore(newChild, insertBefore); |
This is a fairly trivial improvement, but it does avoid loading empty functions on the client.
To make it work, I needed to run the simplification twice (before and after segmentation) but it's still fast.
TODO: