fix: apply security and correctness review findings across both stacks
Legacy panel: - upstream body reads now carry their own deadline and a 10 MB byte budget; a stalled modem can no longer hang /api/status fan-out forever nor OOM the proxy (request headers alone had the timeout, bodies had none) - add X-Frame-Options DENY / CSP frame-ancestors none / nosniff; the panel (delete-instance and confirmed-write dialogs) is no longer clickjackable - only send a JSON content-type when the API console request has a body, so payload-less dangerous writes stop failing with 400 and burning the one-use confirmation token - register a form-urlencoded parser (the proxy branch was unreachable) and drop the multipart parser that buffered up to 60 MB before rejecting; bodyLimit drops to 2 MB; framework-level 415 keeps the stable error body - remove /api/sms/send from readable paths: GET bypassed the write confirmation for a send endpoint - /api/instances/:id/login maps upstream failures to a stable 502 instead of leaking raw error text - guard MULTI_SIMADMIN_TIMEOUT_MS parsing (NaN aborted every request); prune dead code (buildClients, cookie expando no-op) Control plane: - deleting a notification channel detaches it from rules instead of leaving dangling ids that made every referencing rule unreadable and silently dropped future notifications; rule reads tolerate unknown ids - startup sweep resets notification_queue rows stranded in 'sending' by a crash (mirrors the sms outbox sweep); terminal outbox rows are pruned on the retention timer - /api/v1/metrics no longer emits operator-assigned node names on the session-free scrape; login limiter map is bounded and pruned; Secure cookie honors the gateway-declared x-forwarded-proto - webhook delivery sets redirect: manual (signed payloads are not replayed) - SMTP envelope sender is validated against CR/LF smuggling - scheduled reboots with delaySeconds != 3 fail fast at the dispatcher with a clear reason instead of burning every retry; contract narrowed to the pinned baseline
This commit is contained in:
@@ -114,6 +114,14 @@ export class ScheduledOperationDispatcher {
|
||||
const schema = operationSchemas[task.operationType];
|
||||
if (!operationId || !schema)
|
||||
return { outcome: 'needs-attention', reason: 'Unsupported scheduled operation', jobIds: [] };
|
||||
// The execution baseline pins delay_seconds to 3; anything else could never
|
||||
// succeed, so refuse up front instead of exhausting the retry policy.
|
||||
if (task.operationType === 'reboot-system' && (task.delaySeconds ?? 3) !== 3)
|
||||
return {
|
||||
outcome: 'needs-attention',
|
||||
reason: 'Scheduled reboots only support the pinned 3-second delay',
|
||||
jobIds: [],
|
||||
};
|
||||
const fields =
|
||||
task.operationType === 'reboot-system'
|
||||
? [
|
||||
|
||||
@@ -559,7 +559,28 @@ export class CentralNotificationService {
|
||||
|
||||
async deleteChannel(id: string): Promise<void> {
|
||||
const channel = this.getChannel(id);
|
||||
this.#db.prepare('DELETE FROM notification_channels WHERE id=?').run(id);
|
||||
this.#db.transaction(() => {
|
||||
this.#db.prepare('DELETE FROM notification_channels WHERE id=?').run(id);
|
||||
// Rules keep channel ids in a JSON column, so no FK cascade applies; a
|
||||
// dangling id would make every referencing rule unreadable and drop
|
||||
// future notifications on the floor.
|
||||
const rules = this.#db
|
||||
.prepare('SELECT id, channel_ids_json FROM notification_rules')
|
||||
.all() as Array<{ id: string; channel_ids_json: string }>;
|
||||
for (const rule of rules) {
|
||||
let ids: unknown;
|
||||
try {
|
||||
ids = JSON.parse(String(rule.channel_ids_json));
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (!Array.isArray(ids) || !ids.includes(id)) continue;
|
||||
const next = JSON.stringify(ids.filter((entry) => entry !== id));
|
||||
this.#db
|
||||
.prepare('UPDATE notification_rules SET channel_ids_json=?, updated_at=? WHERE id=?')
|
||||
.run(next, this.#now(), rule.id);
|
||||
}
|
||||
})();
|
||||
if (channel.secretReference)
|
||||
await this.#options.store.delete(channel.secretReference).catch(() => false);
|
||||
}
|
||||
@@ -1202,6 +1223,19 @@ export class CentralNotificationService {
|
||||
return { processed: succeeded + failed, succeeded, failed };
|
||||
}
|
||||
|
||||
/** Startup sweep: rows claimed as 'sending' before a crash would never drain again. */
|
||||
reconcileInterrupted(): number {
|
||||
return Number(
|
||||
this.#db
|
||||
.prepare(
|
||||
`UPDATE notification_queue
|
||||
SET status='pending',updated_at=?
|
||||
WHERE status='sending'`,
|
||||
)
|
||||
.run(this.#now(), ).changes,
|
||||
);
|
||||
}
|
||||
|
||||
retryQueueItem(id: string): void {
|
||||
const result = this.#db
|
||||
.prepare(
|
||||
@@ -1338,7 +1372,12 @@ export class CentralNotificationService {
|
||||
#queueItem(row: Record<string, unknown>): NotificationQueueItem {
|
||||
const status = String(row.status) as NotificationQueueItem['status'];
|
||||
const attempts = Number(row.attempts ?? 0);
|
||||
const payload = record(JSON.parse(String(row.payload_json))) ?? {};
|
||||
let payload: Record<string, unknown> = {};
|
||||
try {
|
||||
payload = record(JSON.parse(String(row.payload_json))) ?? {};
|
||||
} catch {
|
||||
payload = {};
|
||||
}
|
||||
const deliveredAt =
|
||||
typeof row.delivered_at === 'string' && row.delivered_at ? row.delivered_at : undefined;
|
||||
const ruleId = typeof row.rule_id === 'string' && row.rule_id ? row.rule_id : undefined;
|
||||
@@ -1560,7 +1599,12 @@ export class CentralNotificationService {
|
||||
enabled: row.enabled === 1,
|
||||
condition: parseJson(String(row.condition_json), this.#condition.bind(this)),
|
||||
scope: parseJson(String(row.scope_json), this.#scope.bind(this)),
|
||||
channels: Object.freeze(channelIds.map((id) => this.getChannel(id))),
|
||||
channels: Object.freeze(
|
||||
channelIds.flatMap((id) => {
|
||||
const row = this.#row(`SELECT * FROM notification_channels WHERE id = ?`, id);
|
||||
return row ? [this.#channel(row)] : [];
|
||||
}),
|
||||
),
|
||||
templates: parseJson(String(row.templates_json), this.#templates.bind(this)),
|
||||
rateLimit: parseJson(String(row.rate_limit_json), validateRateLimit),
|
||||
quietHours: parseJson(String(row.quiet_hours_json), validateQuietHours),
|
||||
|
||||
@@ -160,6 +160,8 @@ async function defaultRequester(request: HttpRequest, timeoutMs: number): Promis
|
||||
headers: request.headers,
|
||||
...(request.body === undefined ? {} : { body: request.body }),
|
||||
signal: AbortSignal.timeout(timeoutMs),
|
||||
// A signed payload must never be replayed to a redirect target.
|
||||
redirect: 'manual',
|
||||
});
|
||||
const text = (await response.text()).slice(0, MAX_RESPONSE_BYTES);
|
||||
return { status: response.status, body: text };
|
||||
|
||||
@@ -220,6 +220,9 @@ export async function sendEmailNotification(
|
||||
const receivers = splitAddresses(configText(input.config, 'receiver_addresses'));
|
||||
if (!host) return { ok: false, detail: '缺少 SMTP 地址' };
|
||||
if (!sender) return { ok: false, detail: '缺少发件地址' };
|
||||
// Envelope commands are line-framed; reject anything that could break the frame.
|
||||
if (!/^[^\s@<>,;:"']+@[^\s@<>,;:"']+$/u.test(sender))
|
||||
return { ok: false, detail: '发件地址无效' };
|
||||
if (receivers.length === 0) return { ok: false, detail: '缺少有效的收件地址' };
|
||||
const port = configNumber(input.config, 'smtp_port', 465);
|
||||
const security = configText(input.config, 'smtp_security') || 'implicit_tls';
|
||||
|
||||
@@ -80,7 +80,9 @@ describe('MetricsService', () => {
|
||||
const text = metrics.render();
|
||||
expect(text).toContain('multi_simadmin_nodes_total 2');
|
||||
expect(text).toContain('multi_simadmin_node_enabled{node="node-b"} 0');
|
||||
expect(text).toContain('multi_simadmin_node_info{node="node-a",name="机房 A"} 1');
|
||||
expect(text).toContain('multi_simadmin_node_info{node="node-a"} 1');
|
||||
// Operator-assigned names stay out of the session-free exposition entirely.
|
||||
expect(text).not.toContain('机房 A');
|
||||
expect(text).toContain('multi_simadmin_node_probe_total{node="node-a",outcome="success"} 38');
|
||||
expect(text).toContain('multi_simadmin_node_availability_ratio{node="node-a"} 0.950');
|
||||
expect(text).toContain('multi_simadmin_sms_outbox{status="failed"} 1');
|
||||
@@ -108,14 +110,15 @@ describe('MetricsService', () => {
|
||||
expect(text).toContain('multi_simadmin_identity_pending_total 2');
|
||||
});
|
||||
|
||||
it('escapes label values so a node name cannot forge a series', () => {
|
||||
it('never emits a node name, so a hostile name cannot forge a series', () => {
|
||||
const { db: database, metrics } = fixture();
|
||||
seedNode(database, 'node-a', 'evil" name\nwith break');
|
||||
|
||||
const text = metrics.render();
|
||||
expect(text).toContain('name="evil\\" name\\nwith break"');
|
||||
// The injected newline must not become its own sample line.
|
||||
expect(text.split('\n').filter((line) => line.includes('with break'))).toHaveLength(1);
|
||||
// Names are excluded from the session-free exposition outright; escaping a
|
||||
// label is not the right tool for untrusted operator input.
|
||||
expect(text).not.toContain('evil"');
|
||||
expect(text.split('\n').filter((line) => line.includes('with break'))).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('emits a valid exposition: help and type precede every series', () => {
|
||||
|
||||
@@ -77,9 +77,9 @@ export class MetricsService {
|
||||
]),
|
||||
);
|
||||
|
||||
const info = nodes.map(
|
||||
(node) => `${PREFIX}_node_info{node="${label(node.id)}",name="${label(node.name)}"} 1`,
|
||||
);
|
||||
// Operator-assigned names can leak hostnames/locations to unauthenticated
|
||||
// scrapers; the series intentionally stays opaque to node ids.
|
||||
const info = nodes.map((node) => `${PREFIX}_node_info{node="${label(node.id)}"} 1`);
|
||||
lines.push(
|
||||
...sample(`${PREFIX}_node_info`, 'Registered nodes and their addresses.', 'gauge', info),
|
||||
...sample(`${PREFIX}_nodes_total`, 'Number of registered nodes.', 'gauge', [
|
||||
|
||||
@@ -107,6 +107,9 @@ function trustedUpstreamHeaders(
|
||||
delete sanitized[GATEWAY_CLIENT_IP_HEADER];
|
||||
if (gatewayToken !== undefined) sanitized[GATEWAY_AUTH_HEADER] = gatewayToken;
|
||||
if (clientIp !== undefined) sanitized[GATEWAY_CLIENT_IP_HEADER] = clientIp;
|
||||
// The browser connection's protocol is the gateway's to declare; the API uses
|
||||
// it to decide whether session cookies may carry the Secure flag.
|
||||
sanitized['x-forwarded-proto'] = (headers['x-forwarded-proto'] as string | undefined) ?? 'http';
|
||||
return sanitized;
|
||||
}
|
||||
|
||||
|
||||
@@ -320,6 +320,7 @@ export function buildControlPlaneApp(options: ControlPlaneOptions): ControlPlane
|
||||
...(options.now ? { now: options.now } : {}),
|
||||
});
|
||||
smsOutbox.reconcileInterrupted();
|
||||
centralNotifications.reconcileInterrupted();
|
||||
// Removal follows the hub rule the operator already knows: an online device is told to release
|
||||
// its own binding first and a refusal keeps the record, while a device that cannot be reached
|
||||
// at all is forgotten locally. The closure is late-bound because the device action service is
|
||||
@@ -581,6 +582,10 @@ export function buildControlPlaneApp(options: ControlPlaneOptions): ControlPlane
|
||||
void Promise.resolve()
|
||||
.then(() => centralNotifications.pruneLogs())
|
||||
.catch(() => undefined);
|
||||
// Delivered and abandoned outbox rows otherwise accumulate forever.
|
||||
void Promise.resolve()
|
||||
.then(() => smsOutbox.prune(30 * 24 * 60 * 60 * 1000))
|
||||
.catch(() => undefined);
|
||||
}, logPruneIntervalMs)
|
||||
: undefined;
|
||||
// Scheduled component backups: the tick is cheap and only writes when a period is overdue.
|
||||
|
||||
@@ -33,7 +33,8 @@ const cookie = (token: string, secure: boolean): string =>
|
||||
`${CONSOLE_SESSION_COOKIE}=${token}; Path=${COOKIE_PATH}; HttpOnly; SameSite=Strict${secure ? '; Secure' : ''}; Max-Age=${MAX_AGE_SECONDS}`;
|
||||
const clearCookie = (secure: boolean): string =>
|
||||
`${CONSOLE_SESSION_COOKIE}=; Path=${COOKIE_PATH}; HttpOnly; SameSite=Strict${secure ? '; Secure' : ''}; Max-Age=0`;
|
||||
const usesSecureCookie = (request: FastifyRequest): boolean => request.protocol === 'https';
|
||||
const usesSecureCookie = (request: FastifyRequest): boolean =>
|
||||
request.protocol === 'https' || request.headers['x-forwarded-proto'] === 'https';
|
||||
|
||||
const problem = (request: FastifyRequest, status: number, code: string, detail: string) => ({
|
||||
type: 'about:blank',
|
||||
@@ -83,8 +84,26 @@ const loginSchema = {
|
||||
properties: { password: { type: 'string', minLength: 1, maxLength: 1024 } },
|
||||
} as const;
|
||||
|
||||
const LOGIN_BUCKETS_MAX = 10_000;
|
||||
|
||||
export function registerConsoleAuth(app: FastifyInstance, auth: ConsoleAuthService): void {
|
||||
const loginAttempts = new Map<string, { count: number; resetAt: number }>();
|
||||
const rememberAttempt = (key: string, attempt: { count: number; resetAt: number }): void => {
|
||||
// Direct-to-API callers can send arbitrary client-ip values; cap the map so
|
||||
// bucket keys cannot grow without bound.
|
||||
if (loginAttempts.size >= LOGIN_BUCKETS_MAX) {
|
||||
const now = Date.now();
|
||||
for (const [existing, value] of loginAttempts) {
|
||||
if (value.resetAt <= now) loginAttempts.delete(existing);
|
||||
}
|
||||
while (loginAttempts.size >= LOGIN_BUCKETS_MAX) {
|
||||
const oldest = loginAttempts.keys().next().value;
|
||||
if (oldest === undefined) break;
|
||||
loginAttempts.delete(oldest);
|
||||
}
|
||||
}
|
||||
loginAttempts.set(key, attempt);
|
||||
};
|
||||
app.addHook('preHandler', async (request, reply) => {
|
||||
const pathname = request.url.split(/[?#]/u, 1)[0] ?? request.url;
|
||||
if (!auth.isProtectedPath(pathname)) return;
|
||||
@@ -121,7 +140,7 @@ export function registerConsoleAuth(app: FastifyInstance, auth: ConsoleAuthServi
|
||||
attempts && attempts.resetAt > now
|
||||
? attempts
|
||||
: { count: 0, resetAt: now + LOGIN_WINDOW_MS };
|
||||
loginAttempts.set(key, { count: active.count + 1, resetAt: active.resetAt });
|
||||
rememberAttempt(key, { count: active.count + 1, resetAt: active.resetAt });
|
||||
return handle(request, reply, error);
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user