fix(SEC-REVIEW-4-7): Address remaining MEDIUM security review findings
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
- Graceful container shutdown: detect "not running" containers and skip force-remove escalation, only SIGKILL for genuine stop failures - data: URI stripping: add security audit logging via NestJS Logger when data: URIs are blocked in markdown links and images - Orchestrator bootstrap: replace void bootstrap() with .catch() handler for clear startup failure logging and clean process.exit(1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -124,6 +124,7 @@ const SANITIZE_OPTIONS: sanitizeHtml.IOptions = {
|
||||
const href = attribs.href;
|
||||
// Strip data: URI scheme from links
|
||||
if (href?.trim().toLowerCase().startsWith("data:")) {
|
||||
logger.warn(`Blocked data: URI in link href`);
|
||||
const { href: _removed, ...safeAttribs } = attribs;
|
||||
return {
|
||||
tagName,
|
||||
@@ -149,6 +150,7 @@ const SANITIZE_OPTIONS: sanitizeHtml.IOptions = {
|
||||
img: (tagName: string, attribs: sanitizeHtml.Attributes) => {
|
||||
const src = attribs.src;
|
||||
if (src?.trim().toLowerCase().startsWith("data:")) {
|
||||
logger.warn(`Blocked data: URI in image src`);
|
||||
const { src: _removed, ...safeAttribs } = attribs;
|
||||
return {
|
||||
tagName,
|
||||
|
||||
@@ -17,4 +17,7 @@ async function bootstrap() {
|
||||
logger.log(`🚀 Orchestrator running on http://${host}:${String(port)}`);
|
||||
}
|
||||
|
||||
void bootstrap();
|
||||
bootstrap().catch((err: unknown) => {
|
||||
logger.error("Failed to start orchestrator", err instanceof Error ? err.stack : String(err));
|
||||
process.exit(1);
|
||||
});
|
||||
|
||||
@@ -243,11 +243,25 @@ describe("DockerSandboxService", () => {
|
||||
expect(mockContainer.remove).toHaveBeenCalledWith({ force: false });
|
||||
});
|
||||
|
||||
it("should fall back to force remove when graceful stop fails", async () => {
|
||||
it("should remove without force when container is not running", async () => {
|
||||
const containerId = "container-123";
|
||||
|
||||
(mockContainer.stop as ReturnType<typeof vi.fn>).mockRejectedValueOnce(
|
||||
new Error("Container already stopped")
|
||||
new Error("container is not running")
|
||||
);
|
||||
|
||||
await service.removeContainer(containerId);
|
||||
|
||||
expect(mockContainer.stop).toHaveBeenCalledWith({ t: 10 });
|
||||
// Not-running containers are removed without force, no escalation needed
|
||||
expect(mockContainer.remove).toHaveBeenCalledWith({ force: false });
|
||||
});
|
||||
|
||||
it("should fall back to force remove when graceful stop fails with unknown error", async () => {
|
||||
const containerId = "container-123";
|
||||
|
||||
(mockContainer.stop as ReturnType<typeof vi.fn>).mockRejectedValueOnce(
|
||||
new Error("Connection timeout")
|
||||
);
|
||||
|
||||
await service.removeContainer(containerId);
|
||||
|
||||
@@ -361,8 +361,17 @@ export class DockerSandboxService {
|
||||
this.logger.log(`Container gracefully stopped and removed: ${containerId}`);
|
||||
return;
|
||||
} catch (gracefulError) {
|
||||
const errMsg = gracefulError instanceof Error ? gracefulError.message : String(gracefulError);
|
||||
|
||||
// If container is already stopped, just remove without force
|
||||
if (errMsg.includes("is not running") || errMsg.includes("304")) {
|
||||
this.logger.log(`Container ${containerId} already stopped, removing without force`);
|
||||
await container.remove({ force: false });
|
||||
return;
|
||||
}
|
||||
|
||||
this.logger.warn(
|
||||
`Graceful stop failed for container ${containerId}, falling back to force remove: ${gracefulError instanceof Error ? gracefulError.message : String(gracefulError)}`
|
||||
`Graceful stop failed for container ${containerId}, falling back to force remove: ${errMsg}`
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user