fix(core): correct four rule-specificity bugs in the Dockerfile engine

.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=<stage>` 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.

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 =

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=<stage> 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;

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(

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,13 +111,36 @@ 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/
       `);
     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", () => {

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"
   },

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([]);
+  });
+});

packages/docker-doctor/test/fixtures/clean/.dockerignore

@@ -0,0 +1 @@
+node_modules

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"]

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"]