From 1c03f673922fbd19c15b8f94de34d8cb99e75a56 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 16:07:53 -0400 Subject: [PATCH 1/3] fix(sentry): give the build the org and project it needs to create a release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @Sentry/vite-plugin only creates a release and uploads source maps when org, project, authToken and a release name are all present at build time. It reports a missing one with logger.warn and returns — after it has already injected the debug IDs during the bundle phase. So a build with nothing configured and a build that worked perfectly produce indistinguishable artifacts: both have debug IDs in the bundle, and both build green. SENTRY_ORG and SENTRY_PROJECT were Fly *runtime* secrets, which the Docker build cannot see. The plugin's org guard therefore fired on every deploy since 2024-04-16: no release was ever created, and no source map was ever uploaded. Worse, filesToDeleteAfterUpload is honoured in the finally of writeBundle rather than only after a successful upload, so the maps were deleted anyway — leaving symbolication broken on the client too, with nothing left to debug it with. See #16. Passes both as --build-secret, mounts and exports them in the build stage alongside the token, and turns the remaining silent skips into a build failure: the plugin has exactly one chance to upload, so a red build is far cheaper than a release that silently does not exist. tests/sentry-release.test.ts links the three files that have to agree — the workflow that supplies the values, the Dockerfile that mounts them, and the config that reads them. Nothing in the type system did, so dropping one flag was a small edit that disabled the whole pipeline without failing a check. --- .env.example | 9 +++ .github/workflows/deploy.yml | 22 +++++- other/Dockerfile | 17 +++- tests/sentry-release.test.ts | 145 +++++++++++++++++++++++++++++++++++ vite.config.ts | 91 ++++++++++++++++++---- 5 files changed, 262 insertions(+), 22 deletions(-) create mode 100644 tests/sentry-release.test.ts diff --git a/.env.example b/.env.example index 5d89543..8b9eb46 100644 --- a/.env.example +++ b/.env.example @@ -2,6 +2,15 @@ SESSION_SECRET="super-duper-s3cret" HONEYPOT_SECRET="super-duper-s3cret" INTERNAL_COMMAND_TOKEN="some-made-up-token" SENTRY_DSN="your-dsn" +# SENTRY_ORG and SENTRY_PROJECT are BUILD-TIME only — @sentry/vite-plugin reads +# them while the image is being built (vite.config.ts) and never again, so the +# deploy workflow passes them to `flyctl deploy --build-secret` and +# other/Dockerfile mounts them into the build stage. Setting them here has no +# effect on a deployed app. They are listed for the local case only: a local +# `npm run build` skips release management entirely unless SENTRY_AUTH_TOKEN is +# also set, and the plugin then refuses to build if any of the three is missing. +SENTRY_ORG="your-org-slug" +SENTRY_PROJECT="your-project-slug" NODEMAILER_HOST="your-host" NODEMAILER_USER="your-email" diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index fd93ceb..e08af4e 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -96,10 +96,26 @@ jobs: - name: 🎈 Setup Fly uses: superfly/flyctl-actions/setup-flyctl@1.5 + # `run: |` rather than a folded plain scalar, so Prettier cannot reflow + # this command and wrap a flag across a line boundary. + # + # SENTRY_ORG and SENTRY_PROJECT are build-time only: @sentry/vite-plugin + # reads them while the Docker image is being built, and never again. They + # used to be Fly *runtime* secrets instead, which put them in the running + # container where the build cannot see them — the plugin's `org` guard then + # fired, and it silently skipped creating the release and uploading source + # maps while still injecting debug IDs, so the build stayed green. See #16. + # + # Every name passed here must also be mounted and exported in the build + # stage of other/Dockerfile. tests/sentry-release.test.ts fails if the two + # drift apart. - name: 🚀 Deploy Production if: ${{ github.ref == 'refs/heads/main' }} - run: - flyctl deploy --remote-only --build-arg COMMIT_SHA=${{ github.sha }} - --build-secret SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} + run: | + flyctl deploy --remote-only \ + --build-arg COMMIT_SHA=${{ github.sha }} \ + --build-secret SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} \ + --build-secret SENTRY_ORG=${{ secrets.SENTRY_ORG }} \ + --build-secret SENTRY_PROJECT=${{ secrets.SENTRY_PROJECT }} env: FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} diff --git a/other/Dockerfile b/other/Dockerfile index 02e7464..2c3e8d6 100644 --- a/other/Dockerfile +++ b/other/Dockerfile @@ -37,9 +37,22 @@ COPY --from=deps /myapp/node_modules /myapp/node_modules ADD . . -# Mount the secret and set it as an environment variable and run the build +# Mount the secrets and set them as environment variables and run the build. +# Each one needs its own --mount *and* its own export, and every line has to +# carry the continuation: without it the mounts and the exports collapse into a +# single RUN and the shell reads `export` as an argument to the last mount. +# +# The exports are chained with &&, so a secret that was not passed short-circuits +# and fails the build here. That is the point — @sentry/vite-plugin reacts to a +# missing SENTRY_ORG by logging a warning, skipping release creation and skipping +# the source map upload, and then deleting the maps anyway in the `finally` of +# its writeBundle hook. A red build is the cheap outcome. See #16. RUN --mount=type=secret,id=SENTRY_AUTH_TOKEN \ - export SENTRY_AUTH_TOKEN=$(cat /run/secrets/SENTRY_AUTH_TOKEN) && \ + --mount=type=secret,id=SENTRY_ORG \ + --mount=type=secret,id=SENTRY_PROJECT \ + export SENTRY_AUTH_TOKEN="$(cat /run/secrets/SENTRY_AUTH_TOKEN)" && \ + export SENTRY_ORG="$(cat /run/secrets/SENTRY_ORG)" && \ + export SENTRY_PROJECT="$(cat /run/secrets/SENTRY_PROJECT)" && \ npm run build # Finally, build the production image with minimal footprint diff --git a/tests/sentry-release.test.ts b/tests/sentry-release.test.ts new file mode 100644 index 0000000..0fdc859 --- /dev/null +++ b/tests/sentry-release.test.ts @@ -0,0 +1,145 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it } from 'vitest' + +/** + * `@sentry/vite-plugin` creates a Sentry release and uploads source maps only + * when every value it needs is present at build time. It signals a missing one + * with `logger.warn` and then returns, *after* it has already injected the debug + * IDs during the bundle phase. A build with nothing configured and a build that + * worked perfectly therefore produce indistinguishable artifacts: both have + * debug IDs in the bundle and both build green. + * + * That is not theoretical. From 2024-04-16 until 2026-09-29 the deploy workflow + * passed `SENTRY_AUTH_TOKEN` as a build secret but kept `SENTRY_ORG` and + * `SENTRY_PROJECT` as Fly *runtime* secrets, where the Docker build cannot see + * them. The plugin's `org` guard fired on every deploy, no release was ever + * created, and no source map was ever uploaded. Worse, `filesToDeleteAfterUpload` + * is honoured in the `finally` of `writeBundle` rather than only after a + * successful upload, so the maps were deleted anyway — leaving symbolication + * broken on the client too, with nothing left to debug it with. See #16. + * + * Nothing in the type system links the three files that have to agree: the + * workflow that supplies the values, the Dockerfile that mounts them, and the + * config that reads them. Dropping one `--build-secret` line, or one + * `--mount`/`export` pair, is a small edit that disables the whole pipeline + * without failing a single check. These tests are that link. + * + * The build-time assertion in `getSentryPlugin()` covers a value being *absent + * at build time*. It cannot cover one being absent from the wiring, because by + * then the assertion and the plugin are reading the same empty variable and + * agree with each other. That is what these are for. + */ + +const VITE_CONFIG = 'vite.config.ts' +const DOCKERFILE = 'other/Dockerfile' +const DEPLOY_WORKFLOW = '.github/workflows/deploy.yml' + +const read = (path: string) => readFileSync(path, 'utf8') + +/** Every distinct capture of `pattern` in `source`, sorted. */ +function matchAll(source: string, pattern: RegExp) { + return [ + ...new Set([...source.matchAll(pattern)].map((match) => match[1])), + ].sort() +} + +/** + * The `as build` stage of the Dockerfile, which is the only stage that runs + * `npm run build` and therefore the only one these values have to reach. + * + * Scoped to that stage because `ARG` does not cross a `FROM`, so a value + * declared in one stage says nothing about the next, and a `--mount` outside + * the build stage would be a different mechanism entirely. + */ +function buildStage() { + const dockerfile = read(DOCKERFILE) + const start = dockerfile.indexOf('FROM base as build') + const end = dockerfile.indexOf('\nFROM base\n') + expect( + start, + `could not find the \`FROM base as build\` stage in ${DOCKERFILE}`, + ).toBeGreaterThan(-1) + expect( + end, + `could not find the final \`FROM base\` stage in ${DOCKERFILE}`, + ).toBeGreaterThan(start) + return dockerfile.slice(start, end) +} + +/** + * Every build input `@sentry/vite-plugin` reads, as named in `vite.config.ts`. + * + * Matched as a pattern rather than listed literally, so adding a value to + * `SENTRY_RELEASE_ENV` makes this test demand the wiring for it instead of + * silently continuing to pass. + */ +function envUsedByPlugin() { + return matchAll( + read(VITE_CONFIG), + /process\.env\.(SENTRY_[A-Z_]+|COMMIT_SHA)\b/g, + ) +} + +/** Everything `flyctl deploy` was told to make available to the build. */ +function buildInputsFromWorkflow() { + return matchAll( + read(DEPLOY_WORKFLOW), + /--build-(?:secret|arg)\s+(SENTRY_[A-Z_]+|COMMIT_SHA)=/g, + ) +} + +/** Secrets the build stage mounts under `/run/secrets`. */ +function mountedSecrets() { + return matchAll(buildStage(), /--mount=type=secret,id=(SENTRY_[A-Z_]+)\b/g) +} + +/** + * Secrets the build stage reads back out of `/run/secrets` and exports. A mount + * on its own puts a file there and sets no environment variable, so the export + * is a separate, separately-omittable step. + */ +function exportedSecrets() { + return matchAll( + buildStage(), + /export\s+(SENTRY_[A-Z_]+)="\$\(cat \/run\/secrets\//g, + ) +} + +describe('Sentry release management build inputs', () => { + it('reads the four environment variables the plugin needs', () => { + // Guards the three comparisons below. If a refactor changes how the + // config reads its environment, every matcher here returns [] and the + // assertions after this one all pass vacuously — the exact failure mode + // this file exists to prevent. + expect(envUsedByPlugin()).toEqual([ + 'COMMIT_SHA', + 'SENTRY_AUTH_TOKEN', + 'SENTRY_ORG', + 'SENTRY_PROJECT', + ]) + }) + + it('passes every one of them to flyctl deploy', () => { + expect(buildInputsFromWorkflow()).toEqual(envUsedByPlugin()) + }) + + it('mounts every Sentry secret in the build stage', () => { + // COMMIT_SHA is deliberately absent: it is a build arg, not a secret, and + // reaches the build stage through `ARG COMMIT_SHA` instead. + expect(mountedSecrets()).toEqual( + envUsedByPlugin().filter((name) => name !== 'COMMIT_SHA'), + ) + }) + + it('exports every mounted secret, because --mount alone sets no variable', () => { + expect(exportedSecrets()).toEqual(mountedSecrets()) + }) + + it('gives the Dockerfile a build arg for the commit, which is not a secret', () => { + const dockerfile = buildStage() + expect(dockerfile).toMatch(/^ARG COMMIT_SHA$/m) + // `ENV` too, because `ARG` alone is not visible to the RUN that runs + // `npm run build` — and `getSentryPlugin()` reads process.env.COMMIT_SHA. + expect(dockerfile).toMatch(/^ENV COMMIT_SHA=\$COMMIT_SHA$/m) + }) +}) diff --git a/vite.config.ts b/vite.config.ts index a44555f..f4fbb97 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -7,6 +7,79 @@ import { iconsSpritesheet } from 'vite-plugin-icons-spritesheet' const MODE = process.env.NODE_ENV +/** + * Every value `@sentry/vite-plugin` needs before it will create a release and + * upload source maps. Each one has exactly one supplier, and they live in three + * different files: the deploy workflow passes it as a `--build-secret` or + * `--build-arg`, the build stage of `other/Dockerfile` mounts and exports it, + * and the plugin below reads it. `tests/sentry-release.test.ts` fails if those + * three drift apart. + */ +const SENTRY_RELEASE_ENV = { + SENTRY_ORG: process.env.SENTRY_ORG, + SENTRY_PROJECT: process.env.SENTRY_PROJECT, + // The release is named after the commit. The server SDK also tags its events + // with it (see `server/utils/monitoring.ts`), so it is the one value that has + // to mean the same thing at build time and at runtime. + COMMIT_SHA: process.env.COMMIT_SHA, +} as Record + +/** + * Returns the Sentry plugin, or `null` when release management is off. + * + * A missing option is not an error as far as the plugin is concerned: it logs + * `logger.warn` and returns from `createRelease` and from `uploadSourcemaps`, + * having already injected the debug IDs during the bundle phase. The result is + * that a completely misconfigured build produces exactly the same artifacts as a + * working one — debug IDs present, `SENTRY_RELEASE` set — while the release + * never exists in Sentry and no source maps are ever uploaded. Every deploy up + * to 2026-09-29 looked fine and was not, because the workflow passed + * `SENTRY_AUTH_TOKEN` but not `SENTRY_ORG`. + * + * The upload is the plugin's only shot: it runs at build time, and it is gone by + * the time anything is deployed. There is no retry and no fallback, so when the + * plugin is going to do work, refuse to build until the values are all there. + * A red build is much cheaper than a release that silently does not exist. + */ +function getSentryPlugin() { + const authToken = process.env.SENTRY_AUTH_TOKEN + + // A disabled plugin returns before it validates anything, so there is nothing + // worth asserting about a development build. + if (!authToken || MODE !== 'production') return null + + const missing = Object.keys(SENTRY_RELEASE_ENV).filter( + (name) => !SENTRY_RELEASE_ENV[name], + ) + if (missing.length > 0) { + const listed = missing.join(', ') + const pronoun = missing.length === 1 ? 'is' : 'are' + const object = missing.length === 1 ? 'it' : 'them' + throw new Error( + `Sentry release management is enabled (SENTRY_AUTH_TOKEN is set) but ${listed} ${pronoun} missing from the build environment, so @sentry/vite-plugin would log a warning and skip creating the release and uploading source maps. Set ${object} in the build: the deploy workflow passes ${object} to "flyctl deploy" and other/Dockerfile mounts ${object} into the build stage. To build without release management, unset SENTRY_AUTH_TOKEN.`, + ) + } + + // Narrowed by the check above; the cast keeps that fact across the boundary + // without a non-null assertion on each lookup. + const releaseEnv = SENTRY_RELEASE_ENV as Record + + return sentryVitePlugin({ + authToken, + org: releaseEnv.SENTRY_ORG, + project: releaseEnv.SENTRY_PROJECT, + release: { + name: releaseEnv.COMMIT_SHA, + setCommits: { + auto: true, + }, + }, + sourcemaps: { + filesToDeleteAfterUpload: ['./build/**/*.map'], + }, + }) +} + export default defineConfig({ environments: { // server/app.ts is the entry of the SSR build rather than React @@ -80,22 +153,6 @@ export default defineConfig({ plugins: ['babel-plugin-react-compiler'], }, }), - process.env.SENTRY_AUTH_TOKEN - ? sentryVitePlugin({ - disable: MODE !== 'production', - authToken: process.env.SENTRY_AUTH_TOKEN, - org: process.env.SENTRY_ORG, - project: process.env.SENTRY_PROJECT, - release: { - name: process.env.COMMIT_SHA, - setCommits: { - auto: true, - }, - }, - sourcemaps: { - filesToDeleteAfterUpload: ['./build/**/*.map'], - }, - }) - : null, + getSentryPlugin(), ], }) From 97c5c3ea16112031f3a1ff6fb2b4495d3043d10e Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 16:11:21 -0400 Subject: [PATCH 2/3] fix(sentry): pass build secrets through step env instead of the script text Interpolating ${{ secrets.* }} puts the value into the rendered command. The step already passes FLY_API_TOKEN through `env:`, so this follows the file's own convention and keeps the secrets out of the script body. Adds a test for the resulting indirection: a --build-secret whose shell variable has no matching `env:` entry would forward the literal string $SENTRY_ORG, and the build would fail far from the cause. --- .github/workflows/deploy.yml | 11 ++++++++--- tests/sentry-release.test.ts | 27 +++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index e08af4e..98511cf 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -114,8 +114,13 @@ jobs: run: | flyctl deploy --remote-only \ --build-arg COMMIT_SHA=${{ github.sha }} \ - --build-secret SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} \ - --build-secret SENTRY_ORG=${{ secrets.SENTRY_ORG }} \ - --build-secret SENTRY_PROJECT=${{ secrets.SENTRY_PROJECT }} + --build-secret "SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN" \ + --build-secret "SENTRY_ORG=$SENTRY_ORG" \ + --build-secret "SENTRY_PROJECT=$SENTRY_PROJECT" env: FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} + # Passed through the environment rather than interpolated into the + # script with ${{ }}, so the values are never part of the command text. + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + SENTRY_ORG: ${{ secrets.SENTRY_ORG }} + SENTRY_PROJECT: ${{ secrets.SENTRY_PROJECT }} diff --git a/tests/sentry-release.test.ts b/tests/sentry-release.test.ts index 0fdc859..50f7866 100644 --- a/tests/sentry-release.test.ts +++ b/tests/sentry-release.test.ts @@ -84,11 +84,19 @@ function envUsedByPlugin() { function buildInputsFromWorkflow() { return matchAll( read(DEPLOY_WORKFLOW), - /--build-(?:secret|arg)\s+(SENTRY_[A-Z_]+|COMMIT_SHA)=/g, + // The name may be quoted, because its value is a shell expansion rather + // than a ${{ }} interpolation. + /--build-(?:secret|arg)\s+"?([A-Z_]+)=/g, ) } -/** Secrets the build stage mounts under `/run/secrets`. */ +/** + * Secrets the build stage mounts under `/run/secrets`. + * + * Deliberately scoped to the `as build` stage. `ARG` does not cross a `FROM`, + * so each stage has to redeclare what it needs, and a `--mount` in the runtime + * stage would be a different mechanism entirely. + */ function mountedSecrets() { return matchAll(buildStage(), /--mount=type=secret,id=(SENTRY_[A-Z_]+)\b/g) } @@ -142,4 +150,19 @@ describe('Sentry release management build inputs', () => { // `npm run build` — and `getSentryPlugin()` reads process.env.COMMIT_SHA. expect(dockerfile).toMatch(/^ENV COMMIT_SHA=\$COMMIT_SHA$/m) }) + + it('supplies every build secret from the step env, not a ${{ }} interpolation', () => { + // The deploy step passes each secret as `--build-secret "NAME=$NAME"`, so + // the value is a shell expansion. If the `env:` entry behind it were + // dropped, `flyctl` would forward the literal string `$SENTRY_ORG` as a + // secret and the build would fail far from the cause. `COMMIT_SHA` is + // excluded because it is an interpolated `github.sha`, not a secret. + const suppliedByEnv = matchAll( + read(DEPLOY_WORKFLOW), + /^\s+(SENTRY_[A-Z_]+): \$\{\{ secrets\./gm, + ) + expect(suppliedByEnv).toEqual( + buildInputsFromWorkflow().filter((name) => name !== 'COMMIT_SHA'), + ) + }) }) From 6c9a9a756d3355de49dba0fcf6bace2ef0da1321 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 16:59:59 -0400 Subject: [PATCH 3/3] fix(sentry): close the ways a release can still silently not happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass made the *wiring* correct, but a review of the installed plugin turned up five ways a build could still end up green with no release, no source maps, and no evidence either was attempted. - errorHandler: the plugin's documented default is to throw and stop the build; the shipped 11.0.0 implementation logs and continues. Restoring the documented behaviour is what makes the guard in this file mean anything, because `filesToDeleteAfterUpload` is honoured in the `finally` of its writeBundle hook whether or not the upload succeeded. A swallowed failure deleted every source map from the image and still exited zero. - setCommits: dropped. Naming `auto` explicitly opts out of the `shouldNotThrowOnFailure` the plugin's own default carries, so a failure rethrew and skipped `finalizeRelease`, leaving the release stuck in `new` on every deploy. It could not have succeeded anyway — the base image has no `git`, so there is no repository for `--auto` to read. - required=true on the secret mounts. The `&&` chain does not do this: `export X="$(cat /run/secrets/X)"` exits 0 even when the cat fails, so a missing secret arrived as an empty string rather than stopping the build. - The deploy step now rejects an empty secret before calling flyctl. An unset GitHub secret is the empty string, not an unset variable, so a deleted SENTRY_AUTH_TOKEN took the "release management is off" branch: green build, no release, no debug IDs, nothing in the log. - .env.sentry-build-plugin is now gitignored and dockerignored. The plugin merges it into process.env, and it holds an auth token; neither ignore file covered it, and `ADD . .` copies it into both image stages. Also treats an empty value as absent rather than merely undefined, tests getSentryPlugin() on both branches, and guards the build/runtime boundary: the three build-only credentials are asserted to be unread by the running app, which is what makes removing them from the Fly runtime environment safe. That removal is the outstanding operational step of #16. --- .env.example | 15 ++- .github/workflows/deploy.yml | 11 ++ .gitignore | 5 +- other/Dockerfile | 27 ++-- other/Dockerfile.dockerignore | 3 + tests/sentry-release.test.ts | 239 +++++++++++++++++++++++++++++----- vite.config.ts | 114 +++++++++------- 7 files changed, 317 insertions(+), 97 deletions(-) diff --git a/.env.example b/.env.example index 8b9eb46..4e04457 100644 --- a/.env.example +++ b/.env.example @@ -6,11 +6,16 @@ SENTRY_DSN="your-dsn" # them while the image is being built (vite.config.ts) and never again, so the # deploy workflow passes them to `flyctl deploy --build-secret` and # other/Dockerfile mounts them into the build stage. Setting them here has no -# effect on a deployed app. They are listed for the local case only: a local -# `npm run build` skips release management entirely unless SENTRY_AUTH_TOKEN is -# also set, and the plugin then refuses to build if any of the three is missing. -SENTRY_ORG="your-org-slug" -SENTRY_PROJECT="your-project-slug" +# effect on a deployed app. +# +# Left empty rather than filled with a placeholder on purpose: a local build +# skips release management unless SENTRY_AUTH_TOKEN is also set, and once it is, +# getSentryPlugin() treats an empty value as missing and refuses to build. +# A placeholder like "your-org-slug" would pass that check and send an +# authenticated request to an org that does not exist, which the plugin logs and +# then swallows — a green build and no release, which is the bug this guards. +SENTRY_ORG="" +SENTRY_PROJECT="" NODEMAILER_HOST="your-host" NODEMAILER_USER="your-email" diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 98511cf..4871f0a 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -112,6 +112,17 @@ jobs: - name: 🚀 Deploy Production if: ${{ github.ref == 'refs/heads/main' }} run: | + # An unset secret is the empty string, not an unset variable, so a + # deleted SENTRY_AUTH_TOKEN would arrive as "" and turn + # `getSentryPlugin()`'s "off" branch on — a green deploy with no + # release, no debug IDs and no evidence anything was skipped. Check + # here, where the name of the missing secret is still obvious. + for name in SENTRY_AUTH_TOKEN SENTRY_ORG SENTRY_PROJECT; do + if [ -z "${!name}" ]; then + echo "::error::$name is unset or empty in this repository's secrets" + exit 1 + fi + done flyctl deploy --remote-only \ --build-arg COMMIT_SHA=${{ github.sha }} \ --build-secret "SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN" \ diff --git a/.gitignore b/.gitignore index d618829..67109a0 100644 --- a/.gitignore +++ b/.gitignore @@ -22,4 +22,7 @@ node_modules # generated files /app/components/ui/icons -.react-router/ \ No newline at end of file +.react-router/ +# Read by @sentry/vite-plugin at build time and merged into process.env. +# Holds a Sentry auth token, so it must never be committed or baked into an image. +.env.sentry-build-plugin diff --git a/other/Dockerfile b/other/Dockerfile index 2c3e8d6..80d6c4e 100644 --- a/other/Dockerfile +++ b/other/Dockerfile @@ -37,19 +37,22 @@ COPY --from=deps /myapp/node_modules /myapp/node_modules ADD . . -# Mount the secrets and set them as environment variables and run the build. -# Each one needs its own --mount *and* its own export, and every line has to -# carry the continuation: without it the mounts and the exports collapse into a -# single RUN and the shell reads `export` as an argument to the last mount. +# Mount the secrets and run the build. # -# The exports are chained with &&, so a secret that was not passed short-circuits -# and fails the build here. That is the point — @sentry/vite-plugin reacts to a -# missing SENTRY_ORG by logging a warning, skipping release creation and skipping -# the source map upload, and then deleting the maps anyway in the `finally` of -# its writeBundle hook. A red build is the cheap outcome. See #16. -RUN --mount=type=secret,id=SENTRY_AUTH_TOKEN \ - --mount=type=secret,id=SENTRY_ORG \ - --mount=type=secret,id=SENTRY_PROJECT \ +# `required=true` is what makes a missing secret fail the build. It looks like the +# `&&` chain below would do it instead, but it would not: `export X="$(cat +# /run/secrets/X)"` exits 0 even when the `cat` fails, because a failed command +# substitution does not propagate through `export`. Without `required=true` an +# absent secret silently becomes an empty string, and an empty string is exactly +# what `getSentryPlugin()` in vite.config.ts treats as missing — so the build +# would still fail, several layers later and with a less obvious message. +# +# The `cat`s are needed because a secret mount sets no environment variable on +# its own; only the `env=` mount form does, and that needs a Dockerfile syntax +# pin this file does not currently carry. +RUN --mount=type=secret,id=SENTRY_AUTH_TOKEN,required=true \ + --mount=type=secret,id=SENTRY_ORG,required=true \ + --mount=type=secret,id=SENTRY_PROJECT,required=true \ export SENTRY_AUTH_TOKEN="$(cat /run/secrets/SENTRY_AUTH_TOKEN)" && \ export SENTRY_ORG="$(cat /run/secrets/SENTRY_ORG)" && \ export SENTRY_PROJECT="$(cat /run/secrets/SENTRY_PROJECT)" && \ diff --git a/other/Dockerfile.dockerignore b/other/Dockerfile.dockerignore index 6e18765..5cbd611 100644 --- a/other/Dockerfile.dockerignore +++ b/other/Dockerfile.dockerignore @@ -7,3 +7,6 @@ /.cache /public/build /build + +# Read by @sentry/vite-plugin at build time. Holds a Sentry auth token. +.env.sentry-build-plugin diff --git a/tests/sentry-release.test.ts b/tests/sentry-release.test.ts index 50f7866..5cac095 100644 --- a/tests/sentry-release.test.ts +++ b/tests/sentry-release.test.ts @@ -1,5 +1,6 @@ -import { readFileSync } from 'node:fs' -import { describe, expect, it } from 'vitest' +import { existsSync, readFileSync, readdirSync, statSync } from 'node:fs' +import { extname, join } from 'node:path' +import { describe, expect, it, vi } from 'vitest' /** * `@sentry/vite-plugin` creates a Sentry release and uploads source maps only @@ -13,10 +14,7 @@ import { describe, expect, it } from 'vitest' * passed `SENTRY_AUTH_TOKEN` as a build secret but kept `SENTRY_ORG` and * `SENTRY_PROJECT` as Fly *runtime* secrets, where the Docker build cannot see * them. The plugin's `org` guard fired on every deploy, no release was ever - * created, and no source map was ever uploaded. Worse, `filesToDeleteAfterUpload` - * is honoured in the `finally` of `writeBundle` rather than only after a - * successful upload, so the maps were deleted anyway — leaving symbolication - * broken on the client too, with nothing left to debug it with. See #16. + * created, and no source map was ever uploaded. See #16. * * Nothing in the type system links the three files that have to agree: the * workflow that supplies the values, the Dockerfile that mounts them, and the @@ -24,16 +22,35 @@ import { describe, expect, it } from 'vitest' * `--mount`/`export` pair, is a small edit that disables the whole pipeline * without failing a single check. These tests are that link. * - * The build-time assertion in `getSentryPlugin()` covers a value being *absent - * at build time*. It cannot cover one being absent from the wiring, because by - * then the assertion and the plugin are reading the same empty variable and - * agree with each other. That is what these are for. + * They are about the *wiring*, not the values. `getSentryPlugin()` already + * refuses to build when a value is absent or empty, so a test here could not + * catch a name that never reaches it — by then the guard and the plugin are + * reading the same empty variable and agree with each other. */ const VITE_CONFIG = 'vite.config.ts' const DOCKERFILE = 'other/Dockerfile' const DEPLOY_WORKFLOW = '.github/workflows/deploy.yml' +/** Directories that make up the running application. */ +const RUNTIME_ROOTS = ['app', 'server', 'index.ts'] + +/** + * The one Sentry variable the deploy must leave in place, and the three it must + * remove once the build has been proven to work. + * + * `SENTRY_DSN` is read at runtime by `server/utils/monitoring.ts` and shipped to + * the browser by `getEnv()`. The other three are read only by `vite.config.ts`, + * which is build-time, so leaving them in the runtime environment means holding + * a Sentry API token in the production process for no reason. + */ +const RUNTIME_ONLY = 'SENTRY_DSN' +const BUILD_ONLY = [ + 'SENTRY_AUTH_TOKEN', + 'SENTRY_ORG', + 'SENTRY_PROJECT', +] as const + const read = (path: string) => readFileSync(path, 'utf8') /** Every distinct capture of `pattern` in `source`, sorted. */ @@ -48,8 +65,8 @@ function matchAll(source: string, pattern: RegExp) { * `npm run build` and therefore the only one these values have to reach. * * Scoped to that stage because `ARG` does not cross a `FROM`, so a value - * declared in one stage says nothing about the next, and a `--mount` outside - * the build stage would be a different mechanism entirely. + * declared in one stage says nothing about the next, and a `--mount` in the + * runtime stage would be a different mechanism entirely. */ function buildStage() { const dockerfile = read(DOCKERFILE) @@ -67,11 +84,11 @@ function buildStage() { } /** - * Every build input `@sentry/vite-plugin` reads, as named in `vite.config.ts`. + * Every build input `getSentryPlugin()` reads, as named in `vite.config.ts`. * * Matched as a pattern rather than listed literally, so adding a value to - * `SENTRY_RELEASE_ENV` makes this test demand the wiring for it instead of - * silently continuing to pass. + * `getSentryEnv()` makes this test demand the wiring for it instead of silently + * continuing to pass. */ function envUsedByPlugin() { return matchAll( @@ -90,13 +107,7 @@ function buildInputsFromWorkflow() { ) } -/** - * Secrets the build stage mounts under `/run/secrets`. - * - * Deliberately scoped to the `as build` stage. `ARG` does not cross a `FROM`, - * so each stage has to redeclare what it needs, and a `--mount` in the runtime - * stage would be a different mechanism entirely. - */ +/** Secrets the build stage mounts under `/run/secrets`. */ function mountedSecrets() { return matchAll(buildStage(), /--mount=type=secret,id=(SENTRY_[A-Z_]+)\b/g) } @@ -113,18 +124,33 @@ function exportedSecrets() { ) } +/** Every source file that makes up the running application. */ +function runtimeSources() { + const extensions = new Set(['.ts', '.tsx', '.js', '.jsx', '.mjs']) + const files: string[] = [] + const walk = (path: string) => { + if (!existsSync(path)) return + if (!statSync(path).isDirectory()) { + if (extensions.has(extname(path))) files.push(path) + return + } + for (const entry of readdirSync(path, { withFileTypes: true })) { + // Generated, gitignored, and not part of the application. + if (entry.name === 'icons') continue + walk(join(path, entry.name)) + } + } + for (const root of RUNTIME_ROOTS) walk(root) + return files +} + describe('Sentry release management build inputs', () => { it('reads the four environment variables the plugin needs', () => { // Guards the three comparisons below. If a refactor changes how the // config reads its environment, every matcher here returns [] and the // assertions after this one all pass vacuously — the exact failure mode // this file exists to prevent. - expect(envUsedByPlugin()).toEqual([ - 'COMMIT_SHA', - 'SENTRY_AUTH_TOKEN', - 'SENTRY_ORG', - 'SENTRY_PROJECT', - ]) + expect(envUsedByPlugin()).toEqual(['COMMIT_SHA', ...[...BUILD_ONLY].sort()]) }) it('passes every one of them to flyctl deploy', () => { @@ -143,6 +169,22 @@ describe('Sentry release management build inputs', () => { expect(exportedSecrets()).toEqual(mountedSecrets()) }) + it('marks every secret mount required, so a missing one fails the build', () => { + // The `&&` chain in the Dockerfile does not do this: `export X="$(cat + // /run/secrets/X)"` exits 0 even when the cat fails, so a missing secret + // would arrive as an empty string and get as far as a Sentry request. + const mounts = buildStage() + .split('\n') + .filter((line) => line.includes('--mount=type=secret')) + .map((line) => ({ + id: line.match(/id=(SENTRY_[A-Z_]+)/)?.[1], + required: line.includes('required=true'), + })) + expect(mounts).toEqual( + mountedSecrets().map((id) => ({ id, required: true })), + ) + }) + it('gives the Dockerfile a build arg for the commit, which is not a secret', () => { const dockerfile = buildStage() expect(dockerfile).toMatch(/^ARG COMMIT_SHA$/m) @@ -151,18 +193,147 @@ describe('Sentry release management build inputs', () => { expect(dockerfile).toMatch(/^ENV COMMIT_SHA=\$COMMIT_SHA$/m) }) - it('supplies every build secret from the step env, not a ${{ }} interpolation', () => { + it('backs each build secret with a value in the step env', () => { // The deploy step passes each secret as `--build-secret "NAME=$NAME"`, so // the value is a shell expansion. If the `env:` entry behind it were - // dropped, `flyctl` would forward the literal string `$SENTRY_ORG` as a - // secret and the build would fail far from the cause. `COMMIT_SHA` is - // excluded because it is an interpolated `github.sha`, not a secret. - const suppliedByEnv = matchAll( + // dropped, `flyctl` would forward the literal string `$SENTRY_ORG` and the + // build would fail far from the cause. `COMMIT_SHA` is excluded because it + // is an interpolated `github.sha`, not a secret. + const backedByEnv = matchAll( read(DEPLOY_WORKFLOW), /^\s+(SENTRY_[A-Z_]+): \$\{\{ secrets\./gm, ) - expect(suppliedByEnv).toEqual( + expect(backedByEnv).toEqual( buildInputsFromWorkflow().filter((name) => name !== 'COMMIT_SHA'), ) }) + + it('rejects an empty secret before calling flyctl, since unset means empty', () => { + // GitHub Actions expands an unset repository secret to the empty string, + // not to nothing. So a deleted SENTRY_AUTH_TOKEN would reach + // `getSentryPlugin()` as "" and take its "release management is off" + // branch: a green deploy, no release, no debug IDs, nothing in the log. + // The deploy step has to check before flyctl turns "" into a secret. + expect(read(DEPLOY_WORKFLOW)).toMatch( + /for name in SENTRY_AUTH_TOKEN SENTRY_ORG SENTRY_PROJECT;/, + ) + }) +}) + +describe('Sentry credentials at the build/runtime boundary', () => { + // #16 removes the three build-only secrets from the Fly *runtime* + // environment once the build is proven. That is a one-line, unreviewable + // change to production that silently breaks anything reading them at + // runtime, and today nothing but a human grep establishes that nothing does. + // The issue called this the half that "can silently break production". + + it.each([...BUILD_ONLY])( + '%s is never read by the running application', + (name) => { + const offenders = runtimeSources().filter((file) => + read(file).includes(name), + ) + expect( + offenders, + `${name} is build-time only, so #16 removes it from the Fly runtime environment. These files still read it at runtime and will break when it goes:`, + ).toEqual([]) + }, + ) + + it(`${RUNTIME_ONLY} is still read at runtime, since it must survive`, () => { + // The inverse direction is the dangerous one. `SENTRY_DSN` is the single + // value the cleanup must not take with it, and + // `SENTRY_DSN: z.string()` accepts "", so removing it disables error + // reporting with no boot error and no failing check. + const readers = runtimeSources().filter((file) => + read(file).includes(RUNTIME_ONLY), + ) + expect(readers.length).toBeGreaterThan(0) + }) +}) + +describe('getSentryPlugin()', () => { + /** + * Imports the config fresh under the given environment and hands back its + * `getSentryPlugin()`. + * + * It has to be the *import* that is asserted, not a call afterwards: + * `defineConfig({ plugins: [getSentryPlugin()] })` runs at module scope, so a + * build that is going to fail does so while Vite is loading the config, which + * is exactly when it matters in production. + * + * The reset happens on the way in rather than in an `afterEach`, so nothing + * can leak between cases even though several of them never reach the end of + * the body: a rejected import skips any teardown registered after it. + */ + const loadUnder = async (env: Record) => { + vi.resetModules() + vi.unstubAllEnvs() + for (const [name, value] of Object.entries(env)) { + vi.stubEnv(name, value ?? '') + } + return import(/* @vite-ignore */ `../${VITE_CONFIG}`) + } + + it('is off, and asserts nothing, without an auth token', async () => { + const { getSentryPlugin } = await loadUnder({ NODE_ENV: 'production' }) + // Every local build lands here, so this branch is what keeps `npm run + // build` working on a machine with no Sentry credentials at all. + expect(getSentryPlugin()).toBeNull() + }) + + it('is off outside production, because a disabled plugin never validates', async () => { + const { getSentryPlugin } = await loadUnder({ + NODE_ENV: 'development', + SENTRY_AUTH_TOKEN: 'token', + }) + expect(getSentryPlugin()).toBeNull() + }) + + it.each(['SENTRY_ORG', 'SENTRY_PROJECT', 'COMMIT_SHA'])( + 'refuses to build when %s is unset', + async (missing) => { + await expect( + loadUnder({ + NODE_ENV: 'production', + SENTRY_AUTH_TOKEN: 'token', + SENTRY_ORG: 'org', + SENTRY_PROJECT: 'project', + COMMIT_SHA: 'a'.repeat(40), + [missing]: undefined, + }), + ).rejects.toThrow(new RegExp(missing)) + }, + ) + + it('refuses to build when a secret is present but empty', async () => { + // The case an empty `--build-secret`, or a deleted GitHub secret, + // produces. A presence check would wave it through and the plugin would + // then send an authenticated request with an empty org slug. + await expect( + loadUnder({ + NODE_ENV: 'production', + SENTRY_AUTH_TOKEN: 'token', + SENTRY_ORG: '', + SENTRY_PROJECT: 'project', + COMMIT_SHA: 'a'.repeat(40), + }), + ).rejects.toThrow(/SENTRY_ORG/) + }) + + it('returns the plugin once everything is present', async () => { + const { getSentryPlugin } = await loadUnder({ + NODE_ENV: 'production', + SENTRY_AUTH_TOKEN: 'token', + SENTRY_ORG: 'org', + SENTRY_PROJECT: 'project', + COMMIT_SHA: 'a'.repeat(40), + }) + const plugins = getSentryPlugin() + // An array, which Vite flattens. Asserted so a future change to a single + // plugin object is a deliberate edit rather than a silent one. + expect(Array.isArray(plugins)).toBe(true) + expect(plugins).toHaveLength(1) + expect(plugins?.[0]?.name).toBe('sentry-vite-plugin') + }) }) diff --git a/vite.config.ts b/vite.config.ts index f4fbb97..d3814ad 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -8,71 +8,95 @@ import { iconsSpritesheet } from 'vite-plugin-icons-spritesheet' const MODE = process.env.NODE_ENV /** - * Every value `@sentry/vite-plugin` needs before it will create a release and - * upload source maps. Each one has exactly one supplier, and they live in three - * different files: the deploy workflow passes it as a `--build-secret` or + * Every environment variable `@sentry/vite-plugin` reads to create a release + * and upload source maps. Each one has exactly one supplier, and they live in + * three different files: the deploy workflow passes it as a `--build-secret` or * `--build-arg`, the build stage of `other/Dockerfile` mounts and exports it, - * and the plugin below reads it. `tests/sentry-release.test.ts` fails if those + * and this function reads it. `tests/sentry-release.test.ts` fails if those * three drift apart. + * + * Read inside the function rather than cached at module scope so the guard + * below is a pure function of the environment at the moment it runs, which is + * what makes both of its branches testable. */ -const SENTRY_RELEASE_ENV = { - SENTRY_ORG: process.env.SENTRY_ORG, - SENTRY_PROJECT: process.env.SENTRY_PROJECT, - // The release is named after the commit. The server SDK also tags its events - // with it (see `server/utils/monitoring.ts`), so it is the one value that has - // to mean the same thing at build time and at runtime. - COMMIT_SHA: process.env.COMMIT_SHA, -} as Record +function getSentryEnv() { + return { + SENTRY_AUTH_TOKEN: process.env.SENTRY_AUTH_TOKEN, + SENTRY_ORG: process.env.SENTRY_ORG, + SENTRY_PROJECT: process.env.SENTRY_PROJECT, + // The release is named after the commit. #15 makes the server SDK tag its + // events with the same value, so it is the one variable that has to mean + // the same thing at build time and at runtime. + COMMIT_SHA: process.env.COMMIT_SHA, + } +} /** * Returns the Sentry plugin, or `null` when release management is off. * - * A missing option is not an error as far as the plugin is concerned: it logs + * A missing option is not an error as far as the plugin is concerned. It calls * `logger.warn` and returns from `createRelease` and from `uploadSourcemaps`, - * having already injected the debug IDs during the bundle phase. The result is - * that a completely misconfigured build produces exactly the same artifacts as a - * working one — debug IDs present, `SENTRY_RELEASE` set — while the release - * never exists in Sentry and no source maps are ever uploaded. Every deploy up - * to 2026-09-29 looked fine and was not, because the workflow passed - * `SENTRY_AUTH_TOKEN` but not `SENTRY_ORG`. + * having already injected the debug IDs during the bundle phase. So a build with + * nothing configured and a build that worked perfectly produce indistinguishable + * artifacts: both have debug IDs in the bundle, and both build green. From + * 2024-04-16 until 2026-09-29 the workflow passed `SENTRY_AUTH_TOKEN` but not + * `SENTRY_ORG`, and every deploy was the first kind. See #16. * - * The upload is the plugin's only shot: it runs at build time, and it is gone by - * the time anything is deployed. There is no retry and no fallback, so when the - * plugin is going to do work, refuse to build until the values are all there. - * A red build is much cheaper than a release that silently does not exist. + * The upload is the plugin's only shot: it runs at build time and is gone by the + * time anything is deployed. So when the plugin is going to do work, refuse to + * build until every value it needs is present. */ -function getSentryPlugin() { - const authToken = process.env.SENTRY_AUTH_TOKEN +export function getSentryPlugin() { + const env = getSentryEnv() - // A disabled plugin returns before it validates anything, so there is nothing - // worth asserting about a development build. - if (!authToken || MODE !== 'production') return null + // Unset token means release management is off, which is what every local + // build does. A non-production build never reaches the assertions below: a + // disabled plugin returns from the plugin manager before it validates + // anything, so there would be nothing for them to protect. + if (!env.SENTRY_AUTH_TOKEN || process.env.NODE_ENV !== 'production') { + return null + } - const missing = Object.keys(SENTRY_RELEASE_ENV).filter( - (name) => !SENTRY_RELEASE_ENV[name], - ) + // Falsy, not merely undefined: an empty secret reaches the build as an empty + // file, exports as an empty string, and would otherwise pass a presence check + // and then be sent to Sentry as an empty org slug. + const missing = Object.entries(env) + .filter(([, value]) => !value) + .map(([name]) => name) if (missing.length > 0) { const listed = missing.join(', ') - const pronoun = missing.length === 1 ? 'is' : 'are' - const object = missing.length === 1 ? 'it' : 'them' + const many = missing.length > 1 throw new Error( - `Sentry release management is enabled (SENTRY_AUTH_TOKEN is set) but ${listed} ${pronoun} missing from the build environment, so @sentry/vite-plugin would log a warning and skip creating the release and uploading source maps. Set ${object} in the build: the deploy workflow passes ${object} to "flyctl deploy" and other/Dockerfile mounts ${object} into the build stage. To build without release management, unset SENTRY_AUTH_TOKEN.`, + `Sentry release management is enabled (SENTRY_AUTH_TOKEN is set) but ${listed} ${many ? 'are' : 'is'} missing or empty in the build environment, so @sentry/vite-plugin would warn and skip creating the release and uploading source maps. Set ${many ? 'them' : 'it'} in the build: the deploy workflow passes ${many ? 'them' : 'it'} to "flyctl deploy" and other/Dockerfile mounts ${many ? 'them' : 'it'} into the build stage. To build without release management, unset SENTRY_AUTH_TOKEN.`, ) } - // Narrowed by the check above; the cast keeps that fact across the boundary - // without a non-null assertion on each lookup. - const releaseEnv = SENTRY_RELEASE_ENV as Record - return sentryVitePlugin({ - authToken, - org: releaseEnv.SENTRY_ORG, - project: releaseEnv.SENTRY_PROJECT, + authToken: env.SENTRY_AUTH_TOKEN, + org: env.SENTRY_ORG, + project: env.SENTRY_PROJECT, + // The plugin's documented default is to throw from here and stop the + // build. The shipped 11.0.0 implementation does not: it logs and + // continues. Restoring the documented behaviour is what makes the stance + // above true, and it matters more than it looks — `filesToDeleteAfterUpload` + // below is honoured in the `finally` of the plugin's writeBundle hook + // whether or not the upload succeeded, so a swallowed failure would + // delete every source map from the image and still exit zero. + errorHandler: (error) => { + throw error + }, release: { - name: releaseEnv.COMMIT_SHA, - setCommits: { - auto: true, - }, + name: env.COMMIT_SHA, + // `setCommits` is deliberately omitted. The plugin's own default + // (`{ auto: true, shouldNotThrowOnFailure: true, ... }`) is what makes + // a commit-association failure non-fatal; naming `auto` explicitly + // opts out of `shouldNotThrowOnFailure`, and the resulting throw skips + // `finalizeRelease`, leaving the release stuck in `new` forever. Note + // that `auto` cannot actually succeed in the build stage regardless: + // the base image has no `git`, so there is nothing for it to read a + // repository from. Commit association needs either a Sentry GitHub + // integration or a `sentry-cli releases set-commits` run from the + // deploy job, which has a checkout. }, sourcemaps: { filesToDeleteAfterUpload: ['./build/**/*.map'],