Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 50 additions & 36 deletions api/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import type { Request, RequestHandler, Response, Router } from 'express'
import express from 'express'
import history from 'connect-history-api-fallback'
import cors from 'cors'
import csrf from 'csurf'
import cookieParser from 'cookie-parser'
import { csrfSync } from 'csrf-sync'
import morgan from 'morgan'
import type { Settings, User } from './config/store.ts'
import store from './config/store.ts'
Expand Down Expand Up @@ -532,6 +533,7 @@ app.use(
},
) as RequestHandler,
)
app.use(cookieParser())

Check failure

Code scanning / CodeQL

Missing CSRF middleware High

This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.
This cookie middleware is serving a
request handler
without CSRF protection.

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:

  1. Leave the existing cookieParser and body parsers as they are.
  2. Keep using the existing doubleCsrfProtection middleware instance (assumed to be defined earlier in api/app.ts since it’s imported and already used).
  3. Add doubleCsrfProtection into the middleware chain for all authenticated, state‑changing routes that currently lack CSRF protection. That includes the following routes in api/app.ts:
    • POST /api/restart
    • POST /api/statistics
    • POST /api/versions
    • POST /api/importConfig
    • PUT /api/store
    • DELETE /api/store
    • PUT /api/store-multi
    • POST /api/store-multi
    • POST /api/store/upload
    • POST /api/debug/start
    • POST /api/debug/stop
    • POST /api/debug/cancel
  4. Preserve middleware order such that doubleCsrfProtection runs before isAuthenticated (or at least before the route handler). In the existing code, doubleCsrfProtection comes before isAuthenticated on /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.


