chore: clear the dependency backlog, and fix a log-injection finding (v0.23.32-beta)
Two pieces of housekeeping that had gone quiet. **A code-scanning alert nobody had looked at.** CodeQL flagged js/log-injection in the preflight handler: a request value interpolated straight into a log line. Log entries are newline-delimited, so a value containing a line break forges additional entries — and an audit trail its own subject can write into is worse than none, because it is believed. The interesting part is that the risk had already been recognised. Two sites in the clients routes strip carriage returns inline, ad hoc, and the third was simply missed. That is what a shared helper is for, so there is now a `logSafe` utility with tests, used at all three. It replaces control characters with a visible marker rather than stripping them, so a tampered value reads as tampered instead of being quietly tidied, and truncates absurd input so one field cannot flood the log. **Twelve Dependabot pull requests, the oldest six weeks old.** Nine had passing checks, but those runs were from July against a main that has moved a long way, so rather than merging stale branches serially the eight safe ones are applied here as one change and verified together: bullmq, pg, helmet, @types/supertest, tailwind-merge, vite, actions/setup-node and github/codeql-action. Deliberately excluded, and left open: * bcryptjs 2.4.3 to 3.0.3 (#79) — a major on the password-hashing path. Dependabot groups it with routine bumps; it is not one, and it deserves its own verification rather than riding along in a batch. * typescript 5.9 to 7.0 (#81), tailwindcss 3.4 to 4.3 (#76) and eslint-plugin-react-hooks (#78) — all three already fail their own checks. Verified beyond the gates: the application was rebuilt and exercised on the new dependencies — authentication, the BullMQ-backed poller, Postgres reads and helmet's headers all behave. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
1ce57a25ec
commit
33ca7cc651
@@ -1,6 +1,7 @@
|
||||
import { Router, Request, Response } from 'express';
|
||||
import * as dgram from 'dgram';
|
||||
import { query } from '../config/database';
|
||||
import { logSafe } from '../utils/logSafe';
|
||||
import { requireAuth, requireAdmin, requireWrite } from '../middleware/auth';
|
||||
import { PollerService } from '../services/PollerService';
|
||||
import { getQueryApi, bucket } from '../config/influxdb';
|
||||
@@ -326,7 +327,7 @@ router.get('/:mac/traffic', async (req: Request, res: Response) => {
|
||||
if (time && field && value != null) raw.push({ time, field, value });
|
||||
});
|
||||
} catch (err) {
|
||||
console.error(`[clients/traffic] Flux error for ${mac.replace(/[\r\n]/g, '_')}:`, err);
|
||||
console.error(`[clients/traffic] Flux error for ${logSafe(mac)}:`, err);
|
||||
}
|
||||
|
||||
// Separate tx and rx series, sort by time, then compute non-negative deltas
|
||||
@@ -384,7 +385,7 @@ router.get('/:mac/signal', async (req: Request, res: Response) => {
|
||||
if (time && value != null) points.push({ time, signal_strength: Math.round(value) });
|
||||
});
|
||||
} catch (err) {
|
||||
console.error(`[clients/signal] Flux error for ${mac.replace(/[\r\n]/g, '_')}:`, err);
|
||||
console.error(`[clients/signal] Flux error for ${logSafe(mac)}:`, err);
|
||||
}
|
||||
|
||||
return res.json(points);
|
||||
|
||||
@@ -19,6 +19,7 @@ import { runConfigHealth } from '../services/changeGuard/configHealth';
|
||||
import { isMultiVlanSpec } from '../utils/vlan';
|
||||
import { redis } from '../config/redis';
|
||||
import { enqueueBulkAddJob, getBulkAddJobState } from '../services/DeviceBulkAddWorker';
|
||||
import { logSafe } from '../utils/logSafe';
|
||||
import { parseBandList, uplinkAnchor, totalBandwidthMhz, type LteBandInfo } from '../utils/lte';
|
||||
|
||||
// Resolve a credential preset id into decrypted credentials. Returns null if
|
||||
@@ -1017,7 +1018,7 @@ async function withGuardedChange<T>(
|
||||
return;
|
||||
}
|
||||
} catch (err) {
|
||||
console.warn(`[preflight] analysis skipped for device ${id}: ${(err as Error).message}`);
|
||||
console.warn(`[preflight] analysis skipped for device ${logSafe(id)}: ${logSafe((err as Error).message)}`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
import { logSafe } from '../logSafe';
|
||||
|
||||
describe('logSafe', () => {
|
||||
it('leaves ordinary values alone', () => {
|
||||
expect(logSafe('ether1')).toBe('ether1');
|
||||
expect(logSafe(42)).toBe('42');
|
||||
expect(logSafe('2GT-NW-100G')).toBe('2GT-NW-100G');
|
||||
});
|
||||
|
||||
it('stops a value forging a second log entry', () => {
|
||||
// The whole point: a log that its subject can write lines into is worse
|
||||
// than no log, because it is believed.
|
||||
const forged = logSafe('7\n[preflight] device 999 verified OK');
|
||||
expect(forged).not.toContain('\n');
|
||||
expect(forged).toContain('[preflight] device 999 verified OK');
|
||||
});
|
||||
|
||||
it('handles carriage returns and other control characters', () => {
|
||||
expect(logSafe('a\r\nb')).not.toMatch(/[\r\n]/);
|
||||
expect(logSafe('a\u0007b\u001bc')).toBe('a\u2423b\u2423c');
|
||||
});
|
||||
|
||||
it('marks what it replaced rather than hiding it', () => {
|
||||
expect(logSafe('a\nb')).toBe('a\u2423b');
|
||||
});
|
||||
|
||||
it('truncates absurd input so one value cannot flood the log', () => {
|
||||
const out = logSafe('x'.repeat(5000));
|
||||
expect(out.length).toBeLessThanOrEqual(201);
|
||||
expect(out.endsWith('…')).toBe(true);
|
||||
});
|
||||
|
||||
it('handles null and undefined without printing them', () => {
|
||||
expect(logSafe(null)).toBe('');
|
||||
expect(logSafe(undefined)).toBe('');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,26 @@
|
||||
/**
|
||||
* Make an untrusted value safe to put in a log line.
|
||||
*
|
||||
* Log entries are newline-delimited, so a value containing a line break can
|
||||
* forge additional entries — an audit trail that can be written into by the
|
||||
* thing it is auditing is worse than no audit trail, because it is believed.
|
||||
*
|
||||
* Two sources matter and neither is under our control: values from HTTP
|
||||
* requests, and strings read off devices. A device identity is attacker-settable
|
||||
* on a compromised switch, and it appears in log lines throughout the collector.
|
||||
*/
|
||||
|
||||
/** Longest a single interpolated value may be before it is truncated. */
|
||||
const MAX_LEN = 200;
|
||||
|
||||
export function logSafe(value: unknown): string {
|
||||
if (value == null) return '';
|
||||
const text = typeof value === 'string' ? value : String(value);
|
||||
|
||||
// CR, LF, other C0 controls and DEL. Replaced rather than stripped so that a
|
||||
// tampered value reads as tampered instead of being silently tidied up.
|
||||
// eslint-disable-next-line no-control-regex
|
||||
const flattened = text.replace(/[\u0000-\u001F\u007F]/g, '\u2423');
|
||||
|
||||
return flattened.length > MAX_LEN ? `${flattened.slice(0, MAX_LEN)}…` : flattened;
|
||||
}
|
||||
Reference in New Issue
Block a user