fix: replace deprecated csurf and native-url packages#4505
fix: replace deprecated csurf and native-url packages#4505
Conversation
- Replace csurf with csrf-csrf for CSRF protection - Use Node.js built-in URL API instead of native-url - Remove @types/csurf and native-url+0.3.4.patch - Update CSRF middleware to use Double Submit Cookie pattern - Add cookie-parser middleware for CSRF protection - Update MqttClient.ts to use URL constructor - Update Settings.vue to use URL constructor This resolves the npm deprecation warnings during installation. Co-authored-by: robertsLando <[email protected]>
| }, | ||
| ) as RequestHandler, | ||
| ) | ||
| app.use(cookieParser()) |
Check failure
Code scanning / CodeQL
Missing CSRF middleware High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, to fix this issue you should ensure that any state‑changing route that relies on cookie‑based authentication is protected by CSRF middleware. This typically means adding a CSRF protection middleware globally for sensitive routes or per‑route for each POST, PUT, and DELETE endpoint that uses session cookies. In this codebase, doubleCsrf from csrf-csrf is already imported, and some routes (/api/authenticate, /api/password) use doubleCsrfProtection, so the best fix is to consistently apply this existing middleware to the remaining state‑changing, authenticated routes.
The single best fix with minimal functional impact is:
- Leave the existing
cookieParserand body parsers as they are. - Keep using the existing
doubleCsrfProtectionmiddleware instance (assumed to be defined earlier inapi/app.tssince it’s imported and already used). - Add
doubleCsrfProtectioninto the middleware chain for all authenticated, state‑changing routes that currently lack CSRF protection. That includes the following routes inapi/app.ts:POST /api/restartPOST /api/statisticsPOST /api/versionsPOST /api/importConfigPUT /api/storeDELETE /api/storePUT /api/store-multiPOST /api/store-multiPOST /api/store/uploadPOST /api/debug/startPOST /api/debug/stopPOST /api/debug/cancel
- Preserve middleware order such that
doubleCsrfProtectionruns beforeisAuthenticated(or at least before the route handler). In the existing code,doubleCsrfProtectioncomes beforeisAuthenticatedon/api/password, so we should follow the same pattern for consistency.
No new imports or helper methods are required; we just add doubleCsrfProtection as an extra middleware argument on the listed routes.
Pull Request Test Coverage Report for Build 20577551083Details
💛 - Coveralls |
robertsLando
left a comment
There was a problem hiding this comment.
When I try to login from UI I get this error:
ForbiddenError: invalid csrf token
at doubleCsrf (file:///home/daniel/GitProjects/zwave-js-ui/node_modules/csrf-csrf/dist/index.js:30:33)
at file:///home/daniel/GitProjects/zwave-js-ui/api/app.ts:467:34
at ModuleJob.run (node:internal/modules/esm/module_job:345:25)
at onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:665:26)
at asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)
Also now that you removed the only patch available you could also remove the patch-package npm module from dependencies (remember to also remove postinstall script).
There was a problem hiding this comment.
Pull request overview
This PR addresses deprecation warnings by replacing two deprecated npm packages: csurf (archived CSRF protection) with csrf-csrf, and native-url (which depends on deprecated querystring) with Node.js's built-in URL API. The changes successfully remove deprecation warnings but introduce critical bugs in the CSRF protection implementation that will prevent it from functioning correctly.
Key Changes
- Replaced
csurfwithcsrf-csrffor CSRF protection using the Double Submit Cookie pattern - Migrated from
native-urlto Node.js built-inURLAPI for URL parsing in both backend and frontend - Removed deprecated package dependencies and associated type definitions
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| api/app.ts | Replaced csurf with csrf-csrf middleware, added cookie-parser configuration, updated CSRF protection for authenticate and password endpoints |
| api/lib/MqttClient.ts | Replaced native-url with Node.js URL API for parsing MQTT broker URLs with protocol/hostname extraction |
| src/views/Settings.vue | Replaced native-url with URL API for detecting secure MQTT protocols (mqtts/tls) |
| package.json | Removed csurf and native-url dependencies, added csrf-csrf dependency |
| package-lock.json | Updated lock file reflecting dependency changes and transitive dependency removals |
| patches/native-url+0.3.4.patch | Removed patch file no longer needed after removing native-url dependency |
| ) | ||
|
|
||
| const parsed = url.parse(config.host || '') | ||
| let parsed: URL | null = null |
There was a problem hiding this comment.
The variable parsed is declared on line 353 but is never used after the try-catch block. It's only used to extract protocol and hostname which are then stored in separate variables. Since parsed serves no further purpose after the extraction, this is unnecessary variable declaration.
Consider removing the parsed variable and directly extracting the values within the try block without storing the URL object.
api/app.ts
Outdated
| sameSite: 'strict', | ||
| path: '/', | ||
| secure: !!process.env.HTTPS || !!process.env.USE_SECURE_COOKIE, | ||
| httpOnly: true, |
There was a problem hiding this comment.
The CSRF cookie is configured with httpOnly: true, which prevents JavaScript from reading the cookie value. The csrf-csrf library uses the Double Submit Cookie pattern, which requires the client to read the cookie and send its value in a request header (typically x-csrf-token).
With httpOnly: true, client-side JavaScript cannot access the cookie to extract the token, breaking the CSRF protection mechanism. The cookie should either:
- Have
httpOnly: falseto allow JavaScript to read it, OR - You need to destructure
generateTokenfrom thedoubleCsrfcall and create a GET endpoint (e.g.,/api/csrf-token) that callsgenerateToken(req, res)to provide the token to clients, while keeping cookies httpOnly
The typical pattern with csrf-csrf is:
- Keep cookies httpOnly: false for double-submit pattern, OR
- Use signed/encrypted cookies with httpOnly: true and provide a token generation endpoint
api/app.ts
Outdated
| const { doubleCsrfProtection } = doubleCsrf({ | ||
| getSecret: () => sessionSecret, | ||
| getSessionIdentifier: (req) => req.session?.user?.username || '', | ||
| cookieName: '__Host-psifi.x-csrf-token', |
There was a problem hiding this comment.
The cookie name __Host-psifi.x-csrf-token uses the __Host- prefix, which requires specific conditions to work:
- The cookie must be set with the
Secureattribute (only works over HTTPS) - The cookie must be set with
Path=/ - The cookie must NOT have a
Domainattribute
In development environments or HTTP-only deployments where secure is false (line 623), browsers will reject cookies with the __Host- prefix. This will cause CSRF protection to fail silently in non-HTTPS environments.
Consider either:
- Using a regular cookie name like
x-csrf-tokenwithout the__Host-prefix - Using the
__Host-prefix only whensecureis true, otherwise use a regular name - Documenting that CSRF protection requires HTTPS to function properly
- Replace csrf-csrf with csrf-sync (Synchronizer Token Pattern) - Add /api/csrf-token endpoint to generate CSRF tokens - Update frontend to automatically fetch and include CSRF tokens - Move session middleware before history middleware for proper CSRF flow - Set saveUninitialized: true to allow CSRF tokens for unauthenticated users - Remove patch-package dependency and postinstall script (no patches remaining) - Update axios interceptors to handle CSRF tokens automatically Co-authored-by: robertsLando <[email protected]>
Fixed in c7a9a20. The issue was that I initially used Changes made:
The login flow now works correctly with CSRF protection. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Removes npm deprecation warnings for
[email protected](archived) and[email protected](transitive dependency vianative-url).Changes
CSRF Protection - Replace
csurfwithcsrf-sync/api/csrf-tokenendpoint to generate CSRF tokens for frontend/api/authenticateand/api/passwordendpoints to usecsrfSynchronisedProtectionsaveUninitialized: trueto allow CSRF tokens for unauthenticated usersFrontend CSRF Handling - Automatic token management in
src/apis/ConfigApis.jsx-csrf-tokenheaderURL Parsing - Replace
native-urlwith Node.js built-inURLAPIapi/lib/MqttClient.tsandsrc/views/Settings.vueto usenew URL()patches/native-url+0.3.4.patchCleanup
patch-packagenpm module (no longer needed)postinstallscript from package.jsonDependencies
Removed:
csurf,@types/csurf,native-url,patch-packageAdded:
[email protected](no vulnerabilities)Testing
npm installOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.