diff --git a/Dockerfile b/Dockerfile index a3bbe88a2..44dc0f8bf 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,6 @@ -FROM node:lts-slim +# Pinned to the major declared in .nvmrc (not `lts-slim`, which floats +# independently of it). Kept in sync by scripts/tests/dockerfileToolchain.unit.tests.js. +FROM node:24-slim # switch user USER node @@ -9,7 +11,7 @@ WORKDIR /home/node # Install app dependencies & setup ARG NODE_ENV=production ENV NODE_ENV=${NODE_ENV} -COPY --chown=node:node package*.json ./ +COPY --chown=node:node package*.json .npmrc ./ RUN if [ "$NODE_ENV" = "test" ]; then npm ci --include=dev; else npm ci --omit=dev; fi COPY --chown=node:node . . diff --git a/ERRORS.md b/ERRORS.md index 6ddd39400..9c36473b5 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -37,3 +37,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-07-17] billing/stripe: #3964 fixed the admin (409 abort) and reconcile (skip-comparison) call sites of the shared plan resolver but left `billing.webhook.service.js resolvePlan`'s `?? 'free'` fallback in place -> a webhook for an existing paid org whose Stripe price is unresolvable (unmapped `priceId`, no valid `metadata.planId` — e.g. a manually-sold enterprise price) silently downgraded the org to `'free'` on the next `customer.subscription.updated`/`.created`/`invoice.payment_succeeded` event; a webhook cannot 409-abort mid-flight like the admin path, so the fix instead retains the org's/subscription's already-loaded `.plan` (never a hardcoded `'free'`) and emits `billing.webhook.plan_unresolved` for manual review — only a brand-new subscription with zero prior plan reference (no DB row anywhere for that org) still defaults to `'free'`; see pierreb-devkit/Node#3970 - [2026-07-25] billing: `forceRotateForPlanChange` (week-quota snapshot rotation) was wired only into `handleSubscriptionUpdated`'s plan-change block -> `handleCheckoutCompleted` and both branches of `handleSubscriptionCreated` never rotated the current-week quota snapshot after a mid-week plan activation, so an upgraded org kept the previous plan's weekly quota (often 0 on free→paid) until the next weekly reset; fixed by calling the same non-fatal, logged `forceRotateForPlanChange(organizationId, { preserveUsage: true })` from all three activation call sites, and by making `billing.usage.service.js incrementMeter` read the live plan quota (already fetched for the snapshot write) instead of the potentially-stale `updatedDoc.meterQuota` for overflow decisions — mirrors the existing live-quota display fix in `billing.controller.js`; see pierreb-devkit/Node#3988 - [2026-09-04] deps/engines: `engines.node` declared `">=22.0.0"` while the committed lock was already npm-11-shaped, so `npm ci` under npm 10 (bundled by every Node 22.x) failed mid-install with a confusing `Missing: conventional-commits-filter@6.0.1 from lock file` error instead of a clean engines rejection -> a public package's `engines.node`/`engines.npm` must bound the exact npm major that wrote the lock, not just "a Node version that happens to work today"; pin the toolchain with `.nvmrc` + CI's `setup-node` reading it (`node-version-file`, not a floating `lts/*`) and add `engine-strict=true` to `.npmrc` so an unsupported toolchain fails fast at the engines check instead of at dependency resolution; see pierreb-devkit/Node#4053 +- [2026-09-04] docker: #4053 pinned the toolchain for CI and local installs, but the Dockerfile still copied `package*.json`, ran `npm ci`, and only then `COPY . .` — so `.npmrc` (`engine-strict=true`) landed AFTER the install, and `FROM node:lts-slim` floated independently of `.nvmrc`, so an unsupported base image inside Docker still reproduced the confusing lockfile error instead of a clean `EBADENGINE`, and the image could silently drift onto a newer Node major once it enters LTS -> a toolchain fail-fast fix must be verified in every install path, not just the one exercised by CI; copy `.npmrc` alongside `package*.json` before `npm ci` in every installing stage, and pin `FROM` to the `.nvmrc` major with a unit test checking the two against each other instead of a floating tag; see pierreb-devkit/Node#4060 diff --git a/scripts/tests/dockerfileToolchain.unit.tests.js b/scripts/tests/dockerfileToolchain.unit.tests.js new file mode 100644 index 000000000..589cb6884 --- /dev/null +++ b/scripts/tests/dockerfileToolchain.unit.tests.js @@ -0,0 +1,138 @@ +/** + * Unit tests for the Dockerfile ↔ toolchain consistency guarded by #4060. + * + * `FROM` cannot read `.nvmrc` at build time, so the Node major is restated + * as a literal in the Dockerfile instead of derived. These tests are the + * drift guard: if the Dockerfile's major and `.nvmrc` are ever edited + * independently, this fails instead of the drift going unnoticed until a + * Node LTS rollover changes what `FROM node:lts-slim` resolves to. + * + * The second test guards the sibling defect from the same issue: `.npmrc` + * (carrying `engine-strict=true`) must be copied into the build context + * before `npm ci` runs, or the engines fail-fast never reaches the + * Docker build. + * + * Both checks are stage-aware: the Dockerfile is split on every top-level + * `FROM` into per-stage blocks, and each check walks every stage instead of + * only the first. A single-occurrence check would miss a multi-stage build + * where an earlier stage (e.g. a build stage) is correct but a later one + * (e.g. the runtime stage that actually ships) drifts. + * + * Scope, decided deliberately for #4060's review: + * - A `FROM` whose image is not `node:...` (a distroless/scratch runtime, + * or `FROM `) has no Node major to check, so it's + * skipped by the major-pin test — but if that stage runs `npm ci`, it's + * still held to the .npmrc-before-npm-ci rule. + * - The .npmrc check is stage-local only: it does not trace whether a + * stage that `FROM`s a previous *named* stage inherits an already-copied + * `.npmrc` from that parent. Resolving that means walking the FROM + * graph, which is a Dockerfile parser, not a drift guard for one file. + * A stage that runs `npm ci` is expected to COPY `.npmrc` itself; widen + * this if that inheritance pattern shows up for real. + * - A digest-pinned image (`node:24-slim@sha256:...`) is accepted the + * same as its tag-only form for the major check. + * - Stage boundaries are detected separately from image-token parsing: any + * line starting with `FROM` opens a new stage, but if its shape can't be + * parsed (e.g. a `--platform=...` flag, which this guard doesn't + * support) the test throws instead of silently merging it into the + * previous stage — a strict-regex-as-splitter would make an + * unrecognized `FROM` vanish rather than fail, which is the same class + * of blind spot #4060's review flagged in the first place. + */ +import { describe, test, expect } from '@jest/globals'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; + +const rootDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const dockerfile = readFileSync(path.join(rootDir, 'Dockerfile'), 'utf8'); + +/** + * Splits the Dockerfile into per-stage blocks. A new stage starts at every + * top-level `FROM` line; each block runs from its `FROM` up to (but not + * including) the next `FROM`, or end of file for the last stage. + * + * Boundary detection (which lines start a stage) is intentionally separate + * from image-token extraction (what the stage's image is): matching both + * in one strict regex would make a `FROM` line the regex doesn't recognize + * silently disappear into the previous stage's block instead of failing — + * exactly the kind of miss this guard exists to prevent. So any line that + * starts with `FROM` opens a stage; if its shape can't then be parsed, the + * test throws rather than guessing. + * + * @param {string} text - the full Dockerfile contents + * @returns {Array<{image: string, block: string}>} one entry per stage, in file + * order: `image` is the raw image token from the `FROM` line (tag or + * digest-pinned, or a prior stage's alias), `block` is that stage's lines up + * to the next `FROM` or end of file. + * @throws {Error} if a `FROM` line's shape cannot be parsed, rather than + * silently folding it into the previous stage. + */ +function getStages(text) { + const fromLineRe = /^FROM\b[^\n]*$/gim; + const imageRe = /^FROM[ \t]+(\S+)(?:[ \t]+AS[ \t]+\S+)?[ \t]*$/i; + + const froms = []; + let match; + while ((match = fromLineRe.exec(text)) !== null) { + const line = match[0]; + const imageMatch = line.match(imageRe); + if (!imageMatch) { + throw new Error( + `Dockerfile has a FROM line the stage-splitter can't parse (e.g. a --platform flag ` + + `isn't supported) — extend getStages() in dockerfileToolchain.unit.tests.js or ` + + `simplify the line: "${line}"`, + ); + } + froms.push({ index: match.index, image: imageMatch[1] }); + } + + return froms.map((from, i) => ({ + image: from.image, + block: text.slice(from.index, i + 1 < froms.length ? froms[i + 1].index : text.length), + })); +} + +describe('Dockerfile toolchain pin (#4060)', () => { + test('every FROM node:-slim stage matches the major pinned in .nvmrc', () => { + const nvmrcMajor = readFileSync(path.join(rootDir, '.nvmrc'), 'utf8') + .trim() + .replace(/^v/, '') + .split('.')[0]; + + const stages = getStages(dockerfile); + const nodeStages = stages.filter((stage) => /^node:/.test(stage.image)); + expect(nodeStages.length).toBeGreaterThan(0); + + for (const stage of nodeStages) { + const majorMatch = stage.image.match(/^node:(\d+)(?:\.\d+)*-slim(?:@sha256:[0-9a-f]{64})?$/); + if (!majorMatch) { + throw new Error( + `FROM ${stage.image} is a node image but not a pinned "node:-slim" form ` + + `(.nvmrc pins major ${nvmrcMajor}) — a floating tag like "node:lts-slim" defeats the pin`, + ); + } + expect(majorMatch[1]).toBe(nvmrcMajor); + } + }); + + test('.npmrc is copied before npm ci runs, in every stage that runs npm ci', () => { + const npmCiRe = /^RUN[^\n]*npm ci/m; + const npmrcCopyRe = /^COPY[^\n]*\.npmrc[^\n]*$/m; + + const stages = getStages(dockerfile); + const npmCiStages = stages.filter((stage) => npmCiRe.test(stage.block)); + expect(npmCiStages.length).toBeGreaterThan(0); + + for (const stage of npmCiStages) { + const npmCiIndex = stage.block.search(npmCiRe); + const copyIndex = stage.block.search(npmrcCopyRe); + if (copyIndex === -1 || copyIndex > npmCiIndex) { + throw new Error( + `Stage "FROM ${stage.image}" runs npm ci without copying .npmrc first in that same ` + + 'stage (engine-strict from .npmrc must be in place before npm ci runs)', + ); + } + } + }); +});