fix(audit-log): neutralize CSV export injection, clamp pagination, bound anomaly history (#1259)

* fix(audit-log): neutralize CSV export injection, clamp pagination, bound anomaly history

Harden the Admiral audit log without changing its tier or hub-only gating.

- CSV export now defuses formula injection: any field that a spreadsheet
  would evaluate as a formula (leading = + - @, or a trigger behind leading
  whitespace, or a leading tab/CR) is prefixed with a single quote before
  RFC 4180 quoting. Audit summaries embed user-controlled resource names, so
  this closes a path where a crafted name could execute on export open.
- Clamp page and limit to positive bounds on the list endpoint so a negative
  limit can no longer reach SQLite as "unlimited" and dump the whole table.
- Bound the anomaly and stats history reads to a capped slice of recent rows
  so the analysis paths stay within fixed memory and latency on large
  histories instead of scanning the full retention window per request.
- Surface failed audit list and stats fetches through the standard error
  toast instead of leaving the view silently stale.
- Add a developer-mode-gated diagnostic log to the stats endpoint for parity
  with the list and export handlers.

Covered by new unit and HTTP-integration tests (CSV neutralization through
the real export route, pagination clamps, bounded history, stats endpoint,
anomaly annotation) and verified end to end in the browser.

* fix(audit-log): compute signal-rail stats with exact SQL aggregates

Address review feedback on the earlier history-cap change. The cap was
correct for the anomaly baseline but made the stats tiles (events, actors,
failure rate, hourly series) silently undercount on a hub with more than the
cap's worth of rows in the window, since they were derived from the capped
row slice.

- Add DatabaseService.getAuditStatsInputs: exact counts via SQL COUNT /
  COUNT(DISTINCT) / GROUP BY hour, and new-ip detection over the small
  DISTINCT (user, ip) pair sets. No row cap, so the tiles stay exact at any
  window size while memory stays bounded.
- Reduce computeAuditStats to a pure formatter over those aggregates.
- Keep the bounded history read only for the list endpoint's anomaly
  annotation, where a recent-activity baseline is an acceptable heuristic.
- Skip the redundant load-failure toast when a fetch fails with a handled
  401, so an expired session does not stack toasts on top of logout.

Adds exactness tests for the aggregate counts, distinct-actor handling, and
new-ip detection, and strengthens the pagination-clamp tests.

* fix(audit-log): exclude future-dated rows and make the new-ip sample deterministic

Two small parity fixes on the stats aggregates: upper-bound every current
window by `now` so a future-dated row (clock skew or a fixture) cannot inflate
the live counts, and order the new-ip pair scan so the sample actor shown in
the tile detail is stable. Adds a test asserting a future row is excluded.
This commit is contained in:
Anso
2026-05-31 16:36:07 -04:00
committed by GitHub
parent dbb7fe8215
commit 5e66b54153
9 changed files with 553 additions and 98 deletions
@@ -113,57 +113,65 @@ describe('AuditAnomalyService - annotateEntries', () => {
});
describe('AuditAnomalyService - computeAuditStats', () => {
function buildEntries(now: number): { last24h: AuditLogEntry[]; last7d: AuditLogEntry[]; last30d: AuditLogEntry[] } {
const last24h: AuditLogEntry[] = [];
for (let i = 0; i < 20; i++) {
last24h.push(entry({
id: i + 1,
timestamp: now - i * HOUR,
status_code: i < 3 ? 500 : 200,
ip_address: i === 5 ? '45.76.1.2' : '10.0.0.1',
}));
}
const older: AuditLogEntry[] = [];
for (let i = 0; i < 60; i++) {
older.push(entry({
id: 100 + i,
timestamp: now - DAY - i * HOUR,
ip_address: '10.0.0.1',
}));
}
return {
last24h,
last7d: [...last24h, ...older.filter(e => e.timestamp >= now - 7 * DAY)],
last30d: [...last24h, ...older],
};
}
const baseInput = (over: Partial<Parameters<typeof computeAuditStats>[0]> = {}) => ({
events24: 0,
events7d: 0,
actors24: 0,
failures24: 0,
activityByHour: Array.from({ length: 24 }, () => 0),
failuresByHour: Array.from({ length: 24 }, () => 0),
newIpCount: 0,
sampleNewIpActor: null as string | null,
...over,
});
it('summarizes events, actors, failures, and peak hour', () => {
const now = new Date('2026-04-18T12:00:00Z').getTime();
const { last24h, last7d, last30d } = buildEntries(now);
const stats = computeAuditStats({ now, last24h, last7d, last30d });
it('summarizes events, actors, and failure rate from exact aggregates', () => {
const activityByHour = Array.from({ length: 24 }, () => 0);
activityByHour[12] = 20;
const stats = computeAuditStats(baseInput({
events24: 20,
events7d: 20,
actors24: 1,
failures24: 3,
activityByHour,
}));
expect(stats.events_24h.value).toBe(20);
expect(stats.actors_24h.value).toBe(1);
expect(stats.failure_rate.value).toBe(15);
expect(stats.failure_rate.detail).toBe('3 of 20 requests');
expect(stats.activity_by_hour).toHaveLength(24);
expect(stats.activity_by_hour.reduce((a, b) => a + b, 0)).toBe(20);
});
it('flags the new_ip detail when an actor uses an ip not seen in prior 29 days', () => {
const now = new Date('2026-04-18T12:00:00Z').getTime();
const { last24h, last7d, last30d } = buildEntries(now);
const stats = computeAuditStats({ now, last24h, last7d, last30d });
expect(stats.actors_24h.detail).toMatch(/new ip/);
it('flags the new_ip detail when the aggregate reports a new pair', () => {
const stats = computeAuditStats(baseInput({
events24: 5,
events7d: 5,
actors24: 1,
newIpCount: 2,
sampleNewIpActor: 'admin',
}));
expect(stats.actors_24h.detail).toMatch(/2 new ips/);
expect(stats.actors_24h.detail).toMatch(/admin/);
expect(stats.actors_24h.severity).toBe('warn');
});
it('surfaces peak hour when it falls outside working hours', () => {
const now = new Date(2026, 3, 18, 12, 0, 0).getTime();
const nightBase = new Date(2026, 3, 18, 3, 15, 0).getTime();
const nightEntries: AuditLogEntry[] = Array.from({ length: 10 }, (_, i) =>
entry({ id: i + 1, timestamp: nightBase - i * 5000 })
);
const stats = computeAuditStats({ now, last24h: nightEntries, last7d: nightEntries, last30d: nightEntries });
const activityByHour = Array.from({ length: 24 }, () => 0);
activityByHour[3] = 10;
const stats = computeAuditStats(baseInput({
events24: 10,
events7d: 10,
actors24: 1,
activityByHour,
}));
expect(stats.unusual_hour.severity).toBe('warn');
expect(stats.unusual_hour.value).toBe(3);
});
it('keeps peak hour blank inside working hours', () => {
const activityByHour = Array.from({ length: 24 }, () => 0);
activityByHour[14] = 8;
const stats = computeAuditStats(baseInput({ events24: 8, events7d: 8, actors24: 1, activityByHour }));
expect(stats.unusual_hour.value).toBeNull();
});
});
+284
View File
@@ -260,6 +260,142 @@ describe('DatabaseService audit methods', () => {
const page2Ids = page2.entries.map(e => e.id);
expect(page1Ids.some(id => page2Ids.includes(id))).toBe(false);
});
it('getAuditLogsInRange caps to the most-recent rows and returns ascending order', () => {
const db = DatabaseService.getInstance();
// Old, isolated window: keeps these rows out of the "most recent" DESC
// queries other tests rely on, while staying easy to range-query here.
const base = 1_000_000_000_000; // 2001, far from any now()-based entry
for (let i = 0; i < 10; i++) {
db.insertAuditLog({
timestamp: base + i,
username: 'rangecapuser',
method: 'POST',
path: `/api/stacks/cap${i}`,
status_code: 200,
node_id: null,
ip_address: '127.0.0.1',
summary: `cap entry ${i}`,
});
}
const capped = db.getAuditLogsInRange(base, base + 100, 3);
expect(capped.length).toBe(3);
// Most-recent three (timestamps base+7, +8, +9), returned ascending.
expect(capped.map(e => e.timestamp)).toEqual([base + 7, base + 8, base + 9]);
const uncapped = db.getAuditLogsInRange(base, base + 100);
expect(uncapped.length).toBe(10);
expect(uncapped[0].timestamp).toBe(base);
});
it('getAuditStatsInputs counts the current window exactly (no row cap)', () => {
const db = DatabaseService.getInstance();
const now = Date.now();
const ts = now - 60 * 60 * 1000; // 1 hour ago, inside the 24h window
const hour = new Date(ts).getHours();
const before = db.getAuditStatsInputs(now);
const K = 12;
const FAILURES = 4;
for (let i = 0; i < K; i++) {
db.insertAuditLog({
timestamp: ts,
username: 'statsexactuser',
method: 'POST',
path: `/api/stacks/statsexact${i}`,
status_code: i < FAILURES ? 500 : 200,
node_id: null,
ip_address: '127.0.0.1',
summary: `stats exact ${i}`,
});
}
const after = db.getAuditStatsInputs(now);
expect(after.events24 - before.events24).toBe(K);
expect(after.events7d - before.events7d).toBe(K);
expect(after.failures24 - before.failures24).toBe(FAILURES);
expect(after.activityByHour[hour] - before.activityByHour[hour]).toBe(K);
expect(after.failuresByHour[hour] - before.failuresByHour[hour]).toBe(FAILURES);
});
it('getAuditStatsInputs excludes future-dated rows from the current window', () => {
const db = DatabaseService.getInstance();
const now = Date.now();
const before = db.getAuditStatsInputs(now);
// A row dated after `now` (clock skew / fixture) must not inflate the live counts.
db.insertAuditLog({
timestamp: now + 60 * 60 * 1000,
username: 'futureuser',
method: 'POST',
path: '/api/stacks/future',
status_code: 500,
node_id: null,
ip_address: '127.0.0.1',
summary: 'future entry',
});
const after = db.getAuditStatsInputs(now);
expect(after.events24 - before.events24).toBe(0);
expect(after.events7d - before.events7d).toBe(0);
expect(after.failures24 - before.failures24).toBe(0);
});
it('getAuditStatsInputs flags an actor whose recent ip is new versus prior history', () => {
const db = DatabaseService.getInstance();
const now = Date.now();
// Prior IP for this actor, older than 24h but inside 30d.
db.insertAuditLog({
timestamp: now - 5 * 24 * 60 * 60 * 1000,
username: 'newipscenariouser',
method: 'POST',
path: '/api/stacks/old',
status_code: 200,
node_id: null,
ip_address: '10.0.0.1',
summary: 'old',
});
const before = db.getAuditStatsInputs(now);
// Recent action from a different IP.
db.insertAuditLog({
timestamp: now - 60 * 1000,
username: 'newipscenariouser',
method: 'POST',
path: '/api/stacks/new',
status_code: 200,
node_id: null,
ip_address: '203.0.113.9',
summary: 'new',
});
const after = db.getAuditStatsInputs(now);
expect(after.newIpCount).toBeGreaterThan(before.newIpCount);
});
it('getAuditStatsInputs counts distinct non-empty actors, excluding blank usernames', () => {
const db = DatabaseService.getInstance();
const now = Date.now();
const ts = now - 30 * 60 * 1000; // inside 24h
const before = db.getAuditStatsInputs(now);
const usernames = ['actorcountA', 'actorcountB', 'actorcountB', 'actorcountC', ''];
for (const username of usernames) {
db.insertAuditLog({
timestamp: ts,
username,
method: 'POST',
path: '/api/stacks/actorcount',
status_code: 200,
node_id: null,
ip_address: '127.0.0.1',
summary: 'actor count',
});
}
const after = db.getAuditStatsInputs(now);
// Three distinct non-empty actors (A, B, C); the blank username is excluded.
expect(after.actors24 - before.actors24).toBe(3);
});
});
// ---- API endpoint tests ----
@@ -318,6 +454,131 @@ describe('GET /api/audit-log', () => {
expect(res.status).toBe(200);
expect(res.body.entries.length).toBeGreaterThanOrEqual(1);
});
it('clamps a negative limit to a single row instead of returning the whole table', async () => {
// A negative LIMIT reaches SQLite as "unlimited" without the clamp.
const res = await request(app)
.get('/api/audit-log?limit=-1')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
expect(res.body.entries.length).toBeLessThanOrEqual(1);
expect(res.body.total).toBeGreaterThan(1);
});
it('clamps an oversized limit to the 200 cap even when more rows match', async () => {
const db = DatabaseService.getInstance();
for (let i = 0; i < 205; i++) {
db.insertAuditLog({
timestamp: Date.now() - i,
username: 'limitcapuser205',
method: 'POST',
path: `/api/stacks/limitcap${i}`,
status_code: 200,
node_id: null,
ip_address: '127.0.0.1',
summary: `limit cap entry ${i}`,
});
}
const res = await request(app)
.get('/api/audit-log?limit=99999&search=limitcapuser205')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
expect(res.body.total).toBe(205);
expect(res.body.entries.length).toBe(200);
});
it('clamps a non-positive page to page 1', async () => {
const negative = await request(app)
.get('/api/audit-log?page=-5&limit=5')
.set('Authorization', `Bearer ${adminToken()}`);
const first = await request(app)
.get('/api/audit-log?page=1&limit=5')
.set('Authorization', `Bearer ${adminToken()}`);
expect(negative.status).toBe(200);
expect(negative.body.entries.length).toBeGreaterThan(0);
// page=-5 must resolve to the same first page, not a negative offset.
expect(negative.body.entries[0].id).toBe(first.body.entries[0].id);
});
it('annotates entries with a flags array when with_anomalies=1', async () => {
const res = await request(app)
.get('/api/audit-log?with_anomalies=1&limit=5')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
expect(res.body.entries.length).toBeGreaterThan(0);
for (const entry of res.body.entries) {
expect(Array.isArray(entry.flags)).toBe(true);
}
});
it('flags a never-before-seen actor as first_seen_actor', async () => {
const db = DatabaseService.getInstance();
db.insertAuditLog({
timestamp: Date.now(),
username: 'brandnewactor_unique',
method: 'POST',
path: '/api/stacks/firstseen',
status_code: 200,
node_id: null,
ip_address: '127.0.0.1',
summary: 'Deployed stack: firstseen',
});
const res = await request(app)
.get('/api/audit-log?with_anomalies=1&search=brandnewactor_unique')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
const entry = res.body.entries.find(
(e: { username: string }) => e.username === 'brandnewactor_unique',
);
expect(entry).toBeDefined();
expect(entry.flags).toContain('first_seen_actor');
});
it('treats limit=0 as the default page size, not zero rows', async () => {
// parseInt('0') is falsy, so the `|| 50` default applies before the clamp.
const res = await request(app)
.get('/api/audit-log?limit=0')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
expect(res.body.entries.length).toBeGreaterThan(0);
expect(res.body.entries.length).toBeLessThanOrEqual(50);
});
});
describe('GET /api/audit-log/stats', () => {
it('returns 403 without Admiral license', async () => {
const { LicenseService } = await import('../services/LicenseService');
vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValueOnce('community');
vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValueOnce(null);
const res = await request(app)
.get('/api/audit-log/stats')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(403);
});
it('returns the four-tile stat structure for admin', async () => {
const res = await request(app)
.get('/api/audit-log/stats')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
expect(res.body).toHaveProperty('events_24h');
expect(res.body).toHaveProperty('actors_24h');
expect(res.body).toHaveProperty('failure_rate');
expect(res.body).toHaveProperty('unusual_hour');
expect(Array.isArray(res.body.activity_by_hour)).toBe(true);
expect(res.body.activity_by_hour.length).toBe(24);
expect(res.body.failures_by_hour.length).toBe(24);
});
});
describe('GET /api/audit-log/export', () => {
@@ -368,6 +629,29 @@ describe('GET /api/audit-log/export', () => {
expect(entry.method).toBe('DELETE');
}
});
it('neutralizes a formula-injection payload in the CSV export', async () => {
const db = DatabaseService.getInstance();
db.insertAuditLog({
timestamp: Date.now(),
username: 'csvinjectuser',
method: 'POST',
path: '/api/stacks/csvinject',
status_code: 200,
node_id: null,
ip_address: '127.0.0.1',
summary: '=DANGER_FORMULA',
});
const res = await request(app)
.get('/api/audit-log/export?format=csv&search=csvinjectuser')
.set('Authorization', `Bearer ${adminToken()}`);
expect(res.status).toBe(200);
// The leading '=' must be defused with a single-quote prefix.
expect(res.text).toContain("'=DANGER_FORMULA");
expect(res.text).not.toMatch(/(^|,)=DANGER_FORMULA/);
});
});
// ---- Audit middleware integration ----
+66
View File
@@ -0,0 +1,66 @@
/**
* Tests for escapeCsvField: RFC 4180 quoting plus CSV formula-injection
* neutralization (CWE-1236). Audit-log export embeds user-controlled resource
* names, so any field can carry a leading formula trigger.
*/
import { describe, it, expect } from 'vitest';
import { escapeCsvField } from '../utils/csv';
describe('escapeCsvField', () => {
it('returns empty string for null and undefined', () => {
expect(escapeCsvField(null)).toBe('');
expect(escapeCsvField(undefined)).toBe('');
});
it('passes benign values through unchanged', () => {
expect(escapeCsvField('Created stack: web')).toBe('Created stack: web');
expect(escapeCsvField('admin')).toBe('admin');
expect(escapeCsvField(201)).toBe('201');
expect(escapeCsvField(0)).toBe('0');
});
it('quotes and doubles embedded quotes / commas / newlines', () => {
expect(escapeCsvField('a,b')).toBe('"a,b"');
expect(escapeCsvField('say "hi"')).toBe('"say ""hi"""');
expect(escapeCsvField('line1\nline2')).toBe('"line1\nline2"');
});
it('prefixes a single quote on each formula trigger', () => {
expect(escapeCsvField('=1+1')).toBe("'=1+1");
expect(escapeCsvField('+1')).toBe("'+1");
expect(escapeCsvField('-1')).toBe("'-1");
expect(escapeCsvField('@SUM(A1)')).toBe("'@SUM(A1)");
expect(escapeCsvField('\tTAB')).toBe("'\tTAB");
// A leading CR is both defused and RFC 4180 quoted.
expect(escapeCsvField('\rCR')).toBe('"\'\rCR"');
});
it('defuses a trigger hidden behind leading whitespace', () => {
expect(escapeCsvField(' =cmd')).toBe("' =cmd");
expect(escapeCsvField(' +1')).toBe("' +1");
expect(escapeCsvField('\t=cmd')).toBe("'\t=cmd");
});
it('prefixes a negative number (conservative, by design)', () => {
expect(escapeCsvField(-5)).toBe("'-5");
});
it('neutralizes a HYPERLINK formula payload in a resource name', () => {
// Embedded quotes + commas force RFC 4180 quoting, so the cell is wrapped;
// the defused content (leading single quote) sits just inside the wrapper.
const malicious = '=HYPERLINK("http://evil","click")';
const out = escapeCsvField(malicious);
expect(out).toContain("'=HYPERLINK");
expect(out.startsWith('=')).toBe(false);
});
it('combines formula prefix with RFC 4180 quoting when needed', () => {
// Leading trigger AND an embedded comma: prefix first, then quote.
expect(escapeCsvField('=cmd,inject')).toBe('"\'=cmd,inject"');
});
it('does not alter a value where the trigger is not first', () => {
expect(escapeCsvField('stack=web')).toBe('stack=web');
expect(escapeCsvField('a-b-c')).toBe('a-b-c');
});
});