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/.changeset/shy-lemons-parse.md b/.changeset/shy-lemons-parse.md new file mode 100644 index 0000000..40f420c --- /dev/null +++ b/.changeset/shy-lemons-parse.md @@ -0,0 +1,12 @@ +--- +"@docker-doctor/cli": patch +--- + +Fix image-reference parsing in `pin-image-version` and `prefer-slim-base`, which both used to split on the first `:` and got several common forms wrong: + +- Images with a registry port (`myregistry.example.com:5000/team/app`) are no longer mistaken for pinned/tagged just because the port number looks like a tag. `pin-image-version` now correctly flags these as unpinned. +- Digest-pinned images (`node@sha256:...`) are no longer misread as an untagged full-OS image by `prefer-slim-base` — the strongest possible pin is now recognized and silently accepted by both rules. +- Multi-stage build aliases (`FROM builder`, referencing an earlier `FROM ... AS builder`) are no longer flagged as unpinned or non-slim. +- `${ARG}`-driven base images are skipped instead of producing a false diagnostic, since the actual image can't be determined statically. + +Image-reference parsing is now handled by a single shared parser (`parseImageRef`/`collectStageAliases` in `@docker-doctor/core`) instead of duplicated ad-hoc string splitting in each rule. Expect diagnostics to change on Dockerfiles that use registry ports, digest pins, or multi-stage aliases as their `FROM` target. diff --git a/packages/core/src/parsers/image-ref.ts b/packages/core/src/parsers/image-ref.ts new file mode 100644 index 0000000..b67d6dc --- /dev/null +++ b/packages/core/src/parsers/image-ref.ts @@ -0,0 +1,74 @@ +import type { DockerfileInstruction } from "../types/index"; + +export interface ImageRef { + registry?: string; + name: string; + tag?: string; + digest?: string; + isVariable: boolean; +} + +export const parseImageRef = (ref: string): ImageRef => { + if (ref.includes("${") || ref.startsWith("$")) { + return { isVariable: true, name: ref }; + } + + let remainder = ref; + let digest: string | undefined; + + const atIndex = remainder.indexOf("@"); + if (atIndex !== -1) { + digest = remainder.slice(atIndex + 1); + remainder = remainder.slice(0, atIndex); + } + + let tag: string | undefined; + const lastColonIndex = remainder.lastIndexOf(":"); + const lastSlashIndex = remainder.lastIndexOf("/"); + + if (lastColonIndex !== -1 && lastColonIndex > lastSlashIndex) { + tag = remainder.slice(lastColonIndex + 1); + remainder = remainder.slice(0, lastColonIndex); + } + + let registry: string | undefined; + const firstSlashIndex = remainder.indexOf("/"); + if (firstSlashIndex !== -1) { + const firstSegment = remainder.slice(0, firstSlashIndex); + if ( + firstSegment.includes(".") || + firstSegment.includes(":") || + firstSegment === "localhost" + ) { + registry = firstSegment; + remainder = remainder.slice(firstSlashIndex + 1); + } + } + + return { + digest, + isVariable: false, + name: remainder, + registry, + tag, + }; +}; + +export const collectStageAliases = ( + instructions: DockerfileInstruction[] +): Set => { + const aliases = new Set(); + + for (const inst of instructions) { + if (inst.instruction !== "FROM") { + continue; + } + + const match = /\sas\s+(?\S+)/iu.exec(inst.args); + if (match?.groups?.alias) { + aliases.add(match.groups.alias.toLowerCase()); + } + } + + return aliases; +}; 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/image-size.ts b/packages/core/src/rules/image-size.ts index f229046..e719400 100644 --- a/packages/core/src/rules/image-size.ts +++ b/packages/core/src/rules/image-size.ts @@ -1,3 +1,4 @@ +import { collectStageAliases, parseImageRef } from "../parsers/image-ref"; import type { Diagnostic, DockerfileRule } from "../types/index"; const createDiagnostic = ( @@ -13,6 +14,7 @@ export const preferSlimBase: DockerfileRule = { category: "Image Size", check(instructions, file) { const diagnostics: Diagnostic[] = []; + const stageAliases = collectStageAliases(instructions); for (const inst of instructions) { if (inst.instruction === "FROM") { @@ -22,39 +24,39 @@ export const preferSlimBase: DockerfileRule = { continue; } - const colonIndex = imagePart.indexOf(":"); - if (colonIndex !== -1) { - const tag = imagePart.slice(colonIndex + 1).toLowerCase(); - - // If the tag doesn't contain alpine, slim, distroless, or sha256 - const isSlim = - tag.includes("alpine") || - tag.includes("slim") || - tag.includes("distroless"); - const isSha = tag.startsWith("sha256:"); - - // Skip if it's a multi-stage builder step reference (e.g. FROM build AS publish) - const isStageReference = instructions.some( - (other) => - other.line < inst.line && - other.instruction === "FROM" && - other.args - .toLowerCase() - .includes(` as ${imagePart.toLowerCase()}`) - ); + const ref = parseImageRef(imagePart); + + if (ref.isVariable || stageAliases.has(imagePart.toLowerCase())) { + continue; + } + + // Digest pins are already fully deterministic; not our concern here. + if (ref.digest) { + continue; + } + + // No tag: pin-image-version owns the untagged case, don't double-report. + if (!ref.tag) { + continue; + } - if (!isSlim && !isSha && !isStageReference) { - diagnostics.push( - createDiagnostic( - file, - this.key, - this.defaultSeverity as "error" | "warning" | "info", - `Base image '${imagePart}' may be a full-OS distribution. Consider using a slim or alpine alternative.`, - this.help, - inst.line - ) - ); - } + const tag = ref.tag.toLowerCase(); + const isSlim = + tag.includes("alpine") || + tag.includes("slim") || + tag.includes("distroless"); + + if (!isSlim) { + diagnostics.push( + createDiagnostic( + file, + this.key, + this.defaultSeverity as "error" | "warning" | "info", + `Base image '${imagePart}' may be a full-OS distribution. Consider using a slim or alpine alternative.`, + this.help, + inst.line + ) + ); } } } 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..ea7e3a1 100644 --- a/packages/core/src/rules/security.ts +++ b/packages/core/src/rules/security.ts @@ -1,3 +1,4 @@ +import { collectStageAliases, parseImageRef } from "../parsers/image-ref"; import type { Diagnostic, DockerfileRule } from "../types/index"; const createDiagnostic = ( @@ -16,7 +17,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 +52,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) { @@ -133,6 +137,7 @@ export const pinImageVersion: DockerfileRule = { category: "Security", check(instructions, file) { const diagnostics: Diagnostic[] = []; + const stageAliases = collectStageAliases(instructions); for (const inst of instructions) { if (inst.instruction === "FROM") { @@ -145,10 +150,13 @@ export const pinImageVersion: DockerfileRule = { continue; } - const colonIndex = imagePart.indexOf(":"); - const atIndex = imagePart.indexOf("@"); + const ref = parseImageRef(imagePart); + + if (ref.isVariable || stageAliases.has(imagePart.toLowerCase())) { + continue; + } - if (colonIndex === -1 && atIndex === -1) { + if (!(ref.tag || ref.digest)) { diagnostics.push( createDiagnostic( file, @@ -159,20 +167,17 @@ export const pinImageVersion: DockerfileRule = { inst.line ) ); - } else if (colonIndex !== -1) { - const tag = imagePart.slice(colonIndex + 1); - if (tag === "latest") { - diagnostics.push( - createDiagnostic( - file, - this.key, - this.defaultSeverity as "error" | "warning" | "info", - `Base image '${imagePart}' uses the mutable 'latest' tag. This makes builds non-deterministic.`, - this.help, - inst.line - ) - ); - } + } else if (ref.tag === "latest" && !ref.digest) { + diagnostics.push( + createDiagnostic( + file, + this.key, + this.defaultSeverity as "error" | "warning" | "info", + `Base image '${imagePart}' uses the mutable 'latest' tag. This makes builds non-deterministic.`, + this.help, + inst.line + ) + ); } } } @@ -193,7 +198,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/image-ref.test.ts b/packages/core/test/image-ref.test.ts new file mode 100644 index 0000000..70bbfc9 --- /dev/null +++ b/packages/core/test/image-ref.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, test } from "bun:test"; + +import { parseDockerfile } from "../src/parsers/dockerfile-parser"; +import { collectStageAliases, parseImageRef } from "../src/parsers/image-ref"; + +describe("parseImageRef", () => { + test("bare image name", () => { + const ref = parseImageRef("node"); + expect(ref.name).toBe("node"); + expect(ref.tag).toBeUndefined(); + expect(ref.digest).toBeUndefined(); + }); + + test("image with a version tag", () => { + const ref = parseImageRef("node:22.2.0-alpine"); + expect(ref.tag).toBe("22.2.0-alpine"); + }); + + test("image with the latest tag", () => { + const ref = parseImageRef("node:latest"); + expect(ref.tag).toBe("latest"); + }); + + test("registry with a port and no tag", () => { + const ref = parseImageRef("myregistry.example.com:5000/team/app"); + expect(ref.registry).toBe("myregistry.example.com:5000"); + expect(ref.name).toContain("team/app"); + expect(ref.tag).toBeUndefined(); + }); + + test("registry with a port and a tag", () => { + const ref = parseImageRef("myregistry.example.com:5000/team/app:1.2.3"); + expect(ref.registry).toBe("myregistry.example.com:5000"); + expect(ref.tag).toBe("1.2.3"); + }); + + test("digest without a tag", () => { + const ref = parseImageRef("node@sha256:aaaa"); + expect(ref.digest).toBe("sha256:aaaa"); + expect(ref.tag).toBeUndefined(); + }); + + test("tag and digest together", () => { + const ref = parseImageRef("node:22@sha256:aaaa"); + expect(ref.tag).toBe("22"); + expect(ref.digest).toBe("sha256:aaaa"); + }); + + test("variable-driven image", () => { + const ref = parseImageRef(`\${NODE_IMAGE}`); + expect(ref.isVariable).toBe(true); + }); + + test("localhost registry", () => { + const ref = parseImageRef("localhost:5000/app"); + expect(ref.registry).toBe("localhost:5000"); + expect(ref.tag).toBeUndefined(); + }); +}); + +describe("collectStageAliases", () => { + test("collects an alias from a two-stage build", () => { + const instructions = parseDockerfile(` + FROM node:22-alpine AS build + FROM build + `); + expect(collectStageAliases(instructions)).toEqual(new Set(["build"])); + }); + + test("returns an empty set when no AS clause is present", () => { + const instructions = parseDockerfile(` + FROM node:22-alpine + `); + expect(collectStageAliases(instructions)).toEqual(new Set()); + }); + + test("collects a lowercase as clause", () => { + const instructions = parseDockerfile(` + FROM node:22-alpine as build + FROM build + `); + expect(collectStageAliases(instructions)).toEqual(new Set(["build"])); + }); +}); diff --git a/packages/core/test/rules.test.ts b/packages/core/test/rules.test.ts index aaa1c9a..1924d6f 100644 --- a/packages/core/test/rules.test.ts +++ b/packages/core/test/rules.test.ts @@ -53,6 +53,31 @@ describe("Security Rules", () => { `); const diags2 = noRootUser.check(withUserNode, "Dockerfile"); expect(diags2).toHaveLength(0); + + const multiStageWithFinalUser = parseDockerfile(` + FROM node:22-alpine AS build + RUN npm run build + FROM node:22-alpine + COPY --from=build /app/dist ./dist + USER node + CMD ["node", "dist/index.js"] + `); + expect( + noRootUser.check(multiStageWithFinalUser, "Dockerfile") + ).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", () => { @@ -73,6 +98,30 @@ describe("Security Rules", () => { `); const diags3 = pinImageVersion.check(pinned, "Dockerfile"); expect(diags3).toHaveLength(0); + + const registryPortUntagged = parseDockerfile(` + FROM myregistry.example.com:5000/team/app + `); + expect( + pinImageVersion.check(registryPortUntagged, "Dockerfile") + ).toHaveLength(1); + + const stageAlias = parseDockerfile(` + FROM node:22-alpine AS build + FROM build + `); + expect(pinImageVersion.check(stageAlias, "Dockerfile")).toHaveLength(0); + + const argDriven = parseDockerfile(` + ARG NODE_IMAGE=node:22-alpine + FROM \${NODE_IMAGE} + `); + expect(pinImageVersion.check(argDriven, "Dockerfile")).toHaveLength(0); + + const digestPinned = parseDockerfile(` + FROM node@sha256:aaaabbbbccccdddd + `); + expect(pinImageVersion.check(digestPinned, "Dockerfile")).toHaveLength(0); }); test("no-secrets-in-env", () => { @@ -96,6 +145,45 @@ describe("Security Rules", () => { "Dockerfile" ); expect(diagsSpaceNormal).toHaveLength(0); + + const argPassthrough = parseDockerfile(` + ARG API_KEY + ENV API_KEY=\${API_KEY} + `); + expect(noSecretsInEnv.check(argPassthrough, "Dockerfile")).toHaveLength(0); + + const multipleNormalVars = parseDockerfile(` + ENV NODE_ENV=production PORT=3000 + `); + expect(noSecretsInEnv.check(multipleNormalVars, "Dockerfile")).toHaveLength( + 0 + ); + + const awsSecretKey = parseDockerfile(` + ENV AWS_SECRET_ACCESS_KEY=AKIAIOSFODNN7EXAMPLE + `); + expect(noSecretsInEnv.check(awsSecretKey, "Dockerfile")).toHaveLength(1); + + const apiKey = parseDockerfile(` + ENV API_KEY=not-a-real-secret + `); + expect(noSecretsInEnv.check(apiKey, "Dockerfile")).toHaveLength(1); + }); + + 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", () => { @@ -104,6 +192,20 @@ describe("Security Rules", () => { `); const diags1 = noAddRemote.check(remoteAdd, "Dockerfile"); expect(diags1).toHaveLength(1); + + const localArchiveWithChown = parseDockerfile(` + ADD --chown=node:node local-archive.tar.gz /app/ + `); + expect(noAddRemote.check(localArchiveWithChown, "Dockerfile")).toHaveLength( + 0 + ); + }); + + 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); }); }); @@ -117,6 +219,35 @@ describe("Performance Rules", () => { const diags = orderLayers.check(badOrder, "Dockerfile"); expect(diags).toHaveLength(1); expect(diags[0].rule).toBe("docker-doctor/order-layers"); + + const goodOrder = parseDockerfile(` + FROM node:22-alpine + COPY package.json package-lock.json ./ + RUN npm install + COPY . . + `); + expect(orderLayers.check(goodOrder, "Dockerfile")).toHaveLength(0); + }); + + 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", () => { @@ -126,6 +257,16 @@ describe("Performance Rules", () => { `); const diags1 = useMultiStage.check(singleStage, "Dockerfile"); expect(diags1).toHaveLength(1); + + const realisticMultiStage = parseDockerfile(` + FROM node:22-alpine AS build + RUN npm run build + FROM node:22-alpine + COPY --from=build /app/dist ./dist + `); + expect(useMultiStage.check(realisticMultiStage, "Dockerfile")).toHaveLength( + 0 + ); }); test("minimize-layers", () => { @@ -136,6 +277,12 @@ describe("Performance Rules", () => { `); const diags1 = minimizeLayers.check(consecutive, "Dockerfile"); expect(diags1).toHaveLength(1); + + const combinedRun = parseDockerfile(` + FROM node:22-alpine + RUN apt-get update && apt-get install -y git && rm -rf /var/lib/apt/lists/* + `); + expect(minimizeLayers.check(combinedRun, "Dockerfile")).toHaveLength(0); }); test("use-dockerignore", () => { @@ -152,6 +299,26 @@ describe("Performance Rules", () => { projectFiles: ["Dockerfile", ".dockerignore"], }); expect(diags2).toHaveLength(0); + + const copyAllWithChown = parseDockerfile(` + FROM node:22-alpine + COPY --chown=node:node . . + `); + const diags3 = useDockerignore.check(copyAllWithChown, "Dockerfile", { + projectFiles: ["Dockerfile", ".dockerignore"], + }); + expect(diags3).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); }); }); @@ -165,6 +332,16 @@ describe("Compose Rules", () => { }; const diags = noVersionKey.check(composeContent, "compose.yml"); expect(diags).toHaveLength(1); + + const realisticCompliant = { + services: { + db: { image: "postgres:16-alpine" }, + web: { image: "node:22-alpine" }, + }, + }; + expect(noVersionKey.check(realisticCompliant, "compose.yml")).toHaveLength( + 0 + ); }); test("require-resource-limits", () => { @@ -175,6 +352,20 @@ describe("Compose Rules", () => { }; const diags = requireResourceLimits.check(composeContent, "compose.yml"); expect(diags).toHaveLength(1); + + const withLimits = { + services: { + web: { + deploy: { + resources: { limits: { cpus: "0.5", memory: "512M" } }, + }, + image: "node:22-alpine", + }, + }, + }; + expect(requireResourceLimits.check(withLimits, "compose.yml")).toHaveLength( + 0 + ); }); test("require-restart-policy", () => { @@ -193,6 +384,18 @@ describe("Compose Rules", () => { }; const diags2 = requireRestartPolicy.check(withRestart, "compose.yml"); expect(diags2).toHaveLength(0); + + const withDeployRestartPolicy = { + services: { + web: { + deploy: { restart_policy: { condition: "on-failure" } }, + image: "node:22-alpine", + }, + }, + }; + expect( + requireRestartPolicy.check(withDeployRestartPolicy, "compose.yml") + ).toHaveLength(0); }); test("use-depends-on-condition", () => { @@ -203,6 +406,18 @@ describe("Compose Rules", () => { }; const diags1 = useDependsOnCondition.check(shortForm, "compose.yml"); expect(diags1).toHaveLength(1); + + const longForm = { + services: { + web: { + depends_on: { db: { condition: "service_healthy" } }, + image: "node:22-alpine", + }, + }, + }; + expect(useDependsOnCondition.check(longForm, "compose.yml")).toHaveLength( + 0 + ); }); }); @@ -219,6 +434,22 @@ describe("Image Size Rules", () => { `); const diags2 = preferSlimBase.check(slimBase, "Dockerfile"); expect(diags2).toHaveLength(0); + + const digestPinned = parseDockerfile(` + FROM node@sha256:aaaabbbbccccdddd + `); + expect(preferSlimBase.check(digestPinned, "Dockerfile")).toHaveLength(0); + + const registryPort = parseDockerfile(` + FROM myregistry.example.com:5000/team/app + `); + expect(preferSlimBase.check(registryPort, "Dockerfile")).toHaveLength(0); + + const stageAlias = parseDockerfile(` + FROM node:22-alpine AS build + FROM build + `); + expect(preferSlimBase.check(stageAlias, "Dockerfile")).toHaveLength(0); }); test("clean-package-cache", () => { @@ -233,6 +464,11 @@ describe("Image Size Rules", () => { `); const diags2 = cleanPackageCache.check(withCleanup, "Dockerfile"); expect(diags2).toHaveLength(0); + + const apkNoCache = parseDockerfile(` + RUN apk add --no-cache curl + `); + expect(cleanPackageCache.check(apkNoCache, "Dockerfile")).toHaveLength(0); }); test("avoid-dev-dependencies", () => { @@ -303,6 +539,17 @@ describe("Best Practices Rules", () => { expect(diags3).toHaveLength(0); }); + // Known false positive: the pipefail check tests inst.raw with no quote + // awareness, so a regex alternation inside a quoted argument reads as a + // shell pipeline. Not fixed here - see plans/README.md deferred list. + test.todo("use-pipefail ignores pipes inside quoted arguments", () => { + const quotedPipe = parseDockerfile(` + FROM node:22-alpine + RUN grep -E "foo|bar" /etc/passwd + `); + expect(usePipefail.check(quotedPipe, "Dockerfile")).toHaveLength(0); + }); + test("absolute-workdir", () => { const relative = parseDockerfile(` WORKDIR app/src @@ -316,6 +563,13 @@ describe("Best Practices Rules", () => { `); const diags2 = absoluteWorkdir.check(absolute, "Dockerfile"); expect(diags2).toHaveLength(0); + + const variableWorkdir = parseDockerfile(` + WORKDIR \${APP_HOME} + `); + expect(absoluteWorkdir.check(variableWorkdir, "Dockerfile")).toHaveLength( + 0 + ); }); test("avoid-run-cd", () => { @@ -334,6 +588,16 @@ describe("Best Practices Rules", () => { expect(diags2).toHaveLength(0); }); + // Known false positive: /\bcd\b/ matches path segments and words in + // strings, not just the cd command. Not fixed here. + test.todo("avoid-run-cd ignores cd inside paths and strings", () => { + const pathWithCd = parseDockerfile(` + FROM node:22-alpine + RUN mkdir -p /opt/cd && echo abcd + `); + expect(avoidRunCd.check(pathWithCd, "Dockerfile")).toHaveLength(0); + }); + test("sort-multiline-args", () => { const unsorted = parseDockerfile(` RUN apt-get update && apt-get install -y --no-install-recommends \\ @@ -384,6 +648,19 @@ describe("Best Practices Rules", () => { `); const diags2 = requireHealthcheck.check(withHealth, "Dockerfile"); expect(diags2).toHaveLength(0); + + const multiStageWithHealth = parseDockerfile(` + FROM node:22-alpine AS build + RUN npm run build + FROM node:22-alpine + COPY --from=build /app/dist ./dist + EXPOSE 3000 + HEALTHCHECK --interval=30s --timeout=3s CMD curl -f http://localhost:3000/health || exit 1 + CMD ["node", "dist/index.js"] + `); + expect( + requireHealthcheck.check(multiStageWithHealth, "Dockerfile") + ).toHaveLength(0); }); test("prefer-copy-over-add", () => { @@ -398,6 +675,13 @@ describe("Best Practices Rules", () => { `); const diags2 = preferCopyOverAdd.check(addArchive, "Dockerfile"); expect(diags2).toHaveLength(0); + + const addArchiveWithChown = parseDockerfile(` + ADD --chown=node:node archive.tar.gz /app/ + `); + expect( + preferCopyOverAdd.check(addArchiveWithChown, "Dockerfile") + ).toHaveLength(0); }); test("use-exec-form", () => { @@ -412,6 +696,11 @@ describe("Best Practices Rules", () => { `); const diags2 = useExecForm.check(execForm, "Dockerfile"); expect(diags2).toHaveLength(0); + + const entrypointExecForm = parseDockerfile(` + ENTRYPOINT ["docker-entrypoint.sh"] + `); + expect(useExecForm.check(entrypointExecForm, "Dockerfile")).toHaveLength(0); }); test("require-labels", () => { @@ -426,5 +715,10 @@ describe("Best Practices Rules", () => { `); const diags2 = requireLabels.check(withLabels, "Dockerfile"); expect(diags2).toHaveLength(0); + + const withOciLabels = parseDockerfile(` + LABEL org.opencontainers.image.authors="team@example.com" org.opencontainers.image.version="1.0.0" + `); + expect(requireLabels.check(withOciLabels, "Dockerfile")).toHaveLength(0); }); }); 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"]