diff --git a/.changeset/four-quiet-rules.md b/.changeset/four-quiet-rules.md new file mode 100644 index 0000000..7e45559 --- /dev/null +++ b/.changeset/four-quiet-rules.md @@ -0,0 +1,12 @@ +--- +"@docker-doctor/cli": patch +--- + +Fix four rule-specificity bugs in the Dockerfile rule engine: + +- `no-root-user` now resets its tracked user at each `FROM` (build stage), instead of letting a `USER` set in an earlier stage (e.g. a builder) silently satisfy the check for a later stage that never sets one. Multi-stage Dockerfiles may now surface a new diagnostic here. +- `no-add-remote`, `prefer-copy-over-add`, and `use-dockerignore` now read the first non-flag operand of `ADD`/`COPY` instead of always taking the first whitespace-separated token, so a leading flag like `--chown` or `--from` no longer silently disables the check (e.g. `ADD --chown=node:node https://...` now correctly trips `no-add-remote`). As a side effect, `use-dockerignore` now explicitly skips `COPY --from=` since that never reads the build context. These fixes may surface new diagnostics. +- `no-secrets-in-env` no longer matches secret keywords as an unanchored substring, so keys like `AUTHOR` or `OAUTH_ISSUER_URL` no longer false-positive as "potential secret found" (this is the only error-severity rule, so this previously failed CI on Dockerfiles with no secrets). Genuine segment matches like `DB_PASSWORD` still match. This removes diagnostics. +- `order-layers` now resets its copy-tracking at each build stage, so a copy-everything in an earlier stage no longer implicates an install command in a later, correctly-ordered stage, and no longer substring-matches `"src"` anywhere in a path (e.g. `/usr/src/lib`, `mysrcdir`). This removes diagnostics. + +These are bug fixes to rule specificity; expect docker-doctor scores to move on existing Dockerfiles as false negatives are corrected and false positives are removed. diff --git a/packages/core/src/rules/best-practices.ts b/packages/core/src/rules/best-practices.ts index 3b92d04..410b218 100644 --- a/packages/core/src/rules/best-practices.ts +++ b/packages/core/src/rules/best-practices.ts @@ -52,7 +52,11 @@ export const preferCopyOverAdd: DockerfileRule = { for (const inst of instructions) { if (inst.instruction === "ADD") { const parts = inst.args.split(/\s+/u); - const [src] = parts; + const src = parts.find((p) => !p.startsWith("--")); + + if (!src) { + continue; + } // If it's not a remote url (handled by security/no-add-remote) and not a compressed file const isRemote = diff --git a/packages/core/src/rules/performance.ts b/packages/core/src/rules/performance.ts index 1fb65a6..ca100db 100644 --- a/packages/core/src/rules/performance.ts +++ b/packages/core/src/rules/performance.ts @@ -55,6 +55,10 @@ export const orderLayers: DockerfileRule = { let copyAllLine = -1; for (const inst of instructions) { + if (inst.instruction === "FROM") { + copyAllLine = -1; + } + if (inst.instruction === "COPY" || inst.instruction === "ADD") { const parts = inst.args.split(/\s+/u); const src = parts.find((p) => !p.startsWith("--")); @@ -63,10 +67,15 @@ export const orderLayers: DockerfileRule = { continue; } - if ( - (src === "." || src === "./" || src === "*" || src.includes("src")) && - copyAllLine === -1 - ) { + const normalized = src.replace(/^\.\//u, ""); + const isCopyAll = + normalized === "." || + normalized === "" || + normalized === "*" || + normalized === "src" || + normalized.startsWith("src/"); + + if (isCopyAll && copyAllLine === -1) { copyAllLine = inst.line; } } @@ -160,8 +169,18 @@ export const useDockerignore: DockerfileRule = { // If copying everything, we definitely need .dockerignore const hasCopyAll = instructions.some((inst) => { if (inst.instruction === "COPY" || inst.instruction === "ADD") { + // COPY --from= reads from a previous build stage, not the + // build context, so .dockerignore is irrelevant to it. + const isStageCopy = /(?:^|\s)--from=/u.test(inst.args); + if (isStageCopy) { + return false; + } + const parts = inst.args.split(/\s+/u); - const [src] = parts; + const src = parts.find((p) => !p.startsWith("--")); + if (!src) { + return false; + } return src === "." || src === "./" || src === "*"; } return false; diff --git a/packages/core/src/rules/security.ts b/packages/core/src/rules/security.ts index 53a01a7..ae84472 100644 --- a/packages/core/src/rules/security.ts +++ b/packages/core/src/rules/security.ts @@ -16,7 +16,10 @@ export const noRootUser: DockerfileRule = { let lastUserLine = 1; for (const inst of instructions) { - if (inst.instruction === "USER") { + if (inst.instruction === "FROM") { + lastUser = "root"; + lastUserLine = inst.line; + } else if (inst.instruction === "USER") { lastUser = inst.args.trim().toLowerCase(); lastUserLine = inst.line; } @@ -48,12 +51,12 @@ export const noSecretsInEnv: DockerfileRule = { check(instructions, file) { const diagnostics: Diagnostic[] = []; const secretKeywords = [ - /password/iu, - /secret/iu, - /token/iu, - /api_key/iu, - /private_key/iu, - /auth/iu, + /(?:^|[_-])password(?:[_-]|$)/iu, + /(?:^|[_-])secret(?:[_-]|$)/iu, + /(?:^|[_-])token(?:[_-]|$)/iu, + /(?:^|[_-])api_key(?:[_-]|$)/iu, + /(?:^|[_-])private_key(?:[_-]|$)/iu, + /(?:^|[_-])auth(?:[_-]|$)/iu, ]; for (const inst of instructions) { @@ -193,7 +196,11 @@ export const noAddRemote: DockerfileRule = { for (const inst of instructions) { if (inst.instruction === "ADD") { const parts = inst.args.split(/\s+/u); - const [src] = parts; + const src = parts.find((p) => !p.startsWith("--")); + + if (!src) { + continue; + } if (src.startsWith("http://") || src.startsWith("https://")) { diagnostics.push( diff --git a/packages/core/test/rules.test.ts b/packages/core/test/rules.test.ts index aaa1c9a..2a5ac8a 100644 --- a/packages/core/test/rules.test.ts +++ b/packages/core/test/rules.test.ts @@ -55,6 +55,19 @@ describe("Security Rules", () => { expect(diags2).toHaveLength(0); }); + test("no-root-user: multi-stage runtime without USER", () => { + const multiStageRootRuntime = parseDockerfile(` + FROM node:22-alpine AS build + USER node + RUN npm run build + FROM node:22-alpine + COPY --from=build /app/dist ./dist + CMD ["node", "dist/index.js"] + `); + const diags3 = noRootUser.check(multiStageRootRuntime, "Dockerfile"); + expect(diags3).toHaveLength(1); + }); + test("pin-image-version", () => { const unpinned = parseDockerfile(` FROM node @@ -98,6 +111,22 @@ describe("Security Rules", () => { expect(diagsSpaceNormal).toHaveLength(0); }); + test("no-secrets-in-env: AUTHOR is not a secret", () => { + const authorNotSecret = parseDockerfile(` + ENV AUTHOR=grumpy + `); + expect(noSecretsInEnv.check(authorNotSecret, "Dockerfile")).toHaveLength(0); + }); + + test("no-secrets-in-env: OAUTH_ISSUER_URL is not a secret", () => { + const oauthUrlNotSecret = parseDockerfile(` + ENV OAUTH_ISSUER_URL=https://example.com + `); + expect(noSecretsInEnv.check(oauthUrlNotSecret, "Dockerfile")).toHaveLength( + 0 + ); + }); + test("no-add-remote", () => { const remoteAdd = parseDockerfile(` ADD https://example.com/file.txt /app/ @@ -105,6 +134,13 @@ describe("Security Rules", () => { const diags1 = noAddRemote.check(remoteAdd, "Dockerfile"); expect(diags1).toHaveLength(1); }); + + test("no-add-remote: remote URL with --chown flag", () => { + const remoteAddWithChown = parseDockerfile(` + ADD --chown=node:node https://example.com/file.txt /app/ + `); + expect(noAddRemote.check(remoteAddWithChown, "Dockerfile")).toHaveLength(1); + }); }); describe("Performance Rules", () => { @@ -119,6 +155,27 @@ describe("Performance Rules", () => { expect(diags[0].rule).toBe("docker-doctor/order-layers"); }); + test("order-layers: correct multi-stage build", () => { + const correctMultiStage = parseDockerfile(` + FROM node:22-alpine AS build + COPY . . + RUN npm run build + FROM node:22-alpine + COPY package.json ./ + RUN npm ci --omit=dev + `); + expect(orderLayers.check(correctMultiStage, "Dockerfile")).toHaveLength(0); + }); + + test("order-layers: /usr/src path is not a copy-all", () => { + const usrSrcPath = parseDockerfile(` + FROM node:22-alpine + COPY /usr/src/lib /lib + RUN npm ci + `); + expect(orderLayers.check(usrSrcPath, "Dockerfile")).toHaveLength(0); + }); + test("use-multi-stage", () => { const singleStage = parseDockerfile(` FROM node:22 @@ -153,6 +210,17 @@ describe("Performance Rules", () => { }); expect(diags2).toHaveLength(0); }); + + test("use-dockerignore ignores stage-to-stage copies", () => { + const stageCopy = parseDockerfile(` + FROM node:22-alpine AS build + FROM node:22-alpine + COPY --from=build . ./ + `); + expect( + useDockerignore.check(stageCopy, "Dockerfile", { projectFiles: [] }) + ).toHaveLength(0); + }); }); describe("Compose Rules", () => { diff --git a/packages/docker-doctor/package.json b/packages/docker-doctor/package.json index 7a1c718..c9dbb0e 100644 --- a/packages/docker-doctor/package.json +++ b/packages/docker-doctor/package.json @@ -54,6 +54,7 @@ "scripts": { "build": "NODE_OPTIONS='--max-old-space-size=4096' tsdown", "dev": "NODE_OPTIONS='--max-old-space-size=4096' tsdown --watch", + "test": "bun test", "typecheck": "tsc --noEmit", "clean": "git clean -xdf .turbo node_modules dist" }, diff --git a/packages/docker-doctor/test/cli.test.ts b/packages/docker-doctor/test/cli.test.ts new file mode 100644 index 0000000..f6f056b --- /dev/null +++ b/packages/docker-doctor/test/cli.test.ts @@ -0,0 +1,119 @@ +import { beforeAll, describe, expect, test } from "bun:test"; +import path from "node:path"; + +const CLI = path.join(import.meta.dir, "..", "dist", "cli.mjs"); +const fixture = (name: string) => path.join(import.meta.dir, "fixtures", name); + +const runCli = async (args: string[]) => { + const proc = Bun.spawn(["node", CLI, ...args], { + stderr: "pipe", + stdout: "pipe", + }); + const stdout = await new Response(proc.stdout).text(); + const stderr = await new Response(proc.stderr).text(); + const exitCode = await proc.exited; + return { exitCode, stderr, stdout }; +}; + +beforeAll(async () => { + const exists = await Bun.file(CLI).exists(); + if (!exists) { + throw new Error( + `Built CLI not found at ${CLI}. Run: bun run build --filter @docker-doctor/cli` + ); + } +}); + +describe("smoke", () => { + test("--version exits 0 and prints a semver-like string", async () => { + const { exitCode, stdout } = await runCli(["--version"]); + expect(exitCode).toBe(0); + expect(stdout.trim()).toMatch(/^\d+\.\d+\.\d+/u); + }); +}); + +describe("exit codes", () => { + test("clean fixture with --json exits 0", async () => { + const { exitCode } = await runCli([fixture("clean"), "--json"]); + expect(exitCode).toBe(0); + }); + + test("with-error fixture with --json exits 1", async () => { + const { exitCode } = await runCli([fixture("with-error"), "--json"]); + expect(exitCode).toBe(1); + }); + + test("clean fixture with --score exits 0", async () => { + const { exitCode } = await runCli([fixture("clean"), "--score"]); + expect(exitCode).toBe(0); + }); +}); + +describe("--json contract", () => { + test("with-error fixture produces parseable JSON", async () => { + const { stdout } = await runCli([fixture("with-error"), "--json"]); + expect(() => JSON.parse(stdout)).not.toThrow(); + }); + + test("report has exactly the documented top-level keys", async () => { + const { stdout } = await runCli([fixture("with-error"), "--json"]); + const report = JSON.parse(stdout); + expect(Object.keys(report).toSorted()).toEqual( + ["diagnostics", "label", "project", "score", "timestamp"].toSorted() + ); + }); + + test("score is a number in [0, 100]", async () => { + const { stdout } = await runCli([fixture("with-error"), "--json"]); + const report = JSON.parse(stdout); + expect(typeof report.score).toBe("number"); + expect(report.score).toBeGreaterThanOrEqual(0); + expect(report.score).toBeLessThanOrEqual(100); + }); + + test("every diagnostic has the required shape", async () => { + const { stdout } = await runCli([fixture("with-error"), "--json"]); + const report = JSON.parse(stdout); + expect(Array.isArray(report.diagnostics)).toBe(true); + for (const d of report.diagnostics) { + expect(typeof d.file).toBe("string"); + expect(typeof d.help).toBe("string"); + expect(typeof d.message).toBe("string"); + expect(typeof d.rule).toBe("string"); + expect(["error", "warning", "info"]).toContain(d.severity); + } + }); + + test("at least one diagnostic has severity error", async () => { + const { stdout } = await runCli([fixture("with-error"), "--json"]); + const report = JSON.parse(stdout); + expect( + report.diagnostics.some( + (d: { severity: string }) => d.severity === "error" + ) + ).toBe(true); + }); +}); + +describe("--score contract", () => { + test("clean fixture prints only an integer score", async () => { + const { stdout } = await runCli([fixture("clean"), "--score"]); + const trimmed = stdout.trim(); + expect(Number.isInteger(Number(trimmed))).toBe(true); + expect(trimmed.split("\n").length).toBe(1); + }); + + test("clean fixture score is >= 50", async () => { + const { stdout } = await runCli([fixture("clean"), "--score"]); + expect(Number(stdout.trim())).toBeGreaterThanOrEqual(50); + }); +}); + +describe("empty project", () => { + test("scanning an empty directory with --json exits 0 with no diagnostics", async () => { + const { exitCode, stdout } = await runCli([fixture("empty"), "--json"]); + expect(exitCode).toBe(0); + const report = JSON.parse(stdout); + expect(report.diagnostics).toEqual([]); + }); +}); diff --git a/packages/docker-doctor/test/fixtures/clean/.dockerignore b/packages/docker-doctor/test/fixtures/clean/.dockerignore new file mode 100644 index 0000000..3c3629e --- /dev/null +++ b/packages/docker-doctor/test/fixtures/clean/.dockerignore @@ -0,0 +1 @@ +node_modules diff --git a/packages/docker-doctor/test/fixtures/clean/Dockerfile b/packages/docker-doctor/test/fixtures/clean/Dockerfile new file mode 100644 index 0000000..84e0ae1 --- /dev/null +++ b/packages/docker-doctor/test/fixtures/clean/Dockerfile @@ -0,0 +1,8 @@ +FROM node:22.2.0-alpine +WORKDIR /app +COPY package.json ./ +RUN npm ci --omit=dev +COPY src ./src +USER node +HEALTHCHECK --interval=30s --timeout=3s CMD node -e "process.exit(0)" +CMD ["node", "src/index.js"] diff --git a/packages/docker-doctor/test/fixtures/empty/.gitkeep b/packages/docker-doctor/test/fixtures/empty/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/packages/docker-doctor/test/fixtures/with-error/Dockerfile b/packages/docker-doctor/test/fixtures/with-error/Dockerfile new file mode 100644 index 0000000..8a448cc --- /dev/null +++ b/packages/docker-doctor/test/fixtures/with-error/Dockerfile @@ -0,0 +1,4 @@ +FROM node:22.2.0-alpine +ENV DB_PASSWORD=hunter2 +USER node +CMD ["node", "index.js"]