Conversation
🦋 Changeset detectedLatest commit: bfe1059 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@qwik.dev/core
@qwik.dev/router
eslint-plugin-qwik
create-qwik
@qwik.dev/optimizer
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
| 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
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI over 1 year ago
To fix the problem, we need to ensure that entry.filePath is properly sanitized before being used in the dynamically generated JavaScript code. We can achieve this by escaping potentially dangerous characters in the entry.filePath string. This can be done by implementing a function similar to escapeUnsafeChars from the example provided in the background section.
- Implement a function
escapeUnsafeCharsto escape 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))});`); | ||
| } |
| } else if (key === 'value' && key in element) { | ||
| (element as any).value = String(value); | ||
| } else if (key === dangerouslySetInnerHTML) { | ||
| (element as any).innerHTML = value!; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML
| return null; | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
The best fix is to replace String(e || 'Error') in the HTTP response body with a generic error message that does not provide any implementation details (e.g., "An unexpected error occurred."). The actual exception's details (e) should still be logged to the server console (as is already done with console.error(e)), so as not to lose valuable debugging information. Specifically:
- In the catch block starting on line 90, modify the
Responsecreation to use a hard-coded generic error message instead of stringifying the caught exception. - No new imports or methods are needed, as logging is already in place.
| @@ -89,7 +89,7 @@ | ||
| return null; | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('An unexpected error occurred.', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'bun-server' }, | ||
| }); |
| }); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the issue, we should replace String(e || 'Error') in the error response with a generic message, e.g. "An internal server error occurred", and log the actual error on the server using console.error(e).
- Don't send details about the error, including stack traces or error messages, in the HTTP response.
- Only log the error server-side so developers can triage it.
- Make this change only where the error is sent in the HTTP response in file
packages/qwik-router/src/middleware/bun/index.ts, specifically in the catch block on lines 115–121.
| @@ -114,7 +114,7 @@ | ||
| }); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('An internal server error occurred', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'bun-server' }, | ||
| }); |
| return null; | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, do not return the raw stringified exception (String(e)) in the HTTP response to the client. Instead, return a generic error message such as "Internal Server Error" or "An error occurred". Retain the current behavior of logging the full details (console.error(e);) on the server so developers can still perform diagnosis. The actual code change is to update line 170 in the staticFile handler to send only a generic message instead of the stringified error. No new methods or definitions are necessary, and there is no need to add an external logging library unless requested—the existing console.error is sufficient.
| @@ -167,7 +167,7 @@ | ||
| return null; | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('Internal Server Error', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'bun-server' }, | ||
| }); |
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the information exposure issue, the error handling code (specifically within the catch (e: any) block at or after line 127 in packages/qwik-router/src/middleware/cloudflare-pages/index.ts) should be updated so that the HTTP response to the user does not include any information from the caught exception. Instead, return a generic error message such as "Internal Server Error" or "An unexpected error occurred". The detailed error (including stack trace/message) should continue to be logged server-side using console.error(e);.
No additional imports or methods are required to implement the fix, as the existing logging mechanism is sufficient. Only the return statement in the catch block needs to be modified.
| @@ -126,7 +126,7 @@ | ||
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('Internal Server Error', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'cloudflare-pages' }, | ||
| }); |
| return null; | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, modify the router's error-returning code (lines 91–97). Instead of passing potentially sensitive contents of the error (such as a stack trace or message) to the Response body, return a generic error message like "Internal Server Error". Server-side logging (console.error(e)) should be retained for debugging. Only the router function's catch block (lines 91–97) needs changing; other usages (such as in the notFound handler) may remain unless similarly problematic and flagged, but should be checked separately.
No new imports or dependencies are required for this fix because the solution only involves modifying the content of the error message sent in the response body.
| @@ -90,7 +90,7 @@ | ||
| return null; | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response("Internal Server Error", { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'deno-server' }, | ||
| }); |
| }); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the issue, we should avoid sending stack trace or error object details to the client within error responses. Instead, send a generic message like "Internal Server Error" or "An unexpected error occurred." For server-side debugging, the full error (including stack trace) should be logged using console.error(e) as is already done. Specifically, in packages/qwik-router/src/middleware/deno/index.ts, on line 118 inside the catch block of the notFound handler, change new Response(String(e || 'Error'), ...) to new Response("Internal Server Error", ...) (or a similarly generic message). No new methods or imports are needed, as logging is already present.
| @@ -115,7 +115,7 @@ | ||
| }); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('Internal Server Error', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'deno-server' }, | ||
| }); |
| return null; | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix this information exposure, we should make sure that the client only ever receives a generic error message, such as "Internal Server Error" or "An error occurred", instead of the actual error message or stack trace. The detailed error should still be logged on the server for debugging purposes, as is already done with console.error(e).
How to apply the fix:
- Change the response on line 163 to return a constant string like
"An error occurred"or"Internal Server Error"instead of using the stringified error object. - This change should only affect the call to the
Responseconstructor in the catch block inside thestaticFilefunction, so that existing behavior (server-side logging) remains unchanged and error information is still available to developers.
Files/Regions/Lines to change:
- File:
packages/qwik-router/src/middleware/deno/index.ts - Editing the return statement at line 163.
What is needed:
- No additional imports or utility methods are required, as the fix is a literal string substitution.
| @@ -160,7 +160,7 @@ | ||
| return null; | ||
| } catch (e) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('Internal Server Error', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'deno-server' }, | ||
| }); |
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, the error handler should return a generic error message to the user rather than exposing the details of the thrown error object. The stack trace and error details should be logged on the server (using console.error(e) as is currently done) but not exposed in the body of the HTTP response. In this file, specifically, we need to change line 91 so that the response always contains a generic message, such as "Internal Server Error" or "An unexpected error occurred". No new imports are needed—just modify the error response construction to avoid passing String(e) to the client.
| @@ -88,7 +88,7 @@ | ||
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response('Internal Server Error', { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'netlify-edge' }, | ||
| }); |
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix this issue, we should ensure that error details sent back to clients do not expose stack traces, implementation details, or server-side file paths. Instead, only a generic error message ("Internal Server Error" or similar) should be returned in the HTTP response, while details should be logged server-side for debugging purposes.
Specifically, in packages/qwik-router/src/middleware/vercel-edge/index.ts, within the catch block at line 116, only a generic error message should be sent in the response, while the detailed error (e) should continue to be logged using console.error(e). The relevant code to edit starts at line 116:
- Replace
String(e || 'Error')(user-facing) with a safe, generic string like"Internal Server Error". - No extra dependency is required: logging with
console.errorsuffices for server-side logs. - Ensure the change is only made at the point of user-facing error creation; the logging remains unchanged.
| @@ -115,7 +115,7 @@ | ||
| }); | ||
| } catch (e: any) { | ||
| console.error(e); | ||
| return new Response(String(e || 'Error'), { | ||
| return new Response("Internal Server Error", { | ||
| status: 500, | ||
| headers: { 'Content-Type': 'text/plain; charset=utf-8', 'X-Error': 'vercel-edge' }, | ||
| }); |
| const tarballOutDir = join(workspaceRoot, 'temp', 'tarballs'); | ||
| for (const [name, cfg] of Object.entries(packageCfg)) { | ||
| const out = execSync(`pnpm pack --pack-destination=${tarballOutDir}`, { | ||
| const out = execSync(`pnpm pack --json --pack-destination=${tarballOutDir}`, { |
Check warning
Code scanning / CodeQL
Shell command built from environment values
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix the issue, the shell command should avoid direct interpolation of environment-derived values. Instead, the execSync call should be replaced with execFileSync, which allows passing arguments separately. This ensures that the shell does not interpret special characters in the tarballOutDir or other dynamically constructed paths.
Steps to fix:
- Replace the
execSynccall withexecFileSync. - Pass the
pnpmcommand and its arguments as separate parameters toexecFileSync. - Ensure that the
tarballOutDirand other paths are passed as arguments, not interpolated into the command string.
| @@ -1,2 +1,2 @@ | ||
| import { execSync } from 'child_process'; | ||
| import { execFileSync } from 'child_process'; | ||
| import { existsSync, writeFileSync } from 'fs'; | ||
| @@ -30,3 +30,3 @@ | ||
| for (const [name, cfg] of Object.entries(packageCfg)) { | ||
| const out = execSync(`pnpm pack --json --pack-destination=${tarballOutDir}`, { | ||
| const out = execFileSync('pnpm', ['pack', '--json', `--pack-destination=${tarballOutDir}`], { | ||
| cwd: join(workspaceRoot, cfg.packagePath), |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
feat(qwikloader): support events capture phase
fix: run CI on Node 24 while keeping e2e on Node 22
…hat point to missing files
docs: docs color fixes and partytown bump
…moving their experimental feature flags
fix: expose `getClientManifest` from the `@qwik.dev/core` root packag…
…-preload-helper feat(perf): remove vite preload helper from built outputs
…rite-stable feat: make `usePreventNavigate$` and `request.rewrite()` stable
fix: prevent Qwik Vite virtual JSX modules from emitting sourcemaps
fix: escape URI-reserved vnode directive chars in serialized keys
- Bump versions of @typescript-eslint/utils, @typescript-eslint/rule-tester, and related packages to 8.59.2 in pnpm-lock.yaml and package.json. - Refactor eslint-plugin-qwik rules to utilize utility functions for JSX attribute checks and improve code readability. - Remove redundant code and enhance type safety in various rule implementations. - Ensure consistent handling of JSX attributes across rules.
refactor(eslint): modernize plugin deps and rule typing
v2: merge main
Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/9afbb8c3-ec07-4807-8c43-60ee07d1f88e Co-authored-by: wmertens <54934+wmertens@users.noreply.github.com>
preloader: remove unused top-level timestamp initialization in constants
Fix SSR backpatch serialization for async `style` and `class` attributes
fix(core): cursor merging on replacing
…in v2 SSR (#8645) * Initial plan * fix(ssr): avoid inline loader emission in svg/math content Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/1be95a51-99b5-4679-bfa3-601d3c622897 * fix(ssr): cover remaining unsafe inline script contexts Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/04d44be2-e4a7-4870-b388-dcbdc73d8db4 * test(ssr): document internal guard assertion Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/04d44be2-e4a7-4870-b388-dcbdc73d8db4 * Remove 'plaintext' element handling in SSR Removed handling for 'plaintext' element in Qwik SSR. It is deprecated and would break Qwik anyway * fix(ssr): handle plaintext loader timing and loop-style lint Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/e986ac63-b440-49a7-8f6e-8acf4c4e1031 * fix(ssr): drop deprecated plaintext loader handling Agent-Logs-Url: https://github.com/QwikDev/qwik/sessions/3f4c4b8c-5b5d-42d1-a5b5-bc1b391399e7 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Wout Mertens <Wout.Mertens@gmail.com>
…gn (#8638) (#8647) * fix(optimizer): skip props destructuring when a destructured param is reassigned The props-destructuring pass rewrites identifier *reads* of a destructured parameter to `_rawProps.<name>`, but does not rewrite assignment LHS. If the body reassigns a destructured binding (via `=`, `??=`/compound assign, `++`/`--`, or destructuring-assignment LHS like `({foo} = …)` and `[foo] = …`), the LHS was left as a now-undeclared identifier — silently miscompiling the function and producing a strict-mode `ReferenceError` at runtime. In some cases a downstream simplifier even dropped the assignment entirely with no error. Refuse the transform whenever the body reassigns any destructured param. This is safe because Qwik component props are read-only, so reassigning one is semantically meaningless — affected code is either a non-component helper that happens to share the destructuring shape, or a bug in user code that should be surfaced rather than silently miscompiled. Adds regression tests covering template-literal, string-literal, nullish-assign, update-expression (`++`), object-destructuring-assignment (`({foo} = …)`), and array-destructuring-assignment (`[foo] = …`) reassignment forms. * fix(optimizer): also refuse props destructuring on for-of/for-in reassignment * test(e2e): regression for #8638 destructured-reassign helper in DocumentHead * lint(e2e): add braces around if in #8638 head-helper * chore: add changeset for #8638 optimizer fix * test(optimizer): consolidate #8638 destructure-reassign snapshots into direct assertions
This PR is for showing progress on v2, and having installable npm packages.
DO NOT MERGE
The changes are meant to be readable and maintainable, so if things are unclear please let us know.