Suggested changeset 1
api/app.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/api/app.ts b/api/app.ts
--- a/api/app.ts
+++ b/api/app.ts
@@ -1367,6 +1367,7 @@
 app.post(
 	'/api/restart',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1415,6 +1416,7 @@
 app.post(
 	'/api/statistics',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1461,6 +1463,7 @@
 app.post(
 	'/api/versions',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1510,6 +1513,7 @@
 app.post(
 	'/api/importConfig',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		let config = req.body.data
@@ -1622,8 +1626,13 @@
 	}
 })
 
-app.put('/api/store', storeLimiter, isAuthenticated, async function (req, res) {
-	try {
+app.put(
+	'/api/store',
+	storeLimiter,
+	doubleCsrfProtection,
+	isAuthenticated,
+	async function (req, res) {
+		try {
 		const reqPath = getSafePath(req)
 
 		const isNew = req.query.isNew === 'true'
@@ -1656,6 +1665,7 @@
 app.delete(
 	'/api/store',
 	storeLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1675,6 +1685,7 @@
 app.put(
 	'/api/store-multi',
 	storeLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1693,6 +1704,7 @@
 app.post(
 	'/api/store-multi',
 	storeLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		const files = req.body.files || []
@@ -1756,6 +1768,7 @@
 app.post(
 	'/api/store/upload',
 	storeLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		let file: any
@@ -1813,6 +1826,7 @@
 app.post(
 	'/api/debug/start',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1846,6 +1860,7 @@
 app.post(
 	'/api/debug/stop',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
@@ -1883,6 +1898,7 @@
 app.post(
 	'/api/debug/cancel',
 	apisLimiter,
+	doubleCsrfProtection,
 	isAuthenticated,
 	async function (req, res) {
 		try {
EOF
Copilot is powered by AI and may make mistakes. Always verify output.
app.use(express.json({ limit: '50mb' }) as RequestHandler)
app.use(
express.urlencoded({
Expand All @@ -541,6 +543,33 @@ app.use(
}) as RequestHandler,
)

// enable sessions management - must be before history middleware for CSRF to work
app.use(
session({
name: 'zwave-js-ui-session',
secret: sessionSecret,
resave: false,
saveUninitialized: true, // Required for CSRF protection to work
store: new FileStore({
path: path.join(storeDir, 'sessions'),
logFn: (...args: any[]) => {
// skip ENOENT errors
if (
args &&
args.filter((a) => a.indexOf('ENOENT') >= 0).length === 0
) {
logger.debug(args[0])
}
},
}),
cookie: {
secure: !!process.env.HTTPS || !!process.env.USE_SECURE_COOKIE,
httpOnly: true, // prevents cookie to be sent by client javascript
maxAge: 24 * 60 * 60 * 1000, // one day
},
}),
)

// must be placed before history middleware
app.use(function (req, res, next) {
if (pluginsRouter !== undefined) {
Expand All @@ -550,6 +579,17 @@ app.use(function (req, res, next) {
}
})

// CSRF token endpoint - must be before history middleware, placed here as a route
// Session middleware will have run by the time this route is matched
app.get('/api/csrf-token', apisLimiter, function (req, res) {
// Ensure session exists before generating token
if (!req.session) {
return res.status(500).json({ error: 'Session not initialized' })
}
const token = generateToken(req)
res.json({ token })
})

app.use(
// @ts-expect-error types not matching
history({
Expand Down Expand Up @@ -582,37 +622,13 @@ app.use('/', express.static(utils.joinPath(false, 'dist')))

app.use(cors({ credentials: true, origin: true }))

// enable sessions management
app.use(
session({
name: 'zwave-js-ui-session',
secret: sessionSecret,
resave: false,
saveUninitialized: false,
store: new FileStore({
path: path.join(storeDir, 'sessions'),
logFn: (...args: any[]) => {
// skip ENOENT errors
if (
args &&
args.filter((a) => a.indexOf('ENOENT') >= 0).length === 0
) {
logger.debug(args[0])
}
},
}),
cookie: {
secure: !!process.env.HTTPS || !!process.env.USE_SECURE_COOKIE,
httpOnly: true, // prevents cookie to be sent by client javascript
maxAge: 24 * 60 * 60 * 1000, // one day
},
}),
)

// Node.js CSRF protection middleware.
// Requires either a session middleware or cookie-parser to be initialized first.
const csrfProtection = csrf({
value: (req) => req.csrfToken(),
// Node.js CSRF protection middleware using the Synchronizer Token pattern.
// This is more appropriate for session-based authentication than Double Submit Cookie.
const { csrfSynchronisedProtection, generateToken } = csrfSync({
getTokenFromRequest: (req) => {
// Check multiple possible locations for the token
return req.body._csrf || req.query._csrf || req.headers['x-csrf-token']
},
})

// ### SOCKET SETUP
Expand Down Expand Up @@ -879,8 +895,7 @@ app.get('/api/auth-enabled', apisLimiter, function (req, res) {
app.post(
'/api/authenticate',
loginLimiter,
// @ts-expect-error types not matching
csrfProtection,
csrfSynchronisedProtection,
async function (req, res) {
const token = req.body.token
let user: User
Expand Down Expand Up @@ -976,8 +991,7 @@ app.get('/api/logout', apisLimiter, isAuthenticated, function (req, res) {
app.put(
'/api/password',
apisLimiter,
// @ts-expect-error types not matching
csrfProtection,
csrfSynchronisedProtection,
isAuthenticated,
async function (req, res) {
try {
Expand Down
20 changes: 14 additions & 6 deletions api/lib/MqttClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { storeDir } from '../config/app.ts'
import { ensureDir } from './utils.ts'
import { Manager } from 'mqtt-jsonl-store'
import { join } from 'node:path'
import url from 'native-url'

const logger = module('Mqtt')

Expand Down Expand Up @@ -351,10 +350,21 @@ class MqttClient extends TypedEventEmitter<MqttClientEventCallbacks> {
MqttClient.NAME_PREFIX + (process.env.MQTT_NAME || config.name),
)

const parsed = url.parse(config.host || '')
let parsed: URL | null = null
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
let protocol = 'mqtt'
let hostname = config.host

if (parsed.protocol) protocol = parsed.protocol.replace(/:$/, '')
// Try to parse as URL if it contains a protocol
try {
parsed = new URL(config.host || '')
if (parsed.protocol) {
protocol = parsed.protocol.replace(/:$/, '')
hostname = parsed.hostname
}
} catch {
// If parsing fails, treat as hostname without protocol
hostname = config.host
}

const options: IClientOptions = {
clientId: this._clientID,
Expand Down Expand Up @@ -397,9 +407,7 @@ class MqttClient extends TypedEventEmitter<MqttClientEventCallbacks> {
}

try {
const serverUrl = `${protocol}://${
parsed.hostname || config.host
}:${config.port}`
const serverUrl = `${protocol}://${hostname || config.host}:${config.port}`
logger.info(`Connecting to ${serverUrl}`)

const client = connect(serverUrl, options)
Expand Down
Loading