diff --git a/CHANGELOG.md b/CHANGELOG.md index 4abcba67c..2929f2106 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **The 2026-08-13 security pass closes six resource and credential-exposure gaps.** Login admission now caps concurrent password verification before Argon2 runs; standard agent JSON requests have time, body, response, and redirect bounds; unterminated agent SSE events have a finite buffer; container log downloads and initial WebSocket history have finite line/byte limits; slow local log viewers are disconnected; registry data requests refuse redirects; and command/hook strings are redacted from component APIs and execution logs. The dated findings, evidence, and validation record are in `security_best_practices_report.md`. - Added a root `.trivyignore.yaml` suppressing AVD-DS-0002 (Dockerfile missing `USER`) with the same rationale already documented for the Dockerfile's `checkov:skip=CKV_DOCKER_3` comment and the existing qlty `trivy:DS-0002` triage rule: the entrypoint drops privileges at runtime via `su-exec` (`Docker.entrypoint.sh`), so no static `USER` instruction is needed. - **Service-worker `NetworkOnly` rule for `/api/**` now actually matches.** `ui/vite.config.ts`'s `runtimeCaching` entry used a `^`-anchored pathname regex (`/^\/api\//`), but workbox-routing tests a `RegExpRoute`'s `urlPattern` against the full `url.href` (always starting `http://`/`https://`), never the pathname alone, so the rule could never match and silently fell through. It was harmless today only because no other `runtimeCaching` rule exists to catch the fallthrough — any future catch-all caching rule would have started silently caching authenticated `/api` responses. Replaced with an exported `isApiRequest` match-callback function that tests `url.pathname.startsWith('/api/')`, so the rule actually engages. - **Dependency-group bulk update now requires destructive-action confirmation and binds to its own preview (v1.7, [discussion #219](https://github.com/CodesWhat/drydock/discussions/219)).** `POST /api/v1/dependency-groups/:rootId/update` could update or restart every container in a resolved dependency chain with no confirmation step and no binding to whatever chain the UI last previewed — a container added to the chain between preview and confirm was silently swept into the update. The route now requires the `X-DD-Confirm-Action: dependency-group-update` header, matching the existing `container-delete` pattern, and accepts an optional `expectedContainerIds` array in the request body; when present, a live chain that no longer matches it exactly (order-insensitive) is rejected with 409 and the actual current chain, instead of running against a chain the caller never saw. diff --git a/app/agent/AgentClient.test.ts b/app/agent/AgentClient.test.ts index 9a43e436a..a2ee96260 100644 --- a/app/agent/AgentClient.test.ts +++ b/app/agent/AgentClient.test.ts @@ -1297,6 +1297,50 @@ describe('AgentClient', () => { await vi.waitFor(() => expect(handleSpy).toHaveBeenCalledWith('dd:ack', { version: '1.0' })); }); + test('should bound queued SSE chunks while an event handler is blocked', async () => { + const stream = new EventEmitter(); + const destroy = vi.fn(); + Object.assign(stream, { destroy }); + axios.mockResolvedValue({ data: stream }); + const reconnectSpy = vi.spyOn(client, 'scheduleReconnect').mockImplementation(() => {}); + let releaseHandler: () => void = () => {}; + const blockedHandler = new Promise((resolve) => { + releaseHandler = resolve; + }); + vi.spyOn(client, 'handleEvent').mockReturnValue(blockedHandler); + + client.startSse(); + await vi.advanceTimersByTimeAsync(0); + stream.emit('data', Buffer.from('data: {"type":"dd:ack","data":{"version":"1.0"}}\n\n')); + await vi.waitFor(() => expect(client.handleEvent).toHaveBeenCalledOnce()); + + stream.emit('data', Buffer.alloc(16 * 1024 * 1024, 0x61)); + + await vi.waitFor(() => expect(destroy).toHaveBeenCalledOnce()); + expect(reconnectSpy).toHaveBeenCalledOnce(); + releaseHandler(); + }); + + test('should destroy and reconnect when an unterminated SSE event exceeds the buffer limit', async () => { + const stream = new EventEmitter(); + const destroy = vi.fn(); + Object.assign(stream, { destroy }); + axios.mockResolvedValue({ data: stream }); + const reconnectSpy = vi.spyOn(client, 'scheduleReconnect').mockImplementation(() => {}); + + client.startSse(); + await vi.advanceTimersByTimeAsync(0); + + stream.emit('data', Buffer.alloc(16 * 1024 * 1024 + 1, 0x61)); + + await vi.waitFor(() => expect(destroy).toHaveBeenCalledOnce()); + expect(reconnectSpy).toHaveBeenCalledOnce(); + expect((client as any).activeSseStream).toBeUndefined(); + expect(mockLogChild.error).toHaveBeenCalledWith( + 'SSE event buffer exceeded the 16777216-byte limit. Reconnecting...', + ); + }); + test('should process streamed container and watcher snapshot events in order', async () => { const stream = new EventEmitter(); axios.mockResolvedValue({ data: stream }); @@ -3501,6 +3545,15 @@ describe('AgentClient', () => { await client.runRemoteTrigger(container, 'smtp', 'notify'); const [, postedPayload] = axios.post.mock.calls[0]; expect(postedPayload).toBe(container); + expect(axios.post.mock.calls[0][2]).toEqual(expect.objectContaining({ timeout: 65_000 })); + }); + + test('should preserve the 30-second timeout for accepted update triggers', async () => { + axios.post.mockResolvedValue({ data: {} }); + + await client.runRemoteTrigger({ id: 'c1', name: 'web' }, 'docker', 'update'); + + expect(axios.post.mock.calls[0][2]).toEqual(expect.objectContaining({ timeout: 30_000 })); }); test('should throw on failure', async () => { @@ -3663,7 +3716,7 @@ describe('AgentClient', () => { expect(axios.post).toHaveBeenCalledWith( expect.stringContaining('/api/triggers/docker/update/batch'), containers, - expect.any(Object), + expect.objectContaining({ timeout: 30_000 }), ); }); @@ -3681,6 +3734,7 @@ describe('AgentClient', () => { }); await client.runRemoteTriggerBatch([{ id: 'c1' }], 'mock', 'notify'); + expect(axios.post.mock.calls[0][2]).toEqual(expect.objectContaining({ timeout: 65_000 })); await client.handleEvent('dd:container-updated', { id: 'c1', name: 'test', @@ -4585,6 +4639,22 @@ describe('AgentClient', () => { expect(Object.keys((c as any).axiosOptions.headers).length).toBeGreaterThan(0); }); + test('ordinary agent JSON requests have finite resource bounds and reject redirects', async () => { + axios.get.mockResolvedValue({ data: { type: 'docker', name: 'local', configuration: {} } }); + + await client.getWatcher('docker', 'local'); + + expect(axios.get).toHaveBeenCalledWith( + 'https://localhost:3001/api/watchers/docker/local', + expect.objectContaining({ + timeout: 30_000, + maxContentLength: 16 * 1024 * 1024, + maxBodyLength: 16 * 1024 * 1024, + maxRedirects: 0, + }), + ); + }); + // Line 256: ConditionalExpression true — shouldBuildHttpsAgent test('does not create httpsAgent when neither certfile nor cafile is provided', () => { const c = new AgentClient('agent-no-tls', { @@ -7220,6 +7290,24 @@ describe('AgentClient', () => { expect(axiosCallArg.method).not.toBe(''); }); + test('rejects SSE redirects without applying finite JSON response or request time limits', async () => { + axios.mockResolvedValue({ data: new EventEmitter() }); + + client.startSse(); + await vi.advanceTimersByTimeAsync(0); + + const axiosCallArg = (axios as any).mock.calls[0][0]; + expect(axiosCallArg).toEqual( + expect.objectContaining({ + responseType: 'stream', + maxRedirects: 0, + }), + ); + expect(axiosCallArg).not.toHaveProperty('timeout'); + expect(axiosCallArg).not.toHaveProperty('maxContentLength'); + expect(axiosCallArg).not.toHaveProperty('maxBodyLength'); + }); + test('logs non-empty error when startSse axios call fails', async () => { axios.mockRejectedValue(new Error('connection refused')); client.startSse(); @@ -7912,6 +8000,14 @@ describe('AgentClient', () => { '/api/containers/cid/logs?tail=100&since=0×tamps=false', Buffer.alloc(0), ); + expect(axios.get.mock.calls[0][1]).toEqual( + expect.objectContaining({ + timeout: 30_000, + maxContentLength: 16 * 1024 * 1024, + maxBodyLength: 16 * 1024 * 1024, + maxRedirects: 0, + }), + ); }); test('getLogEntries signs the exact query string in wire order', async () => { diff --git a/app/agent/AgentClient.ts b/app/agent/AgentClient.ts index 082397988..90bcc0cae 100644 --- a/app/agent/AgentClient.ts +++ b/app/agent/AgentClient.ts @@ -116,6 +116,10 @@ export interface DockerApiProxyResponse { const MAX_DOCKER_PROXY_RESPONSE_BYTES = 100 * 1024 * 1024; const PORTWING_DOCKER_PROXY_INACTIVITY_TIMEOUT_MS = 30_000; +const AGENT_REQUEST_TIMEOUT_MS = 30_000; +const SYNCHRONOUS_REMOTE_TRIGGER_TIMEOUT_MS = 65_000; +const MAX_AGENT_JSON_BYTES = 16 * 1024 * 1024; +const MAX_SSE_EVENT_BUFFER_BYTES = 16 * 1024 * 1024; function isStreamingDockerTarget(target: string): boolean { const path = target.split('?', 1)[0]; @@ -396,7 +400,7 @@ export class AgentClient { } private buildAxiosOptions(): AxiosRequestConfig { - const options: AxiosRequestConfig = {}; + const options: AxiosRequestConfig = { maxRedirects: 0 }; // Token mode (default): static X-Dd-Agent-Secret header, unchanged from // pre-ed25519 behavior. Ed25519 mode signs each request individually (see @@ -461,9 +465,28 @@ export class AgentClient { * `path` is the exact origin-form request target placed on the wire: * escaped path plus the unmodified raw query string. Portwing signature v2 * verifies those bytes verbatim, including query ordering and escaping. - * In token mode this just returns the static axiosOptions, unchanged. + * In token mode this uses the static authentication options. Ordinary JSON + * requests also receive finite transport and body limits. */ - private buildRequestConfig(method: string, path: string, data?: unknown): AxiosRequestConfig { + private buildRequestConfig( + method: string, + path: string, + data?: unknown, + timeout = AGENT_REQUEST_TIMEOUT_MS, + ): AxiosRequestConfig { + return { + ...this.buildAuthenticatedRequestConfig(method, path, data), + timeout, + maxContentLength: MAX_AGENT_JSON_BYTES, + maxBodyLength: MAX_AGENT_JSON_BYTES, + }; + } + + private buildAuthenticatedRequestConfig( + method: string, + path: string, + data?: unknown, + ): AxiosRequestConfig { if (!this.ed25519PrivateKey || !this.config.signingkeyid) { return this.axiosOptions; } @@ -1145,11 +1168,21 @@ export class AgentClient { return remainder; } - private attachStreamHandlers(stream: NodeJS.EventEmitter) { + private attachStreamHandlers(stream: NodeJS.EventEmitter & { destroy?: () => void }) { const decoder = new StringDecoder('utf8'); let buffer = ''; + let queuedBytes = 0; let sseProcessing = Promise.resolve(); + const failForBufferOverflow = () => { + this.activeSseStream = undefined; + stream.destroy?.(); + this.log.error( + `SSE event buffer exceeded the ${MAX_SSE_EVENT_BUFFER_BYTES}-byte limit. Reconnecting...`, + ); + this.scheduleReconnect(); + }; + stream.on('data', (chunk: Buffer) => { if (this.stopped || this.activeSseStream !== stream) { return; @@ -1158,9 +1191,19 @@ export class AgentClient { if (!decodedChunk) { return; } + const decodedBytes = Buffer.byteLength(decodedChunk, 'utf8'); + if ( + Buffer.byteLength(buffer, 'utf8') + queuedBytes + decodedBytes > + MAX_SSE_EVENT_BUFFER_BYTES + ) { + failForBufferOverflow(); + return; + } + queuedBytes += decodedBytes; sseProcessing = sseProcessing .then(async () => { + queuedBytes = Math.max(0, queuedBytes - decodedBytes); if (this.stopped || this.activeSseStream !== stream) { return; } @@ -1201,7 +1244,7 @@ export class AgentClient { method: 'get', url: `${this.baseUrl}/api/events`, responseType: 'stream', - ...this.buildRequestConfig('GET', '/api/events'), + ...this.buildAuthenticatedRequestConfig('GET', '/api/events'), }) .then((response) => { if (this.stopped) { @@ -1989,7 +2032,14 @@ export class AgentClient { await axios.post( `${this.baseUrl}${target}`, payload, - this.buildRequestConfig('POST', target, payload), + this.buildRequestConfig( + 'POST', + target, + payload, + REMOTE_UPDATE_TRIGGER_TYPES.has(triggerType) + ? AGENT_REQUEST_TIMEOUT_MS + : SYNCHRONOUS_REMOTE_TRIGGER_TIMEOUT_MS, + ), ); if (REMOTE_UPDATE_TRIGGER_TYPES.has(triggerType)) { this.markPendingFreshState(container.id); @@ -2024,7 +2074,14 @@ export class AgentClient { await axios.post( `${this.baseUrl}${target}`, body, - this.buildRequestConfig('POST', target, body), + this.buildRequestConfig( + 'POST', + target, + body, + REMOTE_UPDATE_TRIGGER_TYPES.has(triggerType) + ? AGENT_REQUEST_TIMEOUT_MS + : SYNCHRONOUS_REMOTE_TRIGGER_TIMEOUT_MS, + ), ); if (REMOTE_UPDATE_TRIGGER_TYPES.has(triggerType)) { containers.forEach(({ id }) => this.markPendingFreshState(id)); diff --git a/app/api/auth-lockout.test.ts b/app/api/auth-lockout.test.ts index ebc8c1b23..6a27adaaa 100644 --- a/app/api/auth-lockout.test.ts +++ b/app/api/auth-lockout.test.ts @@ -26,10 +26,22 @@ const { }; }); const LOCKOUT_TRACKED_IDENTITIES_CAP_FOR_TESTS = 5; -const { previousMaxTrackedLockoutIdentities } = vi.hoisted(() => { +const { + previousMaxTrackedLockoutIdentities, + previousAccountLockoutMaxAttempts, + previousMaxConcurrentLoginAttempts, +} = vi.hoisted(() => { const previous = process.env.DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES; + const previousAccountAttempts = process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS; + const previousConcurrentAttempts = process.env.DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS; process.env.DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES = '5'; - return { previousMaxTrackedLockoutIdentities: previous }; + process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS = '3workers'; + process.env.DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS = '2'; + return { + previousMaxTrackedLockoutIdentities: previous, + previousAccountLockoutMaxAttempts: previousAccountAttempts, + previousMaxConcurrentLoginAttempts: previousConcurrentAttempts, + }; }); const lockoutStateFiles = new Map(); @@ -112,10 +124,19 @@ describe('auth-lockout', () => { afterAll(() => { if (previousMaxTrackedLockoutIdentities === undefined) { delete process.env.DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES; - return; + } else { + process.env.DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES = previousMaxTrackedLockoutIdentities; + } + if (previousAccountLockoutMaxAttempts === undefined) { + delete process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS; + } else { + process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS = previousAccountLockoutMaxAttempts; + } + if (previousMaxConcurrentLoginAttempts === undefined) { + delete process.env.DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS; + } else { + process.env.DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS = previousMaxConcurrentLoginAttempts; } - - process.env.DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES = previousMaxTrackedLockoutIdentities; }); beforeEach(() => { @@ -184,6 +205,56 @@ describe('auth-lockout', () => { expect(mockSendErrorResponse).not.toHaveBeenCalled(); }); + test('releases login verification capacity when passport middleware throws synchronously', () => { + const passportError = new Error('passport middleware failed'); + mockPassportAuthenticate.mockImplementationOnce(() => () => { + throw passportError; + }); + const next = vi.fn(); + + expect(() => + authenticateLogin( + { body: { username: 'alice' }, ip: '203.0.113.11' } as any, + createResponse() as any, + next, + ), + ).not.toThrow(); + + expect(next).toHaveBeenCalledWith(passportError); + makePassportInvalidCredentials(); + authenticateLogin( + { body: { username: 'bob' }, ip: '203.0.113.12' } as any, + createResponse() as any, + vi.fn(), + ); + expect(mockPassportAuthenticate).toHaveBeenCalledTimes(2); + }); + + test('rejects excess concurrent login verifications before passport runs', () => { + const pendingCallbacks: Array<(error: unknown, user: false) => void> = []; + mockPassportAuthenticate.mockImplementation((_ids, _options, callback) => { + return () => pendingCallbacks.push(callback); + }); + const req = { body: { username: 'alice' }, ip: '203.0.113.11' } as any; + const rejectedResponse = createResponse(); + + authenticateLogin(req, createResponse() as any, vi.fn()); + authenticateLogin(req, createResponse() as any, vi.fn()); + authenticateLogin(req, rejectedResponse as any, vi.fn()); + + expect(mockPassportAuthenticate).toHaveBeenCalledTimes(2); + expect(rejectedResponse.setHeader).toHaveBeenCalledWith('Retry-After', '1'); + expect(mockSendErrorResponse).toHaveBeenCalledWith( + rejectedResponse, + 429, + 'Too many concurrent login attempts', + ); + + pendingCallbacks[0](null, false); + authenticateLogin(req, createResponse() as any, vi.fn()); + expect(mockPassportAuthenticate).toHaveBeenCalledTimes(3); + }); + test('locks account after repeated failures and sets Retry-After', () => { makePassportInvalidCredentials(); const req = { @@ -1829,20 +1900,8 @@ describe('auth-lockout', () => { vi.useRealTimers(); }); - test('parsePositiveIntegerEnv returns fallback when env value is invalid (block not empty)', () => { - // Line 92:48 BlockStatement {} mutant — if empty, always returns parsed (even invalid) - const previous = process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS; - process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS = 'invalid'; - - // We can only test this at module load time via testable_accountLockoutPolicy - // The policy was loaded at import — and we checked it was set with default - expect(testable_accountLockoutPolicy.maxAttempts).toBeGreaterThan(0); - - if (previous === undefined) { - delete process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS; - } else { - process.env.DD_AUTH_ACCOUNT_LOCKOUT_MAX_ATTEMPTS = previous; - } + test('parsePositiveIntegerEnv rejects a partially numeric env value', () => { + expect(testable_accountLockoutPolicy.maxAttempts).toBe(5); }); test('isLoginLockoutEntry returns false for non-object (candidate is object string not {})', () => { diff --git a/app/api/auth-lockout.ts b/app/api/auth-lockout.ts index 272bb4162..c5c43e18b 100644 --- a/app/api/auth-lockout.ts +++ b/app/api/auth-lockout.ts @@ -10,6 +10,7 @@ import { } from '../prometheus/auth.js'; import * as store from '../store/index.js'; import { getErrorMessage } from '../util/error.js'; +import { toPositiveInteger } from '../util/parse.js'; import { recordLoginAuditEvent } from './auth-audit.js'; import { getAllIds } from './auth-strategies.js'; import type { AuthRequest, UserWithUsername } from './auth-types.js'; @@ -33,10 +34,12 @@ const DEFAULT_LOCKOUT_WINDOW_MS = DEFAULT_LOCKOUT_WINDOW_MINUTES * MS_PER_MINUTE const DEFAULT_LOCKOUT_DURATION_MS = DEFAULT_LOCKOUT_DURATION_MINUTES * MS_PER_MINUTE; const DEFAULT_LOCKOUT_PRUNE_INTERVAL_MS = MS_PER_MINUTE; const DEFAULT_MAX_LOCKOUT_TRACKED_IDENTITIES = 5000; +const DEFAULT_MAX_CONCURRENT_LOGIN_ATTEMPTS = 2; const LOCKOUT_STATE_FILE_SUFFIX = '.auth-lockouts.json'; const LOCKOUT_STATE_PERSIST_DEBOUNCE_MS = 250; const LOGIN_LOCKOUT_ERROR_MESSAGE = 'Account temporarily locked due to repeated failed login attempts'; +const LOGIN_CONCURRENCY_ERROR_MESSAGE = 'Too many concurrent login attempts'; const LOCKOUT_ENTRY_NUMERIC_FIELDS: ReadonlyArray = [ 'failedAttempts', 'windowStartAt', @@ -67,6 +70,7 @@ const ipLoginLockouts = new Map(); let maintenanceTimer: ReturnType | undefined; let persistTimer: ReturnType | undefined; let persistenceInitialized = false; +let activeLoginAttempts = 0; function countActiveLockouts(lockouts: Map, now: number): number { let activeLockouts = 0; @@ -84,15 +88,7 @@ function updateLockoutGaugeTotals(now = Date.now()): void { } function parsePositiveIntegerEnv(name: string, fallback: number): number { - const raw = process.env[name]; - if (raw === undefined) { - return fallback; - } - const parsed = Number.parseInt(raw, 10); - if (!Number.isFinite(parsed) || parsed <= 0) { - return fallback; - } - return parsed; + return toPositiveInteger(process.env[name], fallback); } const accountLockoutPolicy: LoginLockoutPolicy = { @@ -120,6 +116,10 @@ const maxTrackedLockoutIdentities = parsePositiveIntegerEnv( 'DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES', DEFAULT_MAX_LOCKOUT_TRACKED_IDENTITIES, ); +const maxConcurrentLoginAttempts = parsePositiveIntegerEnv( + 'DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS', + DEFAULT_MAX_CONCURRENT_LOGIN_ATTEMPTS, +); function getLockoutStatePath(): string { const storeConfiguration = store.getConfiguration(); @@ -491,72 +491,94 @@ export function authenticateLogin(req: AuthRequest, res: Response, next: NextFun return; } - passport.authenticate( - getAllIds(), - { session: false }, - (error: unknown, user: UserWithUsername | false | null) => { - if (error) { - next(error); - return; - } + if (activeLoginAttempts >= maxConcurrentLoginAttempts) { + setRetryAfterHeader(res, 1); + recordLoginAuditEvent(req, 'error', LOGIN_CONCURRENCY_ERROR_MESSAGE, loginIdentity); + sendErrorResponse(res, 429, LOGIN_CONCURRENCY_ERROR_MESSAGE); + return; + } + + activeLoginAttempts += 1; + const finishAttempt = (): void => { + activeLoginAttempts = Math.max(0, activeLoginAttempts - 1); + }; - if (!user) { - const failedAt = Date.now(); - const accountLockoutAfterFailure = registerFailedLoginAttempt( - accountLoginLockouts, - accountLockoutPolicy, - accountLockoutKey, - failedAt, - ); - const ipLockoutAfterFailure = registerFailedLoginAttempt( - ipLoginLockouts, - ipLockoutPolicy, - ipLockoutKey, - failedAt, - ); - const lockoutUntil = Math.max(accountLockoutAfterFailure ?? 0, ipLockoutAfterFailure ?? 0); - if (lockoutUntil > failedAt) { - sendLockoutResponse(req, res, lockoutUntil, failedAt, loginIdentity); + try { + passport.authenticate( + getAllIds(), + { session: false }, + (error: unknown, user: UserWithUsername | false | null) => { + finishAttempt(); + if (error) { + next(error); return; } - recordLoginAuditEvent( - req, - 'error', - 'Authentication failed (invalid credentials)', - loginIdentity, - ); - sendUnauthorized(res); - return; - } - - clearLoginLockout(accountLoginLockouts, accountLockoutKey); - clearLoginLockout(ipLoginLockouts, ipLockoutKey); + if (!user) { + const failedAt = Date.now(); + const accountLockoutAfterFailure = registerFailedLoginAttempt( + accountLoginLockouts, + accountLockoutPolicy, + accountLockoutKey, + failedAt, + ); + const ipLockoutAfterFailure = registerFailedLoginAttempt( + ipLoginLockouts, + ipLockoutPolicy, + ipLockoutKey, + failedAt, + ); + const lockoutUntil = Math.max( + accountLockoutAfterFailure ?? 0, + ipLockoutAfterFailure ?? 0, + ); + if (lockoutUntil > failedAt) { + sendLockoutResponse(req, res, lockoutUntil, failedAt, loginIdentity); + return; + } + + recordLoginAuditEvent( + req, + 'error', + 'Authentication failed (invalid credentials)', + loginIdentity, + ); + sendUnauthorized(res); + return; + } - const continueWithUser = (authenticatedUser: UserWithUsername): void => { - req.user = authenticatedUser; - next(); - }; + clearLoginLockout(accountLoginLockouts, accountLockoutKey); + clearLoginLockout(ipLoginLockouts, ipLockoutKey); - if (typeof req.login !== 'function') { - continueWithUser(user); - return; - } + const continueWithUser = (authenticatedUser: UserWithUsername): void => { + req.user = authenticatedUser; + next(); + }; - req.login(user, { session: false }, (loginError: unknown) => { - if (loginError) { - next(loginError); + if (typeof req.login !== 'function') { + continueWithUser(user); return; } - continueWithUser(user); - }); - }, - )(req, res, next); + + req.login(user, { session: false }, (loginError: unknown) => { + if (loginError) { + next(loginError); + return; + } + continueWithUser(user); + }); + }, + )(req, res, next); + } catch (error: unknown) { + finishAttempt(); + next(error); + } } export function resetLoginLockoutStateForTests(): void { accountLoginLockouts.clear(); ipLoginLockouts.clear(); + activeLoginAttempts = 0; if (maintenanceTimer) { clearInterval(maintenanceTimer); maintenanceTimer = undefined; diff --git a/app/api/component.test.ts b/app/api/component.test.ts index 98bab4229..6cc297f62 100644 --- a/app/api/component.test.ts +++ b/app/api/component.test.ts @@ -115,6 +115,7 @@ describe('Component Router', () => { webhook: { url: 'https://hooks.example.com/path', }, + cmd: 'curl https://hooks.example.com/secret-token', mode: 'simple', })), }; @@ -126,6 +127,7 @@ describe('Component Router', () => { host: '[REDACTED]', }, webhook: '[REDACTED]', + cmd: '[REDACTED]', mode: 'simple', }); }); diff --git a/app/api/container/log-stream.test.ts b/app/api/container/log-stream.test.ts index aa3dba28f..b40f68519 100644 --- a/app/api/container/log-stream.test.ts +++ b/app/api/container/log-stream.test.ts @@ -115,6 +115,11 @@ describe('api/container/log-stream', () => { follow: true, }); }); + + test('bounds the requested tail for initial stream history', () => { + const query = parseContainerLogStreamQuery(new URLSearchParams({ tail: '999999999' })); + expect(query.tail).toBe(10_000); + }); }); describe('docker stream decoding', () => { @@ -1213,6 +1218,104 @@ describe('api/container/log-stream', () => { expect(dockerStream.destroy).toHaveBeenCalledTimes(1); }); + test('evicts a slow local viewer before sending more Docker log data', async () => { + const dockerStream = new EventEmitter() as EventEmitter & { + destroy: ReturnType; + }; + dockerStream.destroy = vi.fn(); + const ws = Object.assign(new EventEmitter(), { + send: vi.fn(), + close: vi.fn(), + bufferedAmount: 2 * 1024 * 1024, + }); + const gateway = createContainerLogStreamGateway({ + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'my-container', + watcher: 'local', + status: 'running', + })), + getWatchers: vi.fn(() => ({ + 'docker.local': { + dockerApi: { + getContainer: vi.fn(() => ({ logs: vi.fn().mockResolvedValue(dockerStream) })), + }, + }, + })), + sessionMiddleware: (req: any, _res: unknown, next: (error?: unknown) => void) => { + req.session = { passport: { user: '{"username":"alice"}' } }; + req.sessionID = 'session-1'; + next(); + }, + webSocketServer: { + handleUpgrade: vi.fn((_req, _socket, _head, callback: (socket: unknown) => void) => + callback(ws), + ), + }, + isRateLimited: vi.fn(() => false), + }); + + await gateway.handleUpgrade( + createUpgradeRequest('/api/v1/containers/c1/logs/stream') as any, + createUpgradeSocket() as any, + Buffer.alloc(0), + ); + dockerStream.emit('data', dockerFrame('2026-01-01T00:00:00.000000000Z too slow\n')); + + expect(ws.close).toHaveBeenCalledWith(1013, 'Log viewer is too slow'); + expect(ws.send).not.toHaveBeenCalled(); + expect(dockerStream.destroy).toHaveBeenCalledTimes(1); + }); + + test('evicts a local viewer before sending a single oversized serialized message', async () => { + const dockerStream = Object.assign(new EventEmitter(), { destroy: vi.fn() }); + const ws = Object.assign(new EventEmitter(), { + send: vi.fn(), + close: vi.fn(), + bufferedAmount: 0, + }); + const gateway = createContainerLogStreamGateway({ + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'my-container', + watcher: 'local', + status: 'running', + })), + getWatchers: vi.fn(() => ({ + 'docker.local': { + dockerApi: { + getContainer: vi.fn(() => ({ logs: vi.fn().mockResolvedValue(dockerStream) })), + }, + }, + })), + sessionMiddleware: (req: any, _res: unknown, next: (error?: unknown) => void) => { + req.session = { passport: { user: '{"username":"alice"}' } }; + req.sessionID = 'session-1'; + next(); + }, + webSocketServer: { + handleUpgrade: vi.fn((_req, _socket, _head, callback: (socket: unknown) => void) => + callback(ws), + ), + }, + isRateLimited: vi.fn(() => false), + }); + + await gateway.handleUpgrade( + createUpgradeRequest('/api/v1/containers/c1/logs/stream') as any, + createUpgradeSocket() as any, + Buffer.alloc(0), + ); + dockerStream.emit( + 'data', + dockerFrame(`2026-01-01T00:00:00.000000000Z ${'x'.repeat(1024 * 1024)}\n`), + ); + + expect(ws.close).toHaveBeenCalledWith(1013, 'Log viewer is too slow'); + expect(ws.send).not.toHaveBeenCalled(); + expect(dockerStream.destroy).toHaveBeenCalledTimes(1); + }); + test('does not throw when close fails during stream end', async () => { const dockerStream = new EventEmitter() as EventEmitter & { destroy: ReturnType; @@ -1732,6 +1835,44 @@ describe('api/container/log-stream', () => { expect(viewer.send).not.toHaveBeenCalled(); }); + test('evicts an edge viewer before sending a single oversized serialized message', async () => { + let handlers: + | { + onChunk: (chunk: { stream: 'stdout' | 'stderr'; logs: string }) => void; + } + | undefined; + const cancel = vi.fn(); + const streamContainerLogs = vi.fn( + ( + _containerId: string, + _options: Record, + nextHandlers: NonNullable, + ) => { + handlers = nextHandlers; + return { cancel }; + }, + ); + const viewer = Object.assign(new EventEmitter(), { + send: vi.fn(), + close: vi.fn(), + bufferedAmount: 0, + }); + const gateway = createAuthenticatedEdgeGateway(streamContainerLogs, viewer); + const handling = gateway.handleUpgrade( + createUpgradeRequest('/api/v1/containers/edge-container/logs/stream') as any, + createUpgradeSocket() as any, + Buffer.alloc(0), + ); + await vi.waitFor(() => expect(streamContainerLogs).toHaveBeenCalledTimes(1)); + + handlers?.onChunk({ stream: 'stdout', logs: `${'x'.repeat(1024 * 1024)}\n` }); + await handling; + + expect(cancel).toHaveBeenCalledTimes(1); + expect(viewer.close).toHaveBeenCalledWith(1013, 'Log viewer is too slow'); + expect(viewer.send).not.toHaveBeenCalled(); + }); + test('keeps an edge stream error within the WebSocket close-reason limit', async () => { let handlers: | { diff --git a/app/api/container/log-stream.ts b/app/api/container/log-stream.ts index c51a0754a..1bffd66b7 100644 --- a/app/api/container/log-stream.ts +++ b/app/api/container/log-stream.ts @@ -33,6 +33,7 @@ const RATE_LIMIT_MAX = 1000; const CLOSE_CODE_CONTAINER_NOT_RUNNING = 4001; const CLOSE_CODE_CONTAINER_NOT_FOUND = 4004; const MAX_VIEWER_BUFFER_BYTES = 1024 * 1024; +const MAX_CONTAINER_LOG_STREAM_TAIL = 10_000; const MAX_WEBSOCKET_CLOSE_REASON_BYTES = 123; type WebSocketLike = Pick & { @@ -147,10 +148,11 @@ function parseSinceParam(rawValue: string | null, fallback: number): number { export function parseContainerLogStreamQuery( query: URLSearchParams, ): ParsedContainerLogStreamQuery { + const requestedTail = parseIntegerParam(query.get('tail'), 100); return { stdout: parseBooleanParam(query.get('stdout'), true), stderr: parseBooleanParam(query.get('stderr'), true), - tail: parseIntegerParam(query.get('tail'), 100), + tail: Math.min(MAX_CONTAINER_LOG_STREAM_TAIL, requestedTail), since: parseSinceParam(query.get('since'), 0), follow: parseBooleanParam(query.get('follow'), true), }; @@ -327,18 +329,20 @@ function streamEdgeAgentLogsToWebSocket({ ) { continue; } - if ((webSocket.bufferedAmount ?? 0) > MAX_VIEWER_BUFFER_BYTES) { + const payload = JSON.stringify({ + ...message, + displayTs: formatLogDisplayTimestamp(message.ts), + }); + if ( + (webSocket.bufferedAmount ?? 0) + Buffer.byteLength(payload, 'utf8') > + MAX_VIEWER_BUFFER_BYTES + ) { webSocket.close(1013, 'Log viewer is too slow'); cleanup(true); return false; } try { - webSocket.send( - JSON.stringify({ - ...message, - displayTs: formatLogDisplayTimestamp(message.ts), - }), - ); + webSocket.send(payload); } catch { cleanup(true); return false; @@ -472,13 +476,19 @@ async function streamContainerLogsToWebSocket({ const emitMessages = (messages: DockerLogMessage[]): boolean => { for (const message of messages) { + const payload = JSON.stringify({ + ...message, + displayTs: formatLogDisplayTimestamp(message.ts), + }); + if ( + (webSocket.bufferedAmount ?? 0) + Buffer.byteLength(payload, 'utf8') > + MAX_VIEWER_BUFFER_BYTES + ) { + webSocket.close(1013, 'Log viewer is too slow'); + return false; + } try { - webSocket.send( - JSON.stringify({ - ...message, - displayTs: formatLogDisplayTimestamp(message.ts), - }), - ); + webSocket.send(payload); } catch { return false; } diff --git a/app/api/container/logs.test.ts b/app/api/container/logs.test.ts index 591768053..0339a6d01 100644 --- a/app/api/container/logs.test.ts +++ b/app/api/container/logs.test.ts @@ -1,3 +1,4 @@ +import { EventEmitter } from 'node:events'; import { describe, expect, test } from 'vitest'; import { createMockResponse } from '../../test/helpers.js'; import { @@ -7,6 +8,48 @@ import { parseContainerLogDownloadQuery, } from './logs.js'; +function createDockerLogFrame(payload: Buffer): Buffer { + const frame = Buffer.alloc(8 + payload.length); + frame[0] = 1; + frame.writeUInt32BE(payload.length, 4); + payload.copy(frame, 8); + return frame; +} + +function createStreamingLogHandler(dial: ReturnType) { + const response = createMockResponse(); + const handlers = createLogHandlers({ + storeContainer: { + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'streamed', + watcher: 'local', + status: 'running', + })), + }, + getAgent: vi.fn(() => undefined), + getWatchers: vi.fn(() => ({ + 'docker.local': { + dockerApi: { + getContainer: vi.fn(() => ({ + modem: { dial }, + logs: vi.fn(), + })), + }, + }, + })), + getErrorMessage: vi.fn(() => 'stream error'), + } as any); + return { + handle: () => + handlers.getContainerLogs( + { params: { id: 'c1' }, query: {}, headers: {} } as any, + response as any, + ), + response, + }; +} + describe('api/container/logs', () => { describe('isLocalDockerWatcherApi', () => { test('returns false for non-object values', () => { @@ -106,6 +149,11 @@ describe('api/container/logs', () => { timestamps: true, }); }); + + test('bounds tail to prevent unbounded log downloads', () => { + expect(parseContainerLogDownloadQuery({ tail: '-1' } as any).tail).toBe(0); + expect(parseContainerLogDownloadQuery({ tail: '999999999' } as any).tail).toBe(10_000); + }); }); describe('demuxDockerStream', () => { @@ -226,6 +274,35 @@ describe('api/container/logs', () => { expect(res.send).toHaveBeenCalledWith(''); }); + + test('rejects an agent log response above the download byte limit', async () => { + const handlers = createLogHandlers({ + storeContainer: { + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'test', + watcher: 'remote', + status: 'running', + agent: 'edge', + })), + }, + getAgent: vi.fn(() => ({ + getContainerLogs: vi.fn().mockResolvedValue('x'.repeat(16 * 1024 * 1024 + 1)), + })), + getWatchers: vi.fn(() => ({})), + getErrorMessage: vi.fn(() => 'error'), + } as any); + const res = createMockResponse(); + + await handlers.getContainerLogs( + { params: { id: 'c1' }, query: {}, headers: {} } as any, + res as any, + ); + + expect(res.status).toHaveBeenCalledWith(413); + expect(res.json).toHaveBeenCalledWith({ error: 'Container log download exceeds 16 MiB' }); + expect(res.send).not.toHaveBeenCalled(); + }); }); describe('download response headers', () => { @@ -269,5 +346,226 @@ describe('api/container/logs', () => { expect(res.setHeader).toHaveBeenCalledWith('Content-Encoding', 'gzip'); expect(res.send).toHaveBeenCalledWith(expect.any(Buffer)); }); + + test('rejects a local Docker response above the download byte limit before compression', async () => { + const handlers = createLogHandlers({ + storeContainer: { + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'noisy', + watcher: 'local', + status: 'running', + })), + }, + getAgent: vi.fn(() => undefined), + getWatchers: vi.fn(() => ({ + 'docker.local': { + dockerApi: { + getContainer: vi.fn(() => ({ + logs: vi.fn().mockResolvedValue(Buffer.alloc(16 * 1024 * 1024 + 1)), + })), + }, + }, + })), + getErrorMessage: vi.fn(() => 'error'), + } as any); + const res = createMockResponse(); + + await handlers.getContainerLogs( + { + params: { id: 'c1' }, + query: {}, + headers: { 'accept-encoding': 'gzip' }, + } as any, + res as any, + ); + + expect(res.status).toHaveBeenCalledWith(413); + expect(res.json).toHaveBeenCalledWith({ error: 'Container log download exceeds 16 MiB' }); + expect(res.send).not.toHaveBeenCalled(); + }); + + test('destroys the local Docker stream as soon as the download byte limit is exceeded', async () => { + const dockerStream = Object.assign(new EventEmitter(), { destroy: vi.fn() }); + const dial = vi.fn((options, callback) => callback(null, dockerStream)); + const logs = vi.fn(); + const handlers = createLogHandlers({ + storeContainer: { + getContainer: vi.fn(() => ({ + id: 'c1', + name: 'noisy/name', + watcher: 'local', + status: 'running', + })), + }, + getAgent: vi.fn(() => undefined), + getWatchers: vi.fn(() => ({ + 'docker.local': { + dockerApi: { + getContainer: vi.fn(() => ({ + id: 'noisy', + modem: { dial }, + logs, + })), + }, + }, + })), + getErrorMessage: vi.fn(() => 'error'), + } as any); + const res = createMockResponse(); + + const handling = handlers.getContainerLogs( + { params: { id: 'c1' }, query: {}, headers: {} } as any, + res as any, + ); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + dockerStream.emit('data', Buffer.alloc(8 * 1024 * 1024)); + dockerStream.emit('data', Buffer.alloc(8 * 1024 * 1024)); + dockerStream.emit('data', Buffer.alloc(1)); + dockerStream.emit('data', Buffer.from('ignored')); + dockerStream.emit('error', new Error('ignored after overflow')); + dockerStream.emit('end'); + await handling; + + expect(dial).toHaveBeenCalledWith( + expect.objectContaining({ + path: '/containers/noisy%2Fname/logs?', + method: 'GET', + isStream: true, + options: expect.objectContaining({ follow: false }), + }), + expect.any(Function), + ); + expect(dockerStream.destroy).toHaveBeenCalledOnce(); + expect(logs).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(413); + expect(res.send).not.toHaveBeenCalled(); + }); + + test('downloads a completed bounded local Docker stream', async () => { + const dockerStream = new EventEmitter(); + const dial = vi.fn((_options, callback) => callback(null, dockerStream)); + const { handle, response } = createStreamingLogHandler(dial); + + const handling = handle(); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + dockerStream.emit( + 'data', + new Uint8Array(createDockerLogFrame(Buffer.from('streamed line\n', 'utf8'))), + ); + dockerStream.emit('end'); + dockerStream.emit('close'); + await handling; + + expect(response.status).toHaveBeenCalledWith(200); + expect(response.send).toHaveBeenCalledWith('streamed line\n'); + }); + + test('handles an error from a bounded local Docker stream', async () => { + const dockerStream = new EventEmitter(); + const dial = vi.fn((_options, callback) => callback(null, dockerStream)); + const { handle, response } = createStreamingLogHandler(dial); + + const handling = handle(); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + dockerStream.emit('error', new Error('stream failed')); + await handling; + + expect(response.status).toHaveBeenCalledWith(500); + }); + + test('rejects when a bounded local Docker stream closes before completion', async () => { + const dockerStream = new EventEmitter(); + const dial = vi.fn((_options, callback) => callback(null, dockerStream)); + const { handle, response } = createStreamingLogHandler(dial); + + const handling = handle(); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + dockerStream.emit('close'); + + await vi.waitFor(() => expect(response.status).toHaveBeenCalledWith(500), { timeout: 100 }); + await handling; + }); + + test('destroys and rejects a stalled bounded local Docker stream', async () => { + vi.useFakeTimers(); + const previousTimeout = process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS; + process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS = '100'; + try { + const dockerStream = Object.assign(new EventEmitter(), { destroy: vi.fn() }); + const dial = vi.fn((_options, callback) => callback(null, dockerStream)); + const { handle, response } = createStreamingLogHandler(dial); + + const handling = handle(); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + await vi.advanceTimersByTimeAsync(100); + + expect(dockerStream.destroy).toHaveBeenCalledOnce(); + expect(response.status).toHaveBeenCalledWith(500); + await handling; + } finally { + if (previousTimeout === undefined) { + delete process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS; + } else { + process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS = previousTimeout; + } + vi.useRealTimers(); + } + }); + + test('rejects a stalled Docker modem dial and destroys a late stream', async () => { + vi.useFakeTimers(); + const previousTimeout = process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS; + process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS = '100'; + try { + let dialCallback: ((error: Error | null, value?: unknown) => void) | undefined; + const dial = vi.fn((_options, callback) => { + dialCallback = callback; + }); + const { handle, response } = createStreamingLogHandler(dial); + + const handling = handle(); + await vi.waitFor(() => expect(dial).toHaveBeenCalledTimes(1)); + await vi.advanceTimersByTimeAsync(100); + + expect(response.status).toHaveBeenCalledWith(500); + await handling; + + dialCallback?.(new Error('late dial error')); + const lateStream = Object.assign(new EventEmitter(), { destroy: vi.fn() }); + dialCallback?.(null, lateStream); + expect(lateStream.destroy).toHaveBeenCalledOnce(); + } finally { + if (previousTimeout === undefined) { + delete process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS; + } else { + process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS = previousTimeout; + } + vi.useRealTimers(); + } + }); + + test('handles a Docker modem dial error', async () => { + const { handle, response } = createStreamingLogHandler( + vi.fn((_options, callback) => callback(new Error('dial failed'))), + ); + + await handle(); + + expect(response.status).toHaveBeenCalledWith(500); + }); + + test.each([null, 'not a stream', {}])( + 'rejects an invalid Docker modem stream response %#', + async (invalidStream) => { + const { handle, response } = createStreamingLogHandler( + vi.fn((_options, callback) => callback(null, invalidStream)), + ); + + await handle(); + + expect(response.status).toHaveBeenCalledWith(500); + }, + ); }); }); diff --git a/app/api/container/logs.ts b/app/api/container/logs.ts index 61eb27d9a..69d6be477 100644 --- a/app/api/container/logs.ts +++ b/app/api/container/logs.ts @@ -1,6 +1,7 @@ import { gzipSync } from 'node:zlib'; import type { Request, Response } from 'express'; import type { AgentClient } from '../../agent/AgentClient.js'; +import { getOutboundHttpTimeoutMs } from '../../configuration/runtime-defaults.js'; import logger from '../../log/index.js'; import { sanitizeLogParam } from '../../log/sanitize.js'; import type { Container } from '../../model/container.js'; @@ -16,9 +17,29 @@ interface LogStoreContainerApi { } interface LocalDockerContainerApi { + modem?: { + dial: ( + options: { + path: string; + method: string; + isStream: boolean; + statusCodes: Record; + options: LocalDockerLogsOptions; + }, + callback: (error: unknown, stream?: unknown) => void, + ) => void; + }; logs: (options: LocalDockerLogsOptions) => Promise; } +interface LocalDockerLogStream { + destroy?: () => void; + on(event: 'data', listener: (chunk: Buffer | string | Uint8Array) => void): this; + on(event: 'end', listener: () => void): this; + on(event: 'close', listener: () => void): this; + on(event: 'error', listener: (error: Error) => void): this; +} + interface LocalDockerWatcherApi { dockerApi?: { getContainer: (containerName: string) => LocalDockerContainerApi; @@ -51,6 +72,11 @@ interface LogHandlerDependencies { const log = logger.child({ component: 'api-container-logs' }); const CONTAINER_LOG_ERROR_MESSAGE = 'Unable to fetch container logs'; +const MAX_CONTAINER_LOG_DOWNLOAD_LINES = 10_000; +const MAX_CONTAINER_LOG_DOWNLOAD_BYTES = 16 * 1024 * 1024; +const CONTAINER_LOG_TOO_LARGE_MESSAGE = 'Container log download exceeds 16 MiB'; + +class ContainerLogPayloadTooLargeError extends Error {} export function isLocalDockerWatcherApi(value: unknown): value is LocalDockerWatcherApi { if (!value || typeof value !== 'object') { @@ -104,15 +130,20 @@ function parseSinceQueryParam(rawValue: unknown, fallback: number): number { } export function parseContainerLogDownloadQuery(query: Request['query']): ParsedContainerLogQuery { + const requestedTail = parseIntegerQueryParam(query.tail, 1000); return { stdout: parseBooleanQueryParam(query.stdout, true), stderr: parseBooleanQueryParam(query.stderr, true), - tail: parseIntegerQueryParam(query.tail, 1000), + tail: Math.min(MAX_CONTAINER_LOG_DOWNLOAD_LINES, Math.max(0, requestedTail)), since: parseSinceQueryParam(query.since, 0), timestamps: parseBooleanQueryParam(query.timestamps, true), }; } +function isLogPayloadTooLarge(payload: Buffer | string | Uint8Array): boolean { + return Buffer.byteLength(payload) > MAX_CONTAINER_LOG_DOWNLOAD_BYTES; +} + function buildLocalDockerLogsOptions(query: ParsedContainerLogQuery): LocalDockerLogsOptions { return { stdout: query.stdout, @@ -124,6 +155,99 @@ function buildLocalDockerLogsOptions(query: ParsedContainerLogQuery): LocalDocke }; } +function isLocalDockerLogStream(value: unknown): value is LocalDockerLogStream { + return ( + !!value && typeof value === 'object' && typeof (value as { on?: unknown }).on === 'function' + ); +} + +async function getBoundedLocalDockerLogs( + dockerContainer: LocalDockerContainerApi, + containerId: string, + options: LocalDockerLogsOptions, +): Promise { + const modem = dockerContainer.modem; + if (!modem) { + return dockerContainer.logs(options); + } + + return new Promise((resolve, reject) => { + let settled = false; + let stream: LocalDockerLogStream | undefined; + const settle = (action: () => void) => { + if (settled) { + return; + } + settled = true; + clearTimeout(timeoutHandle); + action(); + }; + const handleTimeout = () => { + settle(() => reject(new Error('Docker log stream timed out'))); + stream?.destroy?.(); + }; + const timeoutHandle = setTimeout(handleTimeout, getOutboundHttpTimeoutMs()); + + modem.dial( + { + path: `/containers/${encodeURIComponent(containerId)}/logs?`, + method: 'GET', + isStream: true, + statusCodes: { + 200: true, + 404: 'no such container', + 500: 'server error', + }, + options, + }, + (error, value) => { + if (settled) { + if (isLocalDockerLogStream(value)) { + value.destroy?.(); + } + return; + } + if (error) { + settle(() => reject(error)); + return; + } + if (!isLocalDockerLogStream(value)) { + settle(() => reject(new Error('Docker log response is not a readable stream'))); + return; + } + + stream = value; + const chunks: Buffer[] = []; + let totalBytes = 0; + + value.on('data', (chunk) => { + if (settled) { + return; + } + const normalizedChunk = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); + totalBytes += normalizedChunk.length; + if (totalBytes > MAX_CONTAINER_LOG_DOWNLOAD_BYTES) { + settle(() => reject(new ContainerLogPayloadTooLargeError())); + value.destroy?.(); + return; + } + chunks.push(normalizedChunk); + timeoutHandle.refresh(); + }); + value.on('error', (streamError) => { + settle(() => reject(streamError)); + }); + value.on('end', () => { + settle(() => resolve(Buffer.concat(chunks, totalBytes))); + }); + value.on('close', () => { + settle(() => reject(new Error('Docker log stream closed before completion'))); + }); + }, + ); + }); +} + function resolveLocalDockerWatcher( container: Container, getWatchers: LogHandlerDependencies['getWatchers'], @@ -220,11 +344,16 @@ async function handleAgentContainerLogs({ since: query.since, timestamps: query.timestamps, }); + const logs = getAgentLogPayload(result); + if (isLogPayloadTooLarge(logs)) { + sendErrorResponse(res, 413, CONTAINER_LOG_TOO_LARGE_MESSAGE); + return true; + } sendLogDownloadResponse({ req, res, container, - logs: getAgentLogPayload(result), + logs, }); } catch (error: unknown) { log.warn(`Error fetching logs from agent (${sanitizeLogParam(getErrorMessage(error), 500)})`); @@ -257,12 +386,23 @@ async function handleLocalContainerLogs({ } try { - const logsBuffer = await watcher.dockerApi - .getContainer(container.name) - .logs(buildLocalDockerLogsOptions(query)); + const dockerContainer = watcher.dockerApi.getContainer(container.name); + const logsBuffer = await getBoundedLocalDockerLogs( + dockerContainer, + container.name, + buildLocalDockerLogsOptions(query), + ); + if (isLogPayloadTooLarge(logsBuffer)) { + sendErrorResponse(res, 413, CONTAINER_LOG_TOO_LARGE_MESSAGE); + return; + } const logs = demuxDockerStream(logsBuffer); sendLogDownloadResponse({ req, res, container, logs }); } catch (error: unknown) { + if (error instanceof ContainerLogPayloadTooLargeError) { + sendErrorResponse(res, 413, CONTAINER_LOG_TOO_LARGE_MESSAGE); + return; + } log.warn(`Error fetching container logs (${sanitizeLogParam(getErrorMessage(error), 500)})`); sendErrorResponse(res, 500, CONTAINER_LOG_ERROR_MESSAGE); } diff --git a/app/registries/Registry.test.ts b/app/registries/Registry.test.ts index ca4537cd5..0b2bef457 100644 --- a/app/registries/Registry.test.ts +++ b/app/registries/Registry.test.ts @@ -1991,6 +1991,41 @@ describe('callRegistry', () => { ); }); + test('should refuse redirects for registry data requests', async () => { + const { default: axios } = await import('axios'); + axios.mockResolvedValue({ data: {} }); + const registryMocked = createMockedRegistry(); + + await registryMocked.callRegistry({ + image: {}, + url: 'https://registry.example/v2/image/manifests/latest', + method: 'get', + }); + + expect(axios).toHaveBeenCalledWith(expect.objectContaining({ maxRedirects: 0 })); + }); + + test('should restore redirect refusal after authentication replaces request options', async () => { + const { default: axios } = await import('axios'); + axios.mockResolvedValue({ data: {} }); + axios.mockClear(); + const registryMocked = createMockedRegistry(); + vi.spyOn(registryMocked, 'authenticate').mockResolvedValue({ + url: 'https://registry.example/v2/image/manifests/latest', + method: 'get', + maxRedirects: 5, + }); + + await registryMocked.callRegistry({ + image: {}, + url: 'https://registry.example/v2/image/manifests/latest', + method: 'get', + }); + + expect(axios).toHaveBeenCalledTimes(1); + expect(axios.mock.calls[0][0]).toEqual(expect.objectContaining({ maxRedirects: 0 })); + }); + test('should use centralized outbound timeout when env override is set', async () => { const previousTimeout = process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS; process.env.DD_OUTBOUND_HTTP_TIMEOUT_MS = '2345'; @@ -2454,6 +2489,7 @@ describe('callRegistry', () => { expect(result).toEqual({ tags: ['v1'] }); // axios called twice: original + retry expect(axios).toHaveBeenCalledTimes(2); + expect(axios.mock.calls[1][0]).toEqual(expect.objectContaining({ maxRedirects: 0 })); // acquireToken called twice: once for each request expect(acquireToken).toHaveBeenCalledTimes(2); }); diff --git a/app/registries/Registry.ts b/app/registries/Registry.ts index 37ac88e48..097cef74c 100644 --- a/app/registries/Registry.ts +++ b/app/registries/Registry.ts @@ -503,6 +503,7 @@ class Registry< headers, responseType: 'json', timeout: getRegistryRequestTimeoutMs(), + maxRedirects: 0, }; const axiosOptionsWithAuth = await this.authenticate(image, axiosOptions); @@ -515,9 +516,13 @@ class Registry< /** Execute a single registry request and return the envelope. */ const executeRequest = async (requestOptions: RegistryRequestOptions) => { await acquireToken(getBucketForUrl(url)); + const redirectSafeRequestOptions = { + ...requestOptions, + maxRedirects: 0, + }; return withRetry( () => - axios(requestOptions).then((r) => ({ + axios(redirectSafeRequestOptions).then((r) => ({ status: r.status, headers: r.headers as Record, data: r.data, diff --git a/app/registry/trigger-config-redaction.ts b/app/registry/trigger-config-redaction.ts index 73f5f13fc..23bd14584 100644 --- a/app/registry/trigger-config-redaction.ts +++ b/app/registry/trigger-config-redaction.ts @@ -15,6 +15,7 @@ const TRIGGER_INFRASTRUCTURE_CONFIG_KEYS = new Set([ 'username', 'user', 'botusername', + 'cmd', ]); function isPlainObject(value: unknown): value is Record { diff --git a/app/triggers/hooks/HookRunner.test.ts b/app/triggers/hooks/HookRunner.test.ts index 821246801..76d65dc9e 100644 --- a/app/triggers/hooks/HookRunner.test.ts +++ b/app/triggers/hooks/HookRunner.test.ts @@ -1,8 +1,11 @@ import { afterAll, beforeEach, describe, expect, test, vi } from 'vitest'; import { resetAllowlistWarningStateForTests, runHook } from './HookRunner.js'; -var childProcessMockControl = vi.hoisted(() => ({ - execFileImpl: null as null | ((...args: unknown[]) => unknown), +var { childProcessMockControl, hookLogMock } = vi.hoisted(() => ({ + childProcessMockControl: { + execFileImpl: null as null | ((...args: unknown[]) => unknown), + }, + hookLogMock: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, })); vi.mock('node:child_process', async () => { @@ -22,7 +25,7 @@ vi.mock('node:child_process', async () => { vi.mock('../../log/index.js', () => ({ default: { - child: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }), + child: () => hookLogMock, }, })); @@ -30,6 +33,8 @@ describe('HookRunner', () => { const originalHooksEnabled = process.env.DD_HOOKS_ENABLED; beforeEach(() => { + vi.clearAllMocks(); + childProcessMockControl.execFileImpl = null; process.env.DD_HOOKS_ENABLED = 'true'; resetAllowlistWarningStateForTests(); }); @@ -79,6 +84,25 @@ describe('HookRunner', () => { expect(result.timedOut).toBe(false); }); + test('should not include the configured hook command in logs', async () => { + const secretCommand = 'curl https://hooks.example.com/secret-token'; + childProcessMockControl.execFileImpl = ( + _: string, + __: readonly string[], + ___: unknown, + callback: (...args: unknown[]) => void, + ) => { + setImmediate(() => callback(null, '', '')); + return { exitCode: 0 }; + }; + const result = await runHook(secretCommand, { label: 'post-update' }); + + expect(result.exitCode).toBe(0); + expect( + JSON.stringify([...hookLogMock.info.mock.calls, ...hookLogMock.warn.mock.calls]), + ).not.toContain(secretCommand); + }); + test('should capture non-zero exit code', async () => { var result = await runHook('exit 42', { label: 'test' }); expect(result.exitCode).toBe(42); diff --git a/app/triggers/hooks/HookRunner.ts b/app/triggers/hooks/HookRunner.ts index 105db9ccd..1245a330d 100644 --- a/app/triggers/hooks/HookRunner.ts +++ b/app/triggers/hooks/HookRunner.ts @@ -330,7 +330,7 @@ export async function runHook(command: string, options: HookRunnerOptions): Prom return result; } - hookLog.info(`Running ${options.label} hook: ${command}`); + hookLog.info(`Running ${options.label} hook`); return new Promise((resolve) => { let child: ReturnType | undefined; diff --git a/app/triggers/providers/command/Command.test.ts b/app/triggers/providers/command/Command.test.ts index d9e3cf8e0..afdadf4e6 100644 --- a/app/triggers/providers/command/Command.test.ts +++ b/app/triggers/providers/command/Command.test.ts @@ -116,7 +116,7 @@ test('should trigger with container', async () => { const container = { name: 'test', id: '123' }; await cmd.trigger(container); - expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Command echo test')); + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Command completed')); }); test('should trigger batch with containers', async () => { @@ -127,7 +127,7 @@ test('should trigger batch with containers', async () => { const containers = [{ name: 'test1' }, { name: 'test2' }]; await cmd.triggerBatch(containers); - expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Command echo batch')); + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Command completed')); }); test('should handle command execution error', async () => { @@ -140,7 +140,7 @@ test('should handle command execution error', async () => { const container = { name: 'test' }; await cmd.trigger(container); - expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('execution error')); + expect(logSpy).toHaveBeenCalledWith('Command execution failed'); }); test('runCommand should log execFile callback errors without rejecting', async () => { @@ -154,9 +154,7 @@ test('runCommand should log execFile callback errors without rejecting', async ( }); await expect(cmd.trigger({ name: 'test' })).resolves.toBeUndefined(); - expect(logSpy).toHaveBeenCalledWith( - expect.stringContaining('Command exit 1 \nexecution error (command failed)'), - ); + expect(logSpy).toHaveBeenCalledWith('Command execution failed'); }); test('should log stderr when present', async () => { @@ -172,6 +170,55 @@ test('should log stderr when present', async () => { expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('stderr')); }); +test('should not include child-process stdout or stderr in execution logs', async () => { + const outputSecret = 'configured-output-token'; + const cmd = new Command(); + await cmd.register('trigger', 'command', 'test', { cmd: 'echo test' }); + const infoSpy = vi.spyOn(cmd.log, 'info'); + const warnSpy = vi.spyOn(cmd.log, 'warn'); + childProcessMockControl.execFileImpl = createChildProcessCallbackMock({ + stdout: `stdout ${outputSecret}`, + stderr: `stderr ${outputSecret}`, + }); + + await cmd.trigger({ name: 'test' }); + + expect(JSON.stringify([...infoSpy.mock.calls, ...warnSpy.mock.calls])).not.toContain( + outputSecret, + ); + expect(infoSpy).toHaveBeenCalledWith('Command completed with stdout'); + expect(warnSpy).toHaveBeenCalledWith('Command completed with stderr'); +}); + +test('should not include the configured command in execution logs', async () => { + const secretCommand = 'curl https://hooks.example.com/secret-token'; + const cmd = new Command(); + await cmd.register('trigger', 'command', 'test', { cmd: secretCommand }); + const infoSpy = vi.spyOn(cmd.log, 'info'); + const warnSpy = vi.spyOn(cmd.log, 'warn'); + childProcessMockControl.execFileImpl = createChildProcessCallbackMock({ stdout: 'ok' }); + + await cmd.trigger({ name: 'test' }); + + expect(JSON.stringify([...infoSpy.mock.calls, ...warnSpy.mock.calls])).not.toContain( + secretCommand, + ); +}); + +test('should not include a failed configured command in execution logs', async () => { + const secretCommand = 'curl https://hooks.example.com/secret-token'; + const cmd = new Command(); + await cmd.register('trigger', 'command', 'test', { cmd: secretCommand }); + const warnSpy = vi.spyOn(cmd.log, 'warn'); + childProcessMockControl.execFileImpl = createChildProcessCallbackMock({ + error: new Error(`Command failed: /bin/sh -c ${secretCommand}`), + }); + + await cmd.trigger({ name: 'test' }); + + expect(JSON.stringify(warnSpy.mock.calls)).not.toContain(secretCommand); +}); + test('runCommand should use execFile with shell and -c arguments', async () => { childProcessMockControl.execImpl = ( _: unknown, diff --git a/app/triggers/providers/command/Command.ts b/app/triggers/providers/command/Command.ts index e3818713c..63e29e647 100644 --- a/app/triggers/providers/command/Command.ts +++ b/app/triggers/providers/command/Command.ts @@ -211,13 +211,13 @@ class Command extends Trigger { }, ); if (stdout) { - this.log.info(`Command ${this.configuration.cmd} \nstdout ${stdout}`); + this.log.info('Command completed with stdout'); } if (stderr) { - this.log.warn(`Command ${this.configuration.cmd} \nstderr ${stderr}`); + this.log.warn('Command completed with stderr'); } - } catch (err) { - this.log.warn(`Command ${this.configuration.cmd} \nexecution error (${err.message})`); + } catch { + this.log.warn('Command execution failed'); } } } diff --git a/content/docs/current/api/container.mdx b/content/docs/current/api/container.mdx index 866683ae9..c0ac609b3 100644 --- a/content/docs/current/api/container.mdx +++ b/content/docs/current/api/container.mdx @@ -592,11 +592,11 @@ curl "http://drydock:3000/api/v1/containers/{id}/logs?tail=1000&stdout=true&stde | --- | --- | --- | --- | | `stdout` | boolean | `true` | Include stdout output | | `stderr` | boolean | `true` | Include stderr output | -| `tail` | integer | `1000` | Number of lines to return from the end of the log | +| `tail` | integer | `1000` | Number of lines to return from the end of the log (`0` to `10000`) | | `since` | string | `0` | Unix timestamp (seconds) or ISO 8601 string — only return logs after this time | | `timestamps` | boolean | `true` | Include timestamps in output | -Returns 200 with the log text (Content-Disposition: attachment), 404 if the container is not found, or 500 on failure. +Returns 200 with the log text (Content-Disposition: attachment), 404 if the container is not found, 413 if the log payload exceeds 16 MiB, or 500 on failure. ## Stream container logs (WebSocket) diff --git a/content/docs/current/configuration/authentications/index.mdx b/content/docs/current/configuration/authentications/index.mdx index d76e74ce1..1071029bd 100644 --- a/content/docs/current/configuration/authentications/index.mdx +++ b/content/docs/current/configuration/authentications/index.mdx @@ -24,6 +24,7 @@ To limit brute-force attempts on `POST /auth/login`, drydock tracks failed login | `DD_AUTH_LOCKOUT_DURATION_MS` | ⚪ | Lockout duration (milliseconds) once a threshold is reached | Positive integer (`> 0`) | `900000` | | `DD_AUTH_LOCKOUT_PRUNE_INTERVAL_MS` | ⚪ | Interval for pruning expired lockout entries from memory and the persisted lockout sidecar file | Positive integer (`> 0`) | `60000` | | `DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES` | ⚪ | Maximum number of unique account/IP identities tracked before older entries are pruned | Positive integer (`> 0`) | `5000` | +| `DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS` | ⚪ | Maximum login credential verifications running at once. Excess attempts receive `429` with `Retry-After: 1` before password hashing starts. | Positive integer (`> 0`) | `2` | If a lockout value is missing or invalid, drydock falls back to the corresponding default. diff --git a/content/docs/current/configuration/hooks/index.mdx b/content/docs/current/configuration/hooks/index.mdx index 32b6fe4d8..ac053d3e7 100644 --- a/content/docs/current/configuration/hooks/index.mdx +++ b/content/docs/current/configuration/hooks/index.mdx @@ -121,11 +121,13 @@ services: labels: - dd.watch=true - dd.hook.pre=docker exec mydb pg_dump -U postgres -F c -f /backup/pre-update.dump mydb - - dd.hook.post=curl -X POST https://hooks.slack.com/services/xxx -d '{"text":"myapp updated"}' + - dd.hook.post=/usr/local/bin/notify-update.sh - dd.hook.pre.abort=true - dd.hook.timeout=120000 ``` Commands that require shell redirects (`>`, `|`, etc.) must be wrapped in a script file and invoked via the hook (e.g. `dd.hook.pre=/usr/local/bin/backup.sh`). +Keep credentials out of hook labels. Put notification tokens and signed URLs in a mounted secret or environment variable read by the script instead. + Hooks run with the permissions of the drydock process. Ensure the Docker socket is mounted if your hooks need to interact with Docker. diff --git a/content/docs/current/configuration/server/index.mdx b/content/docs/current/configuration/server/index.mdx index 07e819b8c..e7a71730c 100644 --- a/content/docs/current/configuration/server/index.mdx +++ b/content/docs/current/configuration/server/index.mdx @@ -37,6 +37,7 @@ import { Tab, Tabs } from 'fumadocs-ui/components/tabs'; | `DD_SERVER_RATELIMIT_MAX` | ⚪ | Maximum requests accepted by the outer API limiter in each 15-minute window | integer (`>0`) | `1000` | | `DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES` | ⚪ | Maximum number of unique client identities held in the auth lockout state. Older entries are pruned when the cap is reached. | integer (`>0`) | `5000` | | `DD_AUTH_LOCKOUT_PRUNE_INTERVAL_MS` | ⚪ | Interval for pruning expired auth lockout entries from memory and the persisted lockout sidecar file | integer (`>0`) | `60000` | +| `DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS` | ⚪ | Maximum login credential verifications running at once. Excess attempts receive `429` before password hashing starts. | integer (`>0`) | `2` | | `DD_SSE_MAX_CLIENTS` | ⚪ | Maximum total concurrent SSE connections across all sessions (global cap; individual sessions are also capped at 10). Connections beyond this limit receive a `429 Too Many Requests` response. | integer (`>0`) | `500` | | `DD_RUN_AS_ROOT` | ⚪ | Request break-glass root mode (requires `DD_ALLOW_INSECURE_ROOT=true`) | `true`, `false` | `false` | | `DD_ALLOW_INSECURE_ROOT` | ⚪ | Explicit acknowledgment for break-glass root mode | `true`, `false` | `false` | diff --git a/content/docs/current/monitoring/index.mdx b/content/docs/current/monitoring/index.mdx index 376c03108..57159678f 100644 --- a/content/docs/current/monitoring/index.mdx +++ b/content/docs/current/monitoring/index.mdx @@ -274,6 +274,7 @@ Set the "Login outcomes" panel to use the `outcome` label as the legend and colo | `DD_AUTH_LOCKOUT_DURATION_MS` | `900000` (15 min) | lockouts are too disruptive for operators | attackers resume immediately after lock expires | | `DD_AUTH_LOCKOUT_PRUNE_INTERVAL_MS` | `60000` (1 min) | very high login volume makes pruning too frequent | expired lockout state lingers too long for your memory budget | | `DD_AUTH_LOCKOUT_MAX_TRACKED_IDENTITIES` | `5000` | many unique client IPs exist (e.g. egress across many subnets) | memory pressure from lockout state storage is a concern | +| `DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS` | `2` | legitimate login bursts receive `429` responses | password hashing causes excessive CPU or memory pressure | Operational notes: diff --git a/security_best_practices_report.md b/security_best_practices_report.md new file mode 100644 index 000000000..cd6e81669 --- /dev/null +++ b/security_best_practices_report.md @@ -0,0 +1,154 @@ +# Drydock Security Best-Practices Review + +Date: 2026-08-13 + +Baseline: `7ea69aa86b26296fe70a665de816cb9886c4f2d8` on `dev/v1.7` + +Scope: Express backend, Vue dashboard, Next.js website, authentication and sessions, registry and agent transports, container operations, WebSockets, triggers and hooks, filesystem and subprocess boundaries, Docker image, dependencies, and GitHub Actions. + +## Executive summary + +This review found one High and five Medium security issues. All six are remediated in the reviewed change with regression coverage. + +| ID | Severity | Finding | Status | +| --- | --- | --- | --- | +| DD-SEC-001 | High | Concurrent login requests could start unbounded expensive password verification before lockout accounting | Remediated | +| DD-SEC-002 | Medium | Standard agent requests and unterminated SSE events had no resource bounds | Remediated | +| DD-SEC-003 | Medium | Container log downloads accepted unbounded history and compressed fully buffered output | Remediated | +| DD-SEC-004 | Medium | Local Docker log WebSockets lacked the slow-viewer backpressure guard used by remote streams | Remediated | +| DD-SEC-005 | Medium | Registry data requests followed redirects despite the documented refusal policy | Remediated | +| DD-SEC-006 | Medium | Command and hook strings could expose literal credentials through APIs and logs | Remediated | + +No reachable dependency vulnerability or committed credential was confirmed. Existing residual risks that are already stated in `SECURITY-ASSURANCE.md`, including operator-authorized shell execution and outbound response-size limits that vary by integration, remain deployment considerations rather than new findings from this pass. + +## Methodology and validation + +The review combined source tracing, test-first remediation, and repository scanners. It covered authentication, authorization, CSRF and origin enforcement, request parsing, outbound HTTP, registry authentication, agent transports, WebSockets, container log handling, subprocess execution, browser security policy, filesystem confinement, CI permissions, action pinning, dependencies, and container configuration. + +Checks performed on 2026-08-13 included: + +- production `npm audit` for the root, `app`, `ui`, `e2e`, `apps/demo`, and `apps/web` lockfiles, all with zero findings +- Gitleaks across the complete Git history, with no credential confirmed +- Grype over tracked source with no finding +- Trivy vulnerability and misconfiguration scans with no finding after the repository policy; secret results were placeholder Slack URLs in versioned documentation, and the Docker `USER` result was the documented runtime `su-exec` privilege drop +- Actionlint with no finding +- Zizmor in normal and pedantic modes; no high-severity workflow issue was found, and the medium Scorecard permission result is intentional +- Semgrep with 114 JavaScript/TypeScript and Docker rules; the reported Docker, test-only TLS, documentation, and Renovate results were false positives. Some test files timed out or did not parse, so this was not treated as a complete clean scan +- the repository Qlty gate, which completed successfully with one non-blocking comment note +- backend security tests, UI security tests, website security scripts, focused affected-path tests, TypeScript builds, and repository pre-push verification + +## Findings + +### DD-SEC-001: Concurrent password verification could exhaust CPU and memory + +- Status: Remediated on 2026-08-13 +- Severity: High +- Category: Authentication resource exhaustion +- CWE: CWE-400, Uncontrolled Resource Consumption +- Affected code: `app/api/auth-lockout.ts`, `app/authentications/providers/basic/Basic.ts` + +#### Evidence and impact + +The login lockout checked completed failures before Passport ran, then recorded a failure only after password verification returned. Concurrent requests could therefore all pass the initial lockout check and begin Argon2 verification before any request incremented the failure count. Accepted Argon2 hashes may request substantial memory, and username mismatches intentionally perform the same derivation to prevent username enumeration. + +An unauthenticated burst immediately after startup could queue many CPU- and memory-intensive derivations, delaying legitimate authentication or exhausting the controller. The route-level request-rate limit limited request count over time but did not cap concurrent expensive work. + +#### Remediation + +`authenticateLogin` now reserves one of two global verification slots before invoking Passport. Excess attempts receive HTTP 429 and `Retry-After: 1` before password hashing starts. `DD_AUTH_MAX_CONCURRENT_LOGIN_ATTEMPTS` permits a positive operator override. The slot is released when Passport completes, including authentication failures. See `app/api/auth-lockout.ts:36`, `app/api/auth-lockout.ts:126-129`, and `app/api/auth-lockout.ts:501-522`. + +The regression test holds two Passport callbacks open, proves a third request never reaches Passport, then completes one callback and proves capacity is released. + +### DD-SEC-002: Standard agent transport had unbounded requests and SSE fragments + +- Status: Remediated on 2026-08-13 +- Severity: Medium +- Category: Outbound transport resource exhaustion +- CWE: CWE-400, Uncontrolled Resource Consumption +- Affected code: `app/agent/AgentClient.ts` + +#### Evidence and impact + +Ordinary agent inventory, watcher, trigger, log, and action requests did not set a timeout, response limit, request-body limit, or redirect refusal. The long-lived SSE parser also retained an incomplete event until a blank-line delimiter arrived, with no maximum size. + +A configured, reachable malicious or compromised agent could leave controller operations open, return very large JSON, redirect a request carrying the custom agent credential, or grow one unterminated SSE event until the controller ran out of memory. This was not an unauthenticated internet path. Operators choose the agent endpoint, and shared-secret HTTP already requires an explicit insecure override. + +#### Remediation + +Ordinary requests now use a 30-second timeout, 16 MiB request and response limits, and `maxRedirects: 0`. Both token and Ed25519 modes use the same bounds without changing the exact signed request target. The SSE connection remains intentionally long-lived, but refuses redirects and destroys/reconnects when an incomplete event exceeds 16 MiB. See `app/agent/AgentClient.ts:401-417`, `app/agent/AgentClient.ts:462-500`, and `app/agent/AgentClient.ts:1165-1194`. + +### DD-SEC-003: Container log downloads accepted unbounded history + +- Status: Remediated on 2026-08-13 +- Severity: Medium +- Category: Authenticated resource exhaustion +- CWE: CWE-400, Uncontrolled Resource Consumption +- Affected code: `app/api/container/logs.ts` + +#### Evidence and impact + +The authenticated download endpoint forwarded an arbitrary `tail`, fully materialized local or agent log output, converted it to text, and optionally called synchronous `gzipSync`. One request for a large history could consume multiple full-size buffers and block the Node event loop. Explicit anonymous mode extended the same path to unauthenticated clients. + +#### Remediation + +The endpoint clamps `tail` to 0 through 10,000 lines and rejects materialized output over 16 MiB with HTTP 413 before text demultiplexing or gzip. Agent requests are independently capped at the transport layer by DD-SEC-002. See `app/api/container/logs.ts:109-121`, `app/api/container/logs.ts:226-241`, and `app/api/container/logs.ts:272-281`. + +The local Docker client still materializes its bounded line result before the byte check. Docker log-driver retention and maximum-line policy remain operational controls for unusually large individual log records. + +### DD-SEC-004: Local log WebSockets lacked slow-viewer backpressure + +- Status: Remediated on 2026-08-13 +- Severity: Medium +- Category: WebSocket resource exhaustion +- CWE: CWE-400, Uncontrolled Resource Consumption +- Affected code: `app/api/container/log-stream.ts` + +#### Evidence and impact + +Edge-agent and system-log streams already closed slow viewers when their WebSocket send queue crossed a fixed budget. The local Docker path continued reading and sending without checking `bufferedAmount`. A slow authenticated viewer following a noisy local container could grow the WebSocket queue until memory was exhausted. + +#### Remediation + +The local path now applies the same 1 MiB viewer-buffer limit before every message, closes a slow viewer with code 1013, and destroys the Docker stream during cleanup. Initial history is capped at 10,000 lines. See `app/api/container/log-stream.ts:35-37`, `app/api/container/log-stream.ts:148-158`, and `app/api/container/log-stream.ts:475-529`. + +### DD-SEC-005: Registry data requests followed redirects + +- Status: Remediated on 2026-08-13 +- Severity: Medium +- Category: Server-side request forgery and credential-boundary drift +- CWE: CWE-918, Server-Side Request Forgery +- Affected code: `app/registries/Registry.ts` + +#### Evidence and impact + +Bearer-token acquisition explicitly refused redirects, and `SECURITY-ASSURANCE.md` stated that registry manifests did too. The central registry data request did not set `maxRedirects`, so Axios used its redirect-following default for manifests, tags, and blobs. + +A malicious or compromised configured registry could redirect the controller to an unintended network target. Redirect-library header stripping reduced some credential-forwarding risk but did not remove the SSRF and availability impact. + +#### Remediation + +The central registry request sets `maxRedirects: 0`; all authenticated registry data calls inherit it. See `app/registries/Registry.ts:499-509`. A regression test asserts the actual Axios request options, so the assurance claim is pinned to the behavior rather than a hand-maintained expected value. + +### DD-SEC-006: Command and hook strings could expose literal credentials + +- Status: Remediated on 2026-08-13 +- Severity: Medium +- Category: Sensitive information exposure through logs and API responses +- CWE: CWE-532, Insertion of Sensitive Information into Log File +- Affected code: `app/registry/trigger-config-redaction.ts`, `app/triggers/providers/command/Command.ts`, `app/triggers/hooks/HookRunner.ts` + +#### Evidence and impact + +Command trigger configuration returned `cmd` through the authenticated component API and logged it during registration. Successful, failed, and stderr-producing command executions logged the full command again. Hook execution logged the full label-provided command. The documentation included a webhook-bearing hook directly in a container label. + +If an operator embedded a token, signed URL, or password in one of those command strings, it became visible to API readers, stdout collectors, and the in-memory log API. + +#### Remediation + +Trigger configuration now treats `cmd` as sensitive and returns `[REDACTED]`; command completion and failure logs no longer include the configured command or the child-process error text that can echo it; hook startup logs include only the hook label. The documentation now uses a script and tells operators to read credentials from a mounted secret or environment variable. See `app/registry/trigger-config-redaction.ts:1-19`, `app/triggers/providers/command/Command.ts:213-220`, and `app/triggers/hooks/HookRunner.ts:321-343`. + +Regression tests use a canary URL and prove it does not appear in API output, successful or failed command logs, or hook startup logs. + +## No-findings areas + +The review did not identify an additional confirmed defect in Express session handling, CSRF and mutation content-type enforcement, CORS, webhook raw-body HMAC verification, WebSocket upgrade origin and authentication checks, Vue/Next.js XSS sinks and CSP, filesystem path confinement, release signing, or GitHub Actions trust boundaries. This statement is limited to the reviewed baseline and checks above; it is not a guarantee that no defect exists.