Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
98 changes: 97 additions & 1 deletion app/agent/AgentClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>((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 });
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 }),
);
});

Expand All @@ -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',
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -7912,6 +8000,14 @@ describe('AgentClient', () => {
'/api/containers/cid/logs?tail=100&since=0&timestamps=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 () => {
Expand Down
71 changes: 64 additions & 7 deletions app/agent/AgentClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (!this.ed25519PrivateKey || !this.config.signingkeyid) {
return this.axiosOptions;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down
Loading
Loading