test(core): harden the rule suite with realistic negative cases

.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.

.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.

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<string> => {
+  const aliases = new Set<string>();
+
+  for (const inst of instructions) {
+    if (inst.instruction !== "FROM") {
+      continue;
+    }
+
+    const match = /\sas\s+(?<alias>\S+)/iu.exec(inst.args);
+    if (match?.groups?.alias) {
+      aliases.add(match.groups.alias.toLowerCase());
+    }
+  }
+
+  return aliases;
+};

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/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
+            )
+          );
         }
       }
     }

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

@@ -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(