diff --git a/CHANGELOG.md b/CHANGELOG.md index 59391d63..0e42d164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ Two correctness follow-ups from the multi-engine bug-hunt sweep (issue #255): Si ### Security - **Cross-tenant read on the dashboard API endpoints (fixed)**: the five dashboard endpoints (`/api/v1/dashboard/stats`, `/timeseries`, `/top-services`, `/timeline-events`, `/recent-errors`) plus the newer `/activity-overview` took `organizationId` from the query string and only ran the organization-membership check behind `if (request.user?.id)`, which is set for session auth only. For API-key auth `requireFullAccess` lets any non-write key through without setting `request.user`, so the membership check was skipped and the org was read from the attacker-supplied query rather than the key's bound `request.organizationId`; when `projectId` was omitted the project-in-org check was skipped too. A holder of any full-access API key (bound to org A) could read another organization's dashboard data by passing that org's id. All six handlers now route through a shared `resolveDashboardScope` that, for API-key auth, requires the requested org to match the key's bound org and the requested project to match (defaulting to the key's bound project when omitted), mirroring `resolveQueryProjectId` which already protects the query and traces routes. Session auth keeps the org-membership and project-in-org checks. Reported privately via KIberblick.de - **Stored XSS via OTLP `service.name` in the service map (fixed)**: `service.name` from ingested traces passed through `sanitizeForPostgres`, which only strips null bytes, so `< > " '` survived into `span.service_name` and were served verbatim by the service-map API. In `ServiceMap.svelte` the ECharts `tooltip.formatter` returned a raw HTML string built from `params.name` / `params.data.source` / `params.data.target`, and ECharts renders tooltip output as HTML, so a `service.name` like `` executed in the browser of any operator who opened the project's service map and hovered the node or edge. User-derived tooltip values are now HTML-escaped via a shared `escapeHtml` util (also adopted by the SIEM HTML report builder, replacing its private copy). Stored data is left raw on purpose (escaping at the sink, not the store, avoids double-encoding and keeps the JSON API correct). Reported privately via KIberblick.de +- **Open redirect on auth-free login/register (fixed)**: in `authMode === 'none'` deployments the login and register pages forwarded the user-supplied `redirect` query parameter via `goto(redirectUrl)` with no validation, while the normal submit path already checked it. The check now lives in a shared `isSafeInternalPath`/`safeRedirect` helper used by both the auth-free and normal paths on both pages; it requires a single-leading-slash path and rejects protocol-relative forms including the backslash variant (`/\evil.com`) that some browsers normalize to `//`. Reported privately via KIberblick.de +- **SSRF guard now pins the validated IP (DNS rebinding hardening)**: `safeFetch` resolved and validated the target host, then let `fetch()` re-resolve at connect time, leaving a resolve-then-connect window where a hostname could rebind to an internal address between check and connect (the TCP monitor path already pinned, the HTTP path did not). The HTTP(S) path now connects through a per-request undici dispatcher whose lookup is pinned to the already-validated address, so the socket reaches the exact IP that passed validation; TLS SNI and certificate validation still use the original hostname. Reported privately via KIberblick.de +- **First-admin bootstrap race (fixed)**: `createUser` decided the automatic first-admin promotion with a non-atomic `hasAnyAdmin()` check followed by a separate insert, so concurrent registrations in the zero-admin window could each observe "no admin yet" and all be promoted. The check-then-insert now runs inside a transaction holding a Postgres advisory lock, so at most one registration wins the promotion. Only reachable before the first admin exists (and closed entirely when `INITIAL_ADMIN_*` is set). Reported privately via KIberblick.de +- **Capability limit check-then-act race (fixed)**: resource-creating routes (api keys, custom dashboards, alert rules, sigma rule import/enable, notification channels) ran `COUNT -> assertWithinLimit -> insert` without serialization, so parallel requests could each read a count under the limit and then all insert, exceeding a configured finite cap (a quota bypass, not a tenant boundary; the OSS default has no finite limits). The count+create now runs through a shared `withLimitLock` helper that takes a per-organization, per-capability transaction-scoped advisory lock, so concurrent creators of the same resource type serialize and the cap holds. Reported privately via KIberblick.de +- **Defense-in-depth on OTLP `service.name` at ingestion**: complementing the service-map output-encoding fix above, ingested `service.name` (for logs, spans and metrics) is now run through a shared `sanitizeServiceName` that strips control characters (C0/DEL/C1, including null bytes) and caps the length, while deliberately preserving otherwise legitimate characters (escaping still happens at each sink). This keeps a raw payload from resurfacing through a sink that is added later or forgets to encode. Suggested by KIberblick.de - **PII masking now also covers trace span attributes**: masking was wired only into log ingestion, so trace spans were stored with their attributes verbatim. Spans routinely carry `http.request_body` / `http.response_body` (plaintext credentials, JWTs), `net.peer.ip` and user agents, all persisted unmasked. `tracesService.ingestSpans` now runs the same org/project masking rules over each span's `attributes`, `resourceAttributes` and event/link attributes before storage, and drops (fail-closed) any span whose masking throws. Because request/response bodies are opaque stringified JSON that field-name rules can't see into, those body attributes are deep-masked (parse the JSON, mask `password`/`token`/email inside, re-serialize) with full redaction as a fallback when the value is not parseable JSON. Metric attributes are not yet masked (tracked separately) ### Added diff --git a/SECURITY.md b/SECURITY.md index 853e8e4d..e34a6b99 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -37,6 +37,7 @@ We thank the following researchers for responsibly disclosing security issues: - **Bertie** — cross-tenant authorization gaps in project-scoped routes (alert preview/creation, monitor creation, source map list/delete) and SSRF / internal port-scanning via HTTP/TCP monitors and webhook delivery. Fixed in 0.9.6. - **tonghuaroot** — SSRF in the alert/Sigma webhook delivery path, which still used the bypassable inline filter instead of the centralized `safeFetch` guard (incomplete-fix sibling-gap of the 0.9.6 hardening). Fixed in 0.9.7. (GHSA-7v53-pw6r-99vj) +- **KIberblick.de** ([kiberblick.de](https://kiberblick.de)) — cross-tenant read on the dashboard API endpoints (organization taken from the attacker-supplied query string under API-key auth) and stored XSS via OTLP `service.name` in the service map; plus an open redirect on the auth-free login/register path, a DNS-rebinding gap in the SSRF guard's HTTP path, a first-admin bootstrap promotion race, and a capability-limit check-then-act race. Fixed in 1.0.3. ## Supported Versions diff --git a/packages/backend/package.json b/packages/backend/package.json index 05198cc3..df3b8e9f 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -70,6 +70,7 @@ "safe-regex2": "^5.0.0", "source-map": "^0.7.6", "tsx": "^4.21.0", + "undici": "^7.28.0", "zod": "^3.25.76" }, "devDependencies": { diff --git a/packages/backend/src/capabilities/index.ts b/packages/backend/src/capabilities/index.ts index cc6ed7f5..455d1468 100644 --- a/packages/backend/src/capabilities/index.ts +++ b/packages/backend/src/capabilities/index.ts @@ -32,3 +32,4 @@ export { export { quotaFlagCache } from './quota-cache.js'; export { QuotaEvaluator } from './quota-evaluator.js'; +export { withLimitLock } from './limit-lock.js'; diff --git a/packages/backend/src/capabilities/limit-lock.ts b/packages/backend/src/capabilities/limit-lock.ts new file mode 100644 index 00000000..279cbcf4 --- /dev/null +++ b/packages/backend/src/capabilities/limit-lock.ts @@ -0,0 +1,73 @@ +import { sql } from 'kysely'; +import { db } from '../database/connection.js'; + +// Namespace for the (int4, int4) Postgres advisory lock keyspace used by +// capability limit enforcement, kept distinct from other advisory locks. +const CAP_LOCK_NAMESPACE = 0x4c54; // 'LT' + +/** Deterministic 32-bit signed hash (FNV-1a) for advisory lock keys. */ +function hash32(input: string): number { + let h = 2166136261; + for (let i = 0; i < input.length; i++) { + h ^= input.charCodeAt(i); + h = Math.imul(h, 16777619); + } + return h | 0; // coerce to signed int32 for pg_advisory_xact_lock(int4, int4) +} + +// In-process serialization tails, keyed by org+capability. This bounds the +// number of callers that block on the DB advisory lock to ONE per key per +// process, so concurrent requests don't each hold a transaction connection +// while waiting (which would exhaust the pool). +const localTails = new Map>(); + +async function runExclusiveInProcess(key: string, fn: () => Promise): Promise { + const prevTail = localTails.get(key) ?? Promise.resolve(); + let release!: () => void; + const tail = new Promise((resolve) => { + release = resolve; + }); + localTails.set(key, tail); + await prevTail; // wait for the previous holder of this key + try { + return await fn(); + } finally { + release(); + // Drop the entry once we are the last in line, to keep the map bounded. + if (localTails.get(key) === tail) localTails.delete(key); + } +} + +/** + * Serialize a "count current usage -> assert under limit -> create" sequence + * against concurrent callers for the same (organization, capability). + * + * The enforcement pattern (COUNT -> assertWithinLimit -> insert) is a + * check-then-act: without serialization, parallel requests can each read a count + * below the limit and then all insert, pushing usage past a finite cap. + * + * Serialization happens at two levels: an in-process mutex per org+capability + * (so within one backend instance only one such create runs at a time, and + * waiters do not hold a database connection while queued), wrapping a + * transaction-scoped Postgres advisory lock on the same key (so the guarantee + * also holds across multiple backend instances sharing the database). The + * advisory lock is released automatically on commit/rollback. + * + * Different organizations and different capabilities use distinct keys and do + * not contend. When no finite limit is configured (the OSS default) the only + * added cost is one advisory lock/unlock round-trip. + */ +export async function withLimitLock( + organizationId: string, + capabilityKey: string, + fn: () => Promise, +): Promise { + const key = `${organizationId}:${capabilityKey}`; + const key2 = hash32(key); + return runExclusiveInProcess(key, () => + db.transaction().execute(async (trx) => { + await sql`SELECT pg_advisory_xact_lock(${CAP_LOCK_NAMESPACE}, ${key2})`.execute(trx); + return fn(); + }), + ); +} diff --git a/packages/backend/src/modules/alerts/routes.ts b/packages/backend/src/modules/alerts/routes.ts index 302294cd..add2cbfe 100644 --- a/packages/backend/src/modules/alerts/routes.ts +++ b/packages/backend/src/modules/alerts/routes.ts @@ -8,7 +8,7 @@ import { OrganizationsService } from '../organizations/service.js'; import { projectsService } from '../projects/service.js'; import { notificationChannelsService } from '../notification-channels/index.js'; import { auditLogService } from '../audit-log/index.js'; -import { assertWithinLimit } from '../../capabilities/index.js'; +import { assertWithinLimit, withLimitLock } from '../../capabilities/index.js'; const organizationsService = new OrganizationsService(); @@ -184,26 +184,27 @@ export async function alertsRoutes(fastify: FastifyInstance) { // Session-auth requests don't populate request.organizationId in the ALS // context, so we establish a scoped system context with the org from the // validated body. Same pattern as otlp/trace-routes.ts. - // Note: count -> insert is not atomic; a concurrent create can briefly - // exceed the cap by one. Acceptable for user-initiated rule creation. - await context.runAsSystem('alerts:create-limit-check', async () => { - await context.with({ organizationId: body.organizationId }, async () => { - const currentRuleCount = await alertsService.countAlertRules(body.organizationId); - await assertWithinLimit('alerts.max_rules', currentRuleCount); - }); - }); - + // The count -> insert is serialized per org via withLimitLock so concurrent + // creates can't race past the cap. const { channelIds, alertType, baselineType, deviationMultiplier, minBaselineValue, cooldownMinutes, sustainedMinutes, metadataFilters, ...alertData } = body; - const alertRule = await alertsService.createAlertRule({ - ...alertData, - alertType: alertType || 'threshold', - baselineType: baselineType || null, - deviationMultiplier: deviationMultiplier ?? null, - minBaselineValue: minBaselineValue ?? null, - cooldownMinutes: cooldownMinutes ?? null, - sustainedMinutes: sustainedMinutes ?? null, - emailRecipients: alertData.emailRecipients || [], - metadataFilters: metadataFilters ?? [], + const alertRule = await withLimitLock(body.organizationId, 'alerts.max_rules', async () => { + await context.runAsSystem('alerts:create-limit-check', async () => { + await context.with({ organizationId: body.organizationId }, async () => { + const currentRuleCount = await alertsService.countAlertRules(body.organizationId); + await assertWithinLimit('alerts.max_rules', currentRuleCount); + }); + }); + return alertsService.createAlertRule({ + ...alertData, + alertType: alertType || 'threshold', + baselineType: baselineType || null, + deviationMultiplier: deviationMultiplier ?? null, + minBaselineValue: minBaselineValue ?? null, + cooldownMinutes: cooldownMinutes ?? null, + sustainedMinutes: sustainedMinutes ?? null, + emailRecipients: alertData.emailRecipients || [], + metadataFilters: metadataFilters ?? [], + }); }); // Associate channels with the alert rule diff --git a/packages/backend/src/modules/api-keys/routes.ts b/packages/backend/src/modules/api-keys/routes.ts index 8d1d256f..7d50b8fb 100644 --- a/packages/backend/src/modules/api-keys/routes.ts +++ b/packages/backend/src/modules/api-keys/routes.ts @@ -6,7 +6,7 @@ import { apiKeysService } from './service.js'; import { authenticate } from '../auth/middleware.js'; import { projectsService } from '../projects/service.js'; import { auditLogService } from '../audit-log/index.js'; -import { assertWithinLimit } from '../../capabilities/index.js'; +import { assertWithinLimit, withLimitLock } from '../../capabilities/index.js'; import { CapabilityError } from '../../capabilities/errors.js'; const createApiKeySchema = z.object({ @@ -72,18 +72,20 @@ export async function apiKeysRoutes(fastify: FastifyInstance) { }); } - await context.runAsSystem('apikeys:create-limit-check', async () => { - await context.with({ organizationId: project.organizationId }, async () => { - const count = await apiKeysService.countKeysForOrg(project.organizationId); - await assertWithinLimit('apikeys.max', count); + const result = await withLimitLock(project.organizationId, 'apikeys.max', async () => { + await context.runAsSystem('apikeys:create-limit-check', async () => { + await context.with({ organizationId: project.organizationId }, async () => { + const count = await apiKeysService.countKeysForOrg(project.organizationId); + await assertWithinLimit('apikeys.max', count); + }); }); - }); - const result = await apiKeysService.createApiKey({ - projectId, - name: body.name, - type: body.type, - allowedOrigins: body.allowedOrigins ?? null, + return apiKeysService.createApiKey({ + projectId, + name: body.name, + type: body.type, + allowedOrigins: body.allowedOrigins ?? null, + }); }); await auditLogService.record({ diff --git a/packages/backend/src/modules/custom-dashboards/routes.ts b/packages/backend/src/modules/custom-dashboards/routes.ts index 6d57278b..778d587d 100644 --- a/packages/backend/src/modules/custom-dashboards/routes.ts +++ b/packages/backend/src/modules/custom-dashboards/routes.ts @@ -6,7 +6,7 @@ import { customDashboardsService } from './service.js'; import { panelInstanceSchema } from './panel-registry.js'; import { fetchPanelData } from './panel-data-service.js'; import { context } from '@logtide/shared/context'; -import { assertWithinLimit } from '../../capabilities/index.js'; +import { assertWithinLimit, withLimitLock } from '../../capabilities/index.js'; import { CapabilityError } from '../../capabilities/errors.js'; import { auditLogService } from '../audit-log/service.js'; @@ -92,24 +92,26 @@ export async function customDashboardsRoutes(fastify: FastifyInstance) { return reply.status(403).send({ error: 'Forbidden' }); } - await context.runAsSystem('dashboards:create-limit-check', async () => { - await context.with({ organizationId: body.organizationId }, async () => { - const count = await customDashboardsService.countForOrg(body.organizationId); - await assertWithinLimit('dashboards.max_custom', count); + const dashboard = await withLimitLock(body.organizationId, 'dashboards.max_custom', async () => { + await context.runAsSystem('dashboards:create-limit-check', async () => { + await context.with({ organizationId: body.organizationId }, async () => { + const count = await customDashboardsService.countForOrg(body.organizationId); + await assertWithinLimit('dashboards.max_custom', count); + }); }); - }); - const dashboard = await customDashboardsService.create( - { - organizationId: body.organizationId, - projectId: body.projectId ?? null, - name: body.name, - description: body.description ?? null, - isPersonal: body.isPersonal, - panels: body.panels, - }, - request.user.id - ); + return customDashboardsService.create( + { + organizationId: body.organizationId, + projectId: body.projectId ?? null, + name: body.name, + description: body.description ?? null, + isPersonal: body.isPersonal, + panels: body.panels, + }, + request.user.id + ); + }); await auditLogService.record({ action: 'dashboard.created', @@ -140,18 +142,20 @@ export async function customDashboardsRoutes(fastify: FastifyInstance) { return reply.status(403).send({ error: 'Forbidden' }); } - await context.runAsSystem('dashboards:create-limit-check', async () => { - await context.with({ organizationId: body.organizationId }, async () => { - const count = await customDashboardsService.countForOrg(body.organizationId); - await assertWithinLimit('dashboards.max_custom', count); + const dashboard = await withLimitLock(body.organizationId, 'dashboards.max_custom', async () => { + await context.runAsSystem('dashboards:create-limit-check', async () => { + await context.with({ organizationId: body.organizationId }, async () => { + const count = await customDashboardsService.countForOrg(body.organizationId); + await assertWithinLimit('dashboards.max_custom', count); + }); }); - }); - const dashboard = await customDashboardsService.importYaml( - body.yaml, - body.organizationId, - request.user.id - ); + return customDashboardsService.importYaml( + body.yaml, + body.organizationId, + request.user.id + ); + }); await auditLogService.record({ action: 'dashboard.imported', diff --git a/packages/backend/src/modules/notification-channels/routes.ts b/packages/backend/src/modules/notification-channels/routes.ts index 3213ad46..4bac2739 100644 --- a/packages/backend/src/modules/notification-channels/routes.ts +++ b/packages/backend/src/modules/notification-channels/routes.ts @@ -9,7 +9,7 @@ import { authenticate } from '../auth/middleware.js'; import { OrganizationsService } from '../organizations/service.js'; import type { NotificationEventType } from '@logtide/shared'; import { context } from '@logtide/shared/context'; -import { assertWithinLimit } from '../../capabilities/index.js'; +import { assertWithinLimit, withLimitLock } from '../../capabilities/index.js'; import { CapabilityError } from '../../capabilities/errors.js'; import { auditLogService } from '../audit-log/service.js'; @@ -173,18 +173,20 @@ export async function notificationChannelsRoutes(fastify: FastifyInstance) { return reply.status(403).send({ error: 'Only admins can create notification channels' }); } - await context.runAsSystem('channels:create-limit-check', async () => { - await context.with({ organizationId }, async () => { - const count = await notificationChannelsService.countChannels(organizationId); - await assertWithinLimit('notifications.max_channels', count); + const channel = await withLimitLock(organizationId, 'notifications.max_channels', async () => { + await context.runAsSystem('channels:create-limit-check', async () => { + await context.with({ organizationId }, async () => { + const count = await notificationChannelsService.countChannels(organizationId); + await assertWithinLimit('notifications.max_channels', count); + }); }); - }); - const channel = await notificationChannelsService.createChannel( - organizationId, - body, - request.user.id - ); + return notificationChannelsService.createChannel( + organizationId, + body, + request.user.id + ); + }); await auditLogService.record({ action: 'channel.created', diff --git a/packages/backend/src/modules/otlp/trace-transformer.ts b/packages/backend/src/modules/otlp/trace-transformer.ts index 383c4d79..d31a862f 100644 --- a/packages/backend/src/modules/otlp/trace-transformer.ts +++ b/packages/backend/src/modules/otlp/trace-transformer.ts @@ -7,7 +7,7 @@ */ import type { SpanKind, SpanStatusCode } from '../../database/types.js'; -import { attributesToRecord, sanitizeForPostgres, type OtlpKeyValue } from './transformer.js'; +import { attributesToRecord, sanitizeForPostgres, sanitizeServiceName, type OtlpKeyValue } from './transformer.js'; import { isGzipCompressed, decompressGzip } from './parser.js'; import { createRequire } from 'module'; @@ -332,7 +332,7 @@ export function extractServiceName(attributes?: OtlpKeyValue[]): string { const serviceAttr = attributes.find((attr) => attr.key === 'service.name'); if (serviceAttr?.value?.stringValue) { - return sanitizeForPostgres(serviceAttr.value.stringValue); + return sanitizeServiceName(serviceAttr.value.stringValue); } return 'unknown'; diff --git a/packages/backend/src/modules/otlp/transformer.ts b/packages/backend/src/modules/otlp/transformer.ts index d37d07fb..705214c0 100644 --- a/packages/backend/src/modules/otlp/transformer.ts +++ b/packages/backend/src/modules/otlp/transformer.ts @@ -218,6 +218,27 @@ export function sanitizeForPostgres(str: string): string { return str.replace(/\x00/g, ''); } +/** + * Maximum stored length for an OTLP service name. Service names are short + * identifiers; anything longer is almost certainly abuse or a bug. + */ +export const MAX_SERVICE_NAME_LENGTH = 255; + +/** + * Defense-in-depth sanitization for OTLP service.name before it is stored and + * later rendered (dashboards, service map). Strips control characters (C0, DEL, + * C1) including null bytes, and caps the length. It deliberately does NOT strip + * otherwise legitimate characters (such as < > " '): those are neutralized by + * output-encoding at each sink, and stripping them here would corrupt the value + * for the JSON API and other consumers. Returns 'unknown' if nothing usable + * remains. + */ +export function sanitizeServiceName(raw: string): string { + // eslint-disable-next-line no-control-regex + const cleaned = raw.replace(/[\x00-\x1f\x7f-\x9f]/g, '').slice(0, MAX_SERVICE_NAME_LENGTH); + return cleaned.trim().length > 0 ? cleaned : 'unknown'; +} + /** * Extract service name from resource attributes. * Falls back to 'unknown' if not found. @@ -227,7 +248,7 @@ export function extractServiceName(attributes?: OtlpKeyValue[]): string { const serviceAttr = attributes.find((attr) => attr.key === 'service.name'); if (serviceAttr?.value?.stringValue) { - return sanitizeForPostgres(serviceAttr.value.stringValue); + return sanitizeServiceName(serviceAttr.value.stringValue); } return 'unknown'; diff --git a/packages/backend/src/modules/sigma/routes.ts b/packages/backend/src/modules/sigma/routes.ts index e98a8340..b0002c89 100644 --- a/packages/backend/src/modules/sigma/routes.ts +++ b/packages/backend/src/modules/sigma/routes.ts @@ -8,7 +8,7 @@ import { OrganizationsService } from '../organizations/service.js'; import { notificationChannelsService } from '../notification-channels/index.js'; import { auditLogService } from '../audit-log/service.js'; import { context } from '@logtide/shared/context'; -import { assertWithinLimit } from '../../capabilities/index.js'; +import { assertWithinLimit, withLimitLock } from '../../capabilities/index.js'; import { CapabilityError } from '../../capabilities/errors.js'; const sigmaService = new SigmaService(); @@ -93,15 +93,18 @@ export async function sigmaRoutes(fastify: FastifyInstance) { }); } - // Cap on enabled sigma rules (#214 follow-up, WS2) - await context.runAsSystem('sigma:import-limit-check', async () => { - await context.with({ organizationId: body.organizationId }, async () => { - const activeCount = await sigmaService.countActiveRules(body.organizationId); - await assertWithinLimit('sigma.max_active_rules', activeCount); + // Cap on enabled sigma rules (#214 follow-up, WS2). Serialize the + // count + import per org so concurrent imports can't race past the cap. + const result = await withLimitLock(body.organizationId, 'sigma.max_active_rules', async () => { + await context.runAsSystem('sigma:import-limit-check', async () => { + await context.with({ organizationId: body.organizationId }, async () => { + const activeCount = await sigmaService.countActiveRules(body.organizationId); + await assertWithinLimit('sigma.max_active_rules', activeCount); + }); }); - }); - const result = await sigmaService.importSigmaRule(importData); + return sigmaService.importSigmaRule(importData); + }); // Associate channels with the sigma rule if import was successful if (result.sigmaRule && channelIds && channelIds.length > 0) { @@ -321,22 +324,31 @@ export async function sigmaRoutes(fastify: FastifyInstance) { if (!existingRule) { return reply.code(404).send({ error: 'Sigma rule not found' }); } - // Cap on enabled sigma rules - only check when actually transitioning disabled -> enabled + // Cap on enabled sigma rules - only check when actually transitioning + // disabled -> enabled. Serialize the count + enable per org so two + // concurrent enables can't race past the cap. if (!existingRule.enabled) { - await context.runAsSystem('sigma:enable-limit-check', async () => { - await context.with({ organizationId: body.organizationId }, async () => { - const activeCount = await sigmaService.countActiveRules(body.organizationId); - await assertWithinLimit('sigma.max_active_rules', activeCount); + rule = await withLimitLock(body.organizationId, 'sigma.max_active_rules', async () => { + await context.runAsSystem('sigma:enable-limit-check', async () => { + await context.with({ organizationId: body.organizationId }, async () => { + const activeCount = await sigmaService.countActiveRules(body.organizationId); + await assertWithinLimit('sigma.max_active_rules', activeCount); + }); }); + // Inside the `body.enabled === true` branch: enable explicitly. + return sigmaService.toggleSigmaRule(params.id, body.organizationId, true); }); } } - rule = await sigmaService.toggleSigmaRule( - params.id, - body.organizationId, - body.enabled - ); + // For non-enable-transition cases (disable, or already enabled) toggle now. + if (!rule) { + rule = await sigmaService.toggleSigmaRule( + params.id, + body.organizationId, + body.enabled + ); + } if (!rule) { return reply.code(404).send({ error: 'Sigma rule not found' }); diff --git a/packages/backend/src/modules/users/service.ts b/packages/backend/src/modules/users/service.ts index aec00c16..3dc1f36b 100644 --- a/packages/backend/src/modules/users/service.ts +++ b/packages/backend/src/modules/users/service.ts @@ -1,11 +1,17 @@ import bcrypt from 'bcrypt'; import crypto from 'crypto'; +import { sql } from 'kysely'; import { db } from '../../database/connection.js'; import { CacheManager, CACHE_TTL } from '../../utils/cache.js'; const SALT_ROUNDS = 10; const SESSION_DURATION_DAYS = 30; +// Constant key for the Postgres transaction-scoped advisory lock that +// serializes the first-admin promotion decision across concurrent +// registrations (see createUser). +const FIRST_ADMIN_ADVISORY_LOCK = 947163001; + export interface CreateUserInput { email: string; password: string; @@ -98,26 +104,39 @@ export class UsersService { throw new Error('User with this email already exists'); } - // Hash the password + // Hash the password (outside the lock below - bcrypt is intentionally slow) const passwordHash = await this.hashPassword(input.password); - // Promote first user to admin if no admin exists yet - const shouldBeAdmin = !(await this.hasAnyAdmin()); - if (shouldBeAdmin) { - console.log(`[Users] No admin exists yet. Promoting ${email} to admin on registration.`); - } + // Decide first-admin promotion and insert atomically. Without serialization, + // two concurrent registrations in the zero-admin bootstrap window could each + // observe "no admin yet" and both be promoted. A transaction-scoped advisory + // lock makes the check-then-insert atomic, so at most one registration wins + // the promotion. The lock is released automatically on commit/rollback. + const user = await db.transaction().execute(async (trx) => { + await sql`SELECT pg_advisory_xact_lock(${FIRST_ADMIN_ADVISORY_LOCK})`.execute(trx); - // Insert the user - const user = await db - .insertInto('users') - .values({ - email, - password_hash: passwordHash, - name: input.name, - is_admin: shouldBeAdmin, - }) - .returning(['id', 'email', 'name', 'is_admin', 'disabled', 'created_at', 'last_login']) - .executeTakeFirstOrThrow(); + const adminRow = await trx + .selectFrom('users') + .select('id') + .where('is_admin', '=', true) + .executeTakeFirst(); + + const shouldBeAdmin = !adminRow; + if (shouldBeAdmin) { + console.log(`[Users] No admin exists yet. Promoting ${email} to admin on registration.`); + } + + return trx + .insertInto('users') + .values({ + email, + password_hash: passwordHash, + name: input.name, + is_admin: shouldBeAdmin, + }) + .returning(['id', 'email', 'name', 'is_admin', 'disabled', 'created_at', 'last_login']) + .executeTakeFirstOrThrow(); + }); return { id: user.id, diff --git a/packages/backend/src/tests/modules/capabilities/apikeys-limit.test.ts b/packages/backend/src/tests/modules/capabilities/apikeys-limit.test.ts index 11dededb..b2ce4a9a 100644 --- a/packages/backend/src/tests/modules/capabilities/apikeys-limit.test.ts +++ b/packages/backend/src/tests/modules/capabilities/apikeys-limit.test.ts @@ -137,6 +137,43 @@ describe('apikeys.max enforcement', () => { expect(res.statusCode).toBe(201); }); + // Case 5: concurrent creates must not race past a finite limit + it('serializes concurrent creates so the limit is never exceeded (race)', async () => { + // createTestContext already inserted 1 key; limit 2 leaves room for exactly 1 more. + await db + .insertInto('organization_entitlements') + .values({ organization_id: orgId, capability: 'apikeys.max', enabled: null, limit_value: 2 }) + .execute(); + capabilities.invalidate(orgId); + + const attempts = 6; + const results = await Promise.all( + Array.from({ length: attempts }, (_, i) => + app.inject({ + method: 'POST', + url: `/api/v1/projects/${projectId}/api-keys`, + headers: { Authorization: `Bearer ${token}` }, + payload: { name: `Race Key ${i}` }, + }) + ) + ); + + const created = results.filter((r) => r.statusCode === 201).length; + const blocked = results.filter((r) => r.statusCode === 403).length; + + expect(created).toBe(1); + expect(blocked).toBe(attempts - 1); + + // Hard invariant: the org never ends up over its configured limit. + const total = await db + .selectFrom('api_keys') + .innerJoin('projects', 'projects.id', 'api_keys.project_id') + .select((eb) => eb.fn.countAll().as('c')) + .where('projects.organization_id', '=', orgId) + .executeTakeFirstOrThrow(); + expect(Number(total.c)).toBe(2); + }); + // Case 4: org isolation => org A at limit does not block org B it('does not block org B when org A is at the limit', async () => { await db diff --git a/packages/backend/src/tests/modules/otlp/transformer.test.ts b/packages/backend/src/tests/modules/otlp/transformer.test.ts index a30af39b..3076bfa4 100644 --- a/packages/backend/src/tests/modules/otlp/transformer.test.ts +++ b/packages/backend/src/tests/modules/otlp/transformer.test.ts @@ -3,6 +3,8 @@ import { transformOtlpToLogTide, transformLogRecord, extractServiceName, + sanitizeServiceName, + MAX_SERVICE_NAME_LENGTH, nanosToIso, normalizeTraceId, extractMessage, @@ -464,6 +466,60 @@ describe('OTLP Transformer', () => { expect(extractServiceName(attrs)).toBe('unknown'); }); + + it('strips control characters from service.name (defense-in-depth)', () => { + const attrs: OtlpKeyValue[] = [ + { key: 'service.name', value: { stringValue: 'api\x00-svc\x07\x1bx' } }, + ]; + + expect(extractServiceName(attrs)).toBe('api-svcx'); + }); + + it('caps an over-long service.name', () => { + const attrs: OtlpKeyValue[] = [ + { key: 'service.name', value: { stringValue: 'a'.repeat(1000) } }, + ]; + + expect(extractServiceName(attrs)).toHaveLength(MAX_SERVICE_NAME_LENGTH); + }); + + it('preserves legitimate punctuation (escaping happens at the sink)', () => { + const attrs: OtlpKeyValue[] = [ + { key: 'service.name', value: { stringValue: '' } }, + ]; + + // Not stripped here: the value stays raw and is HTML-escaped at render time. + expect(extractServiceName(attrs)).toBe(''); + }); + + it('falls back to "unknown" when only control characters remain', () => { + const attrs: OtlpKeyValue[] = [ + { key: 'service.name', value: { stringValue: '\x00\x01\x02' } }, + ]; + + expect(extractServiceName(attrs)).toBe('unknown'); + }); + }); + + describe('sanitizeServiceName', () => { + it('passes through a normal name', () => { + expect(sanitizeServiceName('payment-service')).toBe('payment-service'); + }); + + it('removes null bytes and other control characters', () => { + expect(sanitizeServiceName('a\x00b\x1fc\x7fd')).toBe('abcd'); + }); + + it('caps length at MAX_SERVICE_NAME_LENGTH', () => { + expect(sanitizeServiceName('x'.repeat(MAX_SERVICE_NAME_LENGTH + 50))).toHaveLength( + MAX_SERVICE_NAME_LENGTH, + ); + }); + + it('returns "unknown" when empty after cleaning', () => { + expect(sanitizeServiceName('\x00\x01')).toBe('unknown'); + expect(sanitizeServiceName('')).toBe('unknown'); + }); }); describe('nanosToIso', () => { diff --git a/packages/backend/src/tests/modules/users/users-service.test.ts b/packages/backend/src/tests/modules/users/users-service.test.ts index 868a5e55..35481494 100644 --- a/packages/backend/src/tests/modules/users/users-service.test.ts +++ b/packages/backend/src/tests/modules/users/users-service.test.ts @@ -130,6 +130,33 @@ describe('UsersService', () => { expect(second.is_admin).toBe(false); }); + it('promotes at most one admin under concurrent first-time registrations (race)', async () => { + // Fire several registrations at once against an empty (zero-admin) + // users table. Without serialization each would observe "no admin + // yet" and all be promoted to admin. + const N = 5; + const users = await Promise.all( + Array.from({ length: N }, (_, i) => + usersService.createUser({ + email: `race-${i}@example.com`, + password: 'password123', + name: `Race User ${i}`, + }) + ) + ); + + const admins = users.filter((u) => u.is_admin); + expect(admins).toHaveLength(1); + + // Confirm at the storage layer too. + const adminCount = await db + .selectFrom('users') + .select((eb) => eb.fn.countAll().as('c')) + .where('is_admin', '=', true) + .executeTakeFirstOrThrow(); + expect(Number(adminCount.c)).toBe(1); + }); + it('should throw error for duplicate email', async () => { await usersService.createUser({ email: 'duplicate@example.com', diff --git a/packages/backend/src/tests/utils/ssrf-guard.test.ts b/packages/backend/src/tests/utils/ssrf-guard.test.ts index 9d59b65e..e3bcc51f 100644 --- a/packages/backend/src/tests/utils/ssrf-guard.test.ts +++ b/packages/backend/src/tests/utils/ssrf-guard.test.ts @@ -4,6 +4,8 @@ import { resolveAndValidateHost, assertHttpTargetAllowed, safeFetch, + createPinnedLookup, + __setSafeFetchImpl, SsrfBlockedError, } from '../../utils/ssrf-guard.js'; @@ -92,8 +94,31 @@ describe('assertHttpTargetAllowed', () => { }); }); +describe('createPinnedLookup (DNS rebinding protection)', () => { + it('always returns the pinned address regardless of the hostname asked', () => { + const lookup = createPinnedLookup('93.184.216.34'); + let got: { address: string; family?: number } | undefined; + lookup('evil.example.com', {}, (_err, address, family) => { + got = { address: address as string, family }; + }); + expect(got).toEqual({ address: '93.184.216.34', family: 4 }); + }); + + it('supports the { all: true } array form with the correct family', () => { + const lookup = createPinnedLookup('2606:4700:4700::1111'); + let got: unknown; + lookup('evil.example.com', { all: true }, (_err, list) => { + got = list; + }); + expect(got).toEqual([{ address: '2606:4700:4700::1111', family: 6 }]); + }); +}); + describe('safeFetch redirect revalidation', () => { - afterEach(() => vi.restoreAllMocks()); + afterEach(() => { + vi.restoreAllMocks(); + __setSafeFetchImpl(null); + }); function res(status: number, location?: string) { return { @@ -108,16 +133,26 @@ describe('safeFetch redirect revalidation', () => { .fn() .mockResolvedValueOnce(res(302, 'http://1.1.1.1/next')) .mockResolvedValueOnce(res(200)); - global.fetch = fetchMock as any; + __setSafeFetchImpl(fetchMock as any); const result = await safeFetch('http://8.8.8.8/start', {}, { allowPrivate: false }); expect(result.status).toBe(200); expect(fetchMock).toHaveBeenCalledTimes(2); }); + it('passes a pinning dispatcher to the underlying fetch', async () => { + const fetchMock = vi.fn().mockResolvedValueOnce(res(200)); + __setSafeFetchImpl(fetchMock as any); + + await safeFetch('http://8.8.8.8/start', {}, { allowPrivate: false }); + const init = fetchMock.mock.calls[0][1]; + expect(init.dispatcher).toBeDefined(); + expect(init.redirect).toBe('manual'); + }); + it('blocks a redirect that points at an internal address', async () => { const fetchMock = vi.fn().mockResolvedValueOnce(res(302, 'http://169.254.169.254/latest/meta-data')); - global.fetch = fetchMock as any; + __setSafeFetchImpl(fetchMock as any); await expect(safeFetch('http://8.8.8.8/start', {}, { allowPrivate: false })).rejects.toBeInstanceOf( SsrfBlockedError @@ -126,7 +161,7 @@ describe('safeFetch redirect revalidation', () => { it('stops after too many redirects', async () => { const fetchMock = vi.fn().mockResolvedValue(res(302, 'http://1.1.1.1/loop')); - global.fetch = fetchMock as any; + __setSafeFetchImpl(fetchMock as any); await expect(safeFetch('http://8.8.8.8/start', {}, { allowPrivate: false, maxRedirects: 2 })).rejects.toBeInstanceOf( SsrfBlockedError diff --git a/packages/backend/src/utils/ssrf-guard.ts b/packages/backend/src/utils/ssrf-guard.ts index 1315eb81..a3f91e50 100644 --- a/packages/backend/src/utils/ssrf-guard.ts +++ b/packages/backend/src/utils/ssrf-guard.ts @@ -15,6 +15,7 @@ import { lookup } from 'dns/promises'; import { isIP } from 'net'; +import { Agent, fetch as undiciFetch } from 'undici'; export class SsrfBlockedError extends Error { constructor(message: string) { @@ -139,14 +140,49 @@ interface SafeFetchOptions { maxRedirects?: number; } +/** + * Build a DNS lookup function that always resolves to the already-validated + * `address`, regardless of the hostname asked. Handed to an undici Agent so the + * socket connects to the exact IP we checked, closing the resolve-then-connect + * (DNS rebinding) window. TLS SNI and certificate validation still use the + * original hostname; only the destination IP is pinned. + */ +export function createPinnedLookup(address: string) { + const family = isIP(address); // 4 or 6 (0 is rejected upstream as not-an-IP) + return ( + _hostname: string, + options: { all?: boolean } | undefined, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | Array<{ address: string; family: number }>, + family?: number, + ) => void, + ): void => { + if (options && options.all) { + callback(null, [{ address, family }]); + } else { + callback(null, address, family); + } + }; +} + +// Fetch implementation seam. Defaults to undici's fetch so we can attach a +// per-request dispatcher; overridable in tests via __setSafeFetchImpl. +type FetchImpl = (url: string | URL, init: Record) => Promise; +let fetchImpl: FetchImpl = undiciFetch as unknown as FetchImpl; + +/** @internal Test hook to stub the underlying fetch. Pass null to reset. */ +export function __setSafeFetchImpl(fn: FetchImpl | null): void { + fetchImpl = fn ?? (undiciFetch as unknown as FetchImpl); +} + /** * fetch() wrapper that validates the target before connecting and revalidates * every redirect hop instead of blindly following them. Each hop is resolved - * and checked, which closes redirect-to-internal bypasses. - * - * Note: there is a small resolve-then-connect window (DNS rebinding) that this - * does not fully pin for HTTPS without a custom dispatcher; the validation here - * addresses the reported direct-target and redirect bypasses. + * and checked, and the connection is PINNED to the validated IP via a custom + * undici dispatcher, which closes the resolve-then-connect (DNS rebinding) + * window for both HTTP and HTTPS. TLS still uses the original hostname for SNI + * and certificate validation. */ export async function safeFetch( rawUrl: string, @@ -166,9 +202,22 @@ export async function safeFetch( if (url.protocol !== 'http:' && url.protocol !== 'https:') { throw new SsrfBlockedError('Only http and https targets are allowed'); } - await resolveAndValidateHost(url.hostname, options.allowPrivate); + const addresses = await resolveAndValidateHost(url.hostname, options.allowPrivate); - const response = await fetch(url, { ...init, redirect: 'manual' }); + // Pin the socket to the validated address so a hostname that re-resolves to + // an internal IP between validation and connect cannot be reached. + const dispatcher = new Agent({ connect: { lookup: createPinnedLookup(addresses[0]) } }); + + let response: Response; + try { + response = await fetchImpl(url, { ...init, redirect: 'manual', dispatcher }); + } catch (err) { + void dispatcher.close().catch(() => {}); + throw err; + } + // Gracefully close the per-hop dispatcher once the request completes; this + // does not abort the body the caller is about to read. + void dispatcher.close().catch(() => {}); const isRedirect = response.status >= 300 && response.status < 400; const location = isRedirect ? response.headers?.get('location') ?? null : null; diff --git a/packages/frontend/src/lib/utils/redirect.test.ts b/packages/frontend/src/lib/utils/redirect.test.ts new file mode 100644 index 00000000..f55933df --- /dev/null +++ b/packages/frontend/src/lib/utils/redirect.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect } from 'vitest'; +import { isSafeInternalPath, safeRedirect } from './redirect'; + +describe('isSafeInternalPath', () => { + it('accepts a normal in-app path', () => { + expect(isSafeInternalPath('/dashboard')).toBe(true); + expect(isSafeInternalPath('/dashboard/projects/1?tab=x')).toBe(true); + expect(isSafeInternalPath('/')).toBe(true); + }); + + it('rejects empty / nullish values', () => { + expect(isSafeInternalPath(null)).toBe(false); + expect(isSafeInternalPath(undefined)).toBe(false); + expect(isSafeInternalPath('')).toBe(false); + }); + + it('rejects absolute URLs to other origins', () => { + expect(isSafeInternalPath('https://evil.com')).toBe(false); + expect(isSafeInternalPath('http://evil.com/path')).toBe(false); + }); + + it('rejects protocol-relative URLs (//evil.com)', () => { + expect(isSafeInternalPath('//evil.com')).toBe(false); + expect(isSafeInternalPath('//evil.com/path')).toBe(false); + }); + + it('rejects backslash-smuggled protocol-relative URLs (/\\evil.com)', () => { + expect(isSafeInternalPath('/\\evil.com')).toBe(false); + }); + + it('rejects values not anchored at the site root', () => { + expect(isSafeInternalPath('dashboard')).toBe(false); + expect(isSafeInternalPath(' /dashboard')).toBe(false); + expect(isSafeInternalPath('javascript:alert(1)')).toBe(false); + }); +}); + +describe('safeRedirect', () => { + it('returns the path when safe', () => { + expect(safeRedirect('/dashboard/projects')).toBe('/dashboard/projects'); + }); + + it('falls back to /dashboard for unsafe or missing values', () => { + expect(safeRedirect(null)).toBe('/dashboard'); + expect(safeRedirect('//evil.com')).toBe('/dashboard'); + expect(safeRedirect('https://evil.com')).toBe('/dashboard'); + }); + + it('honors a custom fallback', () => { + expect(safeRedirect('//evil.com', '/onboarding')).toBe('/onboarding'); + }); +}); diff --git a/packages/frontend/src/lib/utils/redirect.ts b/packages/frontend/src/lib/utils/redirect.ts new file mode 100644 index 00000000..365f322f --- /dev/null +++ b/packages/frontend/src/lib/utils/redirect.ts @@ -0,0 +1,25 @@ +/** + * Guard against open-redirect via a user-supplied `redirect` query parameter. + * + * A value is only safe to navigate to if it is anchored at the site root and + * cannot be coerced into a cross-origin destination. We require it to start with + * a single "/" and reject protocol-relative forms ("//evil.com") including the + * backslash variant ("/\\evil.com") that some browsers normalize to "//". + */ +export function isSafeInternalPath(path: string | null | undefined): path is string { + if (!path) return false; + if (path[0] !== '/') return false; // must be relative to the site root + // Block "//evil.com" and "/\evil.com" (protocol-relative / browser-normalized). + if (path[1] === '/' || path[1] === '\\') return false; + return true; +} + +/** + * Return `path` if it is a safe in-app destination, otherwise `fallback`. + */ +export function safeRedirect( + path: string | null | undefined, + fallback = '/dashboard', +): string { + return isSafeInternalPath(path) ? path : fallback; +} diff --git a/packages/frontend/src/routes/login/+page.svelte b/packages/frontend/src/routes/login/+page.svelte index 8541dbb2..2fddc1d6 100644 --- a/packages/frontend/src/routes/login/+page.svelte +++ b/packages/frontend/src/routes/login/+page.svelte @@ -17,6 +17,7 @@ import ProviderSelector from '$lib/components/auth/ProviderSelector.svelte'; import LdapLoginForm from '$lib/components/auth/LdapLoginForm.svelte'; import { smallLogoPath } from '$lib/utils/theme'; + import { isSafeInternalPath, safeRedirect } from '$lib/utils/redirect'; // Get redirect URL and error from query params let redirectUrl = $derived(page.url.searchParams.get('redirect')); @@ -53,8 +54,9 @@ try { const config = await authAPI.getAuthConfig(); if (config.authMode === 'none') { - // Auth-free mode: redirect to dashboard directly - goto(redirectUrl || '/dashboard'); + // Auth-free mode: redirect to dashboard directly (validate the + // user-supplied redirect to prevent open redirect) + goto(safeRedirect(redirectUrl)); return; } } catch (e) { @@ -129,8 +131,8 @@ } // If there's a redirect URL (e.g., invitation), go there - // Validate: must be a relative path starting with / and not // (prevent open redirect) - if (redirectUrl && redirectUrl.startsWith('/') && !redirectUrl.startsWith('//')) { + // Validate to prevent open redirect (see isSafeInternalPath) + if (isSafeInternalPath(redirectUrl)) { goto(redirectUrl); } else if (orgs.length === 0) { // No organizations -> redirect to onboarding tutorial diff --git a/packages/frontend/src/routes/register/+page.svelte b/packages/frontend/src/routes/register/+page.svelte index bc767982..7ea8a2aa 100644 --- a/packages/frontend/src/routes/register/+page.svelte +++ b/packages/frontend/src/routes/register/+page.svelte @@ -17,6 +17,7 @@ import ProviderSelector from '$lib/components/auth/ProviderSelector.svelte'; import LdapLoginForm from '$lib/components/auth/LdapLoginForm.svelte'; import { smallLogoPath } from '$lib/utils/theme'; + import { isSafeInternalPath, safeRedirect } from '$lib/utils/redirect'; // Get redirect URL from query params (e.g., for invitation flow) let redirectUrl = $derived(page.url.searchParams.get('redirect')); @@ -47,8 +48,9 @@ try { const config = await authAPI.getAuthConfig(); if (config.authMode === 'none') { - // Auth-free mode: redirect to dashboard directly - goto(redirectUrl || '/dashboard'); + // Auth-free mode: redirect to dashboard directly (validate the + // user-supplied redirect to prevent open redirect) + goto(safeRedirect(redirectUrl)); return; } if (!config.signupEnabled) { @@ -144,8 +146,8 @@ } // If there's a redirect URL (e.g., invitation), go there; otherwise go to onboarding - // Validate: must be a relative path starting with / and not // (prevent open redirect) - if (redirectUrl && redirectUrl.startsWith('/') && !redirectUrl.startsWith('//')) { + // Validate to prevent open redirect (see isSafeInternalPath) + if (isSafeInternalPath(redirectUrl)) { goto(redirectUrl); } else if (orgs.length === 0) { // New users don't have organizations yet -> redirect to onboarding tutorial diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 070008d6..bb8ef01f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -133,6 +133,9 @@ importers: tsx: specifier: ^4.21.0 version: 4.21.0 + undici: + specifier: '>=7.28.0 <8' + version: 7.28.0 zod: specifier: ^3.25.76 version: 3.25.76