Feat/stage

[AdriGeorge]

v2.0.2

In general, to fix incomplete escaping you must escape all characters that are significant in the target context, and ensure that backslashes themselves are escaped before escaping other characters. For HTTP header parameter values like filename="...", that means first escaping backslashes, then double quotes, so that sequences like \" in the original filename cannot be misinterpreted when the header is parsed.

The best minimal change here is to adjust the filename.replace(/"/g, '\\"') call on line 114 so that it also escapes backslashes. We can do this with a single replace using a character class and a replacement function that prefixes any backslash or double quote with a backslash: filename.replace(/["\\]/g, '\\$&'). This ensures that any existing backslash is turned into \\, and any " is turned into \", covering the case CodeQL complains about without changing the surrounding logic or behavior beyond making header values safer.

Concretely, in src/components/storage/S3Storage.ts, at the ContentDisposition property inside the upload method (around lines 107–115), replace the current interpolation expression filename.replace(/"/g, '\\"') with filename.replace(/["\\]/g, '\\$&'). No additional imports or helper functions are required.

In general, to fix incomplete escaping you should ensure that all characters that act as escape/meta characters in the target context are handled consistently. When you already escape a quote character using backslashes, you must also escape any existing backslashes first, otherwise an input containing backslashes followed by quotes can defeat or confuse the escaping.

Here, the header is built as:

'Content-Disposition': `attachment; filename="${filename.replace(/"/g, '\\"')}"`

This replaces only " with \". To avoid inconsistent escaping, we should first escape backslashes, then escape quotes, e.g.:

const safeFilename = filename.replace(/\\/g, '\\\\').replace(/"/g, '\\"')
'Content-Disposition': `attachment; filename="${safeFilename}"`

This change is confined to src/components/storage/UrlStorage.ts, inside the upload method, around line 58. No new methods or imports are needed; we can implement the escaping inline using standard String.prototype.replace with appropriate regular expressions. Functionality remains the same for filenames without backslashes; for filenames with backslashes, the header becomes more robust and correctly escaped.

In general terms, the problem is that detailed exception messages, which can include user-influenced content, are being returned directly to the client and interpreted as HTML. The best fix is to avoid sending raw exception messages in HTTP responses; instead, send a generic, safe error message to the client and log the detailed error message server-side. This removes any user-controlled content from the response and avoids XSS, while preserving debuggability through logs.

For this specific code, the key change is in src/components/httpRoutes/commands.ts in the catch block of the /directCommand route. Instead of:

137:       HTTP_LOGGER.error(err.message)
138:       closedResponse = true
139:       if (!res.headersSent) {
140:         res.status(500).send(err.message)
141:       }

we should:

1. Log the full error (ideally the whole err object) via HTTP_LOGGER.error.
2. Return a generic error string or JSON that does not expose err.message to the client, such as 'Internal server error' or a similar safe message.

To preserve existing behavior shape (string response, 500 status) while making it safe, we can replace err.message in the send call with a constant like 'Internal server error'. We should also guard for non-Error types safely when logging, but we can do that without changing imports or external behavior: e.g., log err as-is or fall back to String(err).

No other locations in these snippets send err.message to clients; the other flows (e.g., via validateCommandParameters and buildInvalidParametersResponse) already use structured validation messages we control. The single change in the catch block addresses all alert variants, because they all target this same res.status(500).send(err.message) sink.