refactor: v2 release #6903
refactor: v2 release #6903
Conversation
🦋 Changeset detectedLatest commit: 338ac31 The changes in this PR will be included in the next version bump. 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}
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 12 months 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 about 2 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 about 2 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 about 2 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 about 2 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 about 2 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 about 2 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 about 2 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 about 2 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 about 2 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' }, | ||
| }); |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: styfle/[email protected] | ||
| if: github.event_name == 'pull_request' | ||
| with: | ||
| workflow_id: ${{ github.event.workflow.id }} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, you should add a permissions block to the workflow or to the specific job. The best practice is to set the minimal permissions required for the workflow to function. In this case, the workflow uses the styfle/cancel-workflow-action, which typically only needs actions: write permission to cancel workflow runs. Therefore, you should add a permissions block with actions: write at either the workflow root (to apply to all jobs) or at the job level (to apply only to the cancel job). The change should be made in .github/workflows/cancel.yml, above the jobs: key (for workflow-level) or inside the cancel: job (for job-level). No additional imports or definitions are needed.
| @@ -2,2 +2,4 @@ | ||
| name: Cancel | ||
| permissions: | ||
| actions: write | ||
| on: |
| 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 6 months 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.
perf: scheduler and rendering improvements
fix: trim script added by vite in dev mode
fix: dont block qrl chores
fix(serdes): preload qrls work again
fix(computed): actually preload (v2)
fix(asynccomputed): don't throw twice
fix: don't emit script before qwik style element
feat: support promises in attributes
V2 Version Packages (beta)
fix(v2): qwikVite client outDir fix
fix: serializing reused qrl
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.