Update unit testing flows partial migration to vitest#7484
Update unit testing flows partial migration to vitest#7484jasonsaayman merged 38 commits intov1.xfrom
Conversation
| }); | ||
|
|
||
| it('should support params', async () => { | ||
| server = await startHTTPServer((req, res) => res.end(req.url)); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
In general, to fix reflected XSS when echoing user-controlled values like req.url, you should escape or encode the data appropriately for the output context (HTML, attribute, JavaScript, etc.) before sending it in the HTTP response. In Node.js/Express-style code, a common approach is to use a well-known library such as escape-html to HTML-encode characters that could break out of the intended context (<, >, ", ', &, etc.).
For this specific test file, the minimal, non-functional change is to HTML-escape req.url before passing it to res.end. We can do this by importing the escape-html package at the top of tests/unit/adapters/fetch.test.js and then replacing res.end(req.url) on line 374 with res.end(escape(req.url)). This preserves the behavior of echoing back the URL text (now escaped), which is generally sufficient for a test that checks query parameter handling, while eliminating the direct reflection of raw user input. No other behavior in the tests needs to change.
Concretely:
- Add
import escape from 'escape-html';alongside the other imports at the top oftests/unit/adapters/fetch.test.js. - Change the server handler in the "should support params" test from
res.end(req.url)tores.end(escape(req.url)).
| @@ -14,6 +14,7 @@ | ||
| import { AbortController } from 'abortcontroller-polyfill/dist/cjs-ponyfill.js'; | ||
| import util from 'util'; | ||
| import NodeFormData from 'form-data'; | ||
| import escape from 'escape-html'; | ||
|
|
||
| const pipelineAsync = util.promisify(stream.pipeline); | ||
|
|
||
| @@ -371,7 +372,7 @@ | ||
| }); | ||
|
|
||
| it('should support params', async () => { | ||
| server = await startHTTPServer((req, res) => res.end(req.url)); | ||
| server = await startHTTPServer((req, res) => res.end(escape(req.url))); | ||
|
|
||
| const { data } = await fetchAxios.get('/?test=1', { | ||
| params: { |
| @@ -175,7 +175,8 @@ | ||
| "dependencies": { | ||
| "follow-redirects": "^1.15.11", | ||
| "form-data": "^4.0.5", | ||
| "proxy-from-env": "^1.1.0" | ||
| "proxy-from-env": "^1.1.0", | ||
| "escape-html": "^1.0.3" | ||
| }, | ||
| "bundlesize": [ | ||
| { |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
| it('should get response headers', async () => { | ||
| server = await startHTTPServer((req, res) => { | ||
| res.setHeader('foo', 'bar'); | ||
| res.end(req.url); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
In general, to prevent reflected XSS when sending user-controlled data (like req.url) in an HTTP response, you should apply appropriate output encoding for the context (HTML, attribute, JS, etc.), or otherwise ensure the data is safe (e.g., by validation or by returning it as a non-executable content type like text/plain with proper escaping).
For this specific test, the minimal, non‑functional change is to HTML-escape req.url before sending it. The test only checks that the exact string returned by the server matches a specific URL, so we also need to ensure that the test cases that depend on the raw URL either (a) compare against the encoded value, or (b) avoid echoing the URL at all when we don't need its exact value. Here, the params test ('should support params') asserts the raw URL string; we can avoid XSS in that test by ending the response with a constant safe string and only using req.url on the client side (the client already knows what URL it requested). For the 'should get response headers' test, the response body is irrelevant (we only inspect headers), so we can replace res.end(req.url) with a constant safe string. Concretely:
- In the "support params" test, change the server callback from
res.end(req.url);tores.end('OK');and assert against something that does not require the server to echo the URL, e.g., assert onconfig.urlor the request URL actually used (whichaxiosexposes). However, we are constrained to only edit shown snippets and must not assume extra axios config fields beyond those already used. The simplest XSS-free change is to HTML-escapereq.urland update the assertion to match that escaped string. - In the "get response headers" test, replace
res.end(req.url);withres.end('OK');because the body is unused in the test, removing the flow of user input into the response.
To implement this, within tests/unit/adapters/fetch.test.js we will:
- Add an import for a well-known escaping library such as
escape-html. - Use
escape(req.url)instead ofreq.urlwhere the echoed value is needed, and adjust the corresponding assertion to expect the escaped value. - For the headers test where the body is irrelevant, change
res.end(req.url);tores.end('OK');.
| @@ -14,6 +14,7 @@ | ||
| import { AbortController } from 'abortcontroller-polyfill/dist/cjs-ponyfill.js'; | ||
| import util from 'util'; | ||
| import NodeFormData from 'form-data'; | ||
| import escape from 'escape-html'; | ||
|
|
||
| const pipelineAsync = util.promisify(stream.pipeline); | ||
|
|
||
| @@ -371,7 +372,7 @@ | ||
| }); | ||
|
|
||
| it('should support params', async () => { | ||
| server = await startHTTPServer((req, res) => res.end(req.url)); | ||
| server = await startHTTPServer((req, res) => res.end(escape(req.url))); | ||
|
|
||
| const { data } = await fetchAxios.get('/?test=1', { | ||
| params: { | ||
| @@ -380,7 +381,7 @@ | ||
| }, | ||
| }); | ||
|
|
||
| assert.strictEqual(data, '/?test=1&foo=1&bar=2'); | ||
| assert.strictEqual(data, escape('/?test=1&foo=1&bar=2')); | ||
| }); | ||
|
|
||
| it('should handle fetch failed error as an AxiosError with ERR_NETWORK code', async () => { | ||
| @@ -396,7 +397,7 @@ | ||
| it('should get response headers', async () => { | ||
| server = await startHTTPServer((req, res) => { | ||
| res.setHeader('foo', 'bar'); | ||
| res.end(req.url); | ||
| res.end('OK'); | ||
| }); | ||
|
|
||
| const { headers } = await fetchAxios.get('/', { |
| @@ -175,7 +175,8 @@ | ||
| "dependencies": { | ||
| "follow-redirects": "^1.15.11", | ||
| "form-data": "^4.0.5", | ||
| "proxy-from-env": "^1.1.0" | ||
| "proxy-from-env": "^1.1.0", | ||
| "escape-html": "^1.0.3" | ||
| }, | ||
| "bundlesize": [ | ||
| { |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
4f39f14 to
12880b9
Compare
There was a problem hiding this comment.
2 issues found across 91 files
Confidence score: 4/5
- This PR looks safe to merge overall: both findings are low severity (4/10) and mainly affect test coverage/portability rather than core runtime behavior.
- The main risk is in
tests/unit/adapters/fetch.test.js, where using the default axios instance means fetch-adapter basic auth behavior is not actually validated, so a fetch-specific regression could slip through. - The issue in
.cursor/plans/rewrite_helper_tests_a2f815ee.plan.mdis housekeeping-oriented but worth fixing, since absolute local paths are machine-specific and reduce repo portability. - Pay close attention to
tests/unit/adapters/fetch.test.jsand.cursor/plans/rewrite_helper_tests_a2f815ee.plan.md- ensure the fetch adapter is truly exercised and replace absolute paths with repo-relative ones.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/unit/adapters/fetch.test.js">
<violation number="1" location="tests/unit/adapters/fetch.test.js:284">
P2: This test uses the default axios instance, so it doesn't exercise the fetch adapter and won't catch fetch-specific basic auth regressions. Use the fetchAxios instance here.</violation>
</file>
<file name=".cursor/plans/rewrite_helper_tests_a2f815ee.plan.md">
<violation number="1" location=".cursor/plans/rewrite_helper_tests_a2f815ee.plan.md:27">
P2: Avoid committing absolute local file paths; they are machine-specific and break portability. Use repo-relative paths instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Summary by cubic
Partially migrates unit tests to Vitest with unit and browser projects, adds a lightweight test server, and keeps Mocha/Karma running. Also transpiles the Node CJS bundle for Node 12.
Description
Docs
Testing
Written for commit 12880b9. Summary will update on new commits.