chore: drop the server tests that only test the platform - #17037
chore: drop the server tests that only test the platform#17037Nic-Polumeyv wants to merge 2 commits into
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/7fee9c85ad3df9a9cc5de04db018930479c77845Open in |
|
📝 WalkthroughWalkthroughThe CSRF test suite now contains one successful POST form request from Merge Risk: 🔵 Low · up to This test-only change reduces CSRF coverage to a trusted-origin case, leaving configured trusted-origin propagation less protected against regression. The change is mergeable with owner awareness of this bounded coverage gap. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/kit/test/apps/basics/unit-test/server.spec.js-201-201 (1)
201-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetain multi-entry
csrf.trustedOriginscoverage.This test validates only one trusted-origin entry.
packages/kit/src/runtime/server/csrf.spec.jsalso tests only a singletontrusted_originsarray. If server configuration drops entries after the first, all remaining tests pass. Keep the additional trusted-origin case here, or add a focused test that passes at least two configured origins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: 06cdfebf-a730-47cc-96bc-22cbf983b8c3
📒 Files selected for processing (1)
packages/kit/test/apps/basics/unit-test/server.spec.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
ff6cb45 to
bb9841b
Compare
Nic-Polumeyv
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kit/test/apps/basics/unit-test/server.spec.js (1)
176-190: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the retained CSRF test with the case it sends.
This test sends no
Originheader, so it covers a missing origin, not an incorrect origin. Rename the test to reflect that case. The response text varies bymethod, not bycontent_type, so use one representative form content type per method.Proposed narrowing
- test('Blocks requests with incorrect origin', async () => { - const content_types = [ - 'application/x-www-form-urlencoded', - 'multipart/form-data', - 'text/plain', - 'text/plaiN' - ]; + test('Reports forbidden response text for mutating form methods', async () => { const methods = ['POST', 'PUT', 'PATCH', 'DELETE']; for (const method of methods) { - for (const content_type of content_types) { - const res = await get('/csrf', { method, headers: { 'content-type': content_type } }); - const message = `request method: ${method}, content-type: ${content_type}`; + const res = await get('/csrf', { + method, + headers: { 'content-type': 'application/x-www-form-urlencoded' } + }); + const message = `request method: ${method}`; expect(res.status, message).toBe(403); expect(await res.text(), message).toBe( `Cross-site ${method} form submissions are forbidden` ); - } } });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: 5f3b44b1-0548-4aa0-a4be-4ace4fa91141
📒 Files selected for processing (2)
packages/kit/test/apps/basics/src/routes/endpoint-input/sha256/+server.jspackages/kit/test/apps/basics/unit-test/server.spec.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
💤 Files with no reviewable changes (1)
- packages/kit/test/apps/basics/src/routes/endpoint-input/sha256/+server.js
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
75eb48d to
63bdd7b
Compare
63bdd7b to
7fee9c8
Compare
1 participant
Two kinds of test in the moved server suite exercise nothing in kit under a direct
Server.respondcall.Six of the eight CSRF cases repeat
csrf.spec.jsone for one: same origin, non-allowed origins, GET, non-form content types, the method loop, missing origin. Only the 403 body text and thecsrf.trustedOriginswiring are checked nowhere else, so those two stay.The slow-read test hands the endpoint an in-memory buffer, so nothing is read slowly and the digest round-trip only proves
Request.body. The backpressure it used to reach over HTTP has its own cases innode/index.spec.js.