feat(worker): GitHub App authentication, beside the personal access token - #321
Merged
Merged
Conversation
Octokit authenticates as a GitHub App by being handed `{ authStrategy, auth }`
instead of a bare token. createCanopyOctokit accepted only `{ auth: string }`,
so there was no way through it.
The strategy is typed STRUCTURALLY -- `authStrategy: (options) => unknown` --
rather than by importing `@octokit/auth-app`. github-service.ts is reachable
from services.ts:39, so anything it imports lands in every adopter's Next.js
server bundle, including the large majority who use a personal access token
and will never register a GitHub App (registering one needs org-admin rights).
`pnpm lint:bundle` cruises the CLIENT entries only and would not have caught a
server-bundle regression, so the placement had to be decided by hand and is now
held by a dependency-cruiser rule (core-no-github-app-auth) that `pnpm
lint:cycles` already evaluates over the whole of packages/*/src. Verified it
fires: an `import { createAppAuth } from '@octokit/auth-app'` in
github-service.ts turns lint:cycles red.
The bare-token call path is byte-identical -- `{ auth: string }` is still a
member of the union, and the options are now spread into the constructor so
both shapes reach it. `throttle` is written after the spread, so the
rate-limit handlers cannot be displaced by the caller.
redactCredentials had three rules -- URL userinfo, `gh[pousr]_`/`github_pat_`,
and `Bearer <value>` -- and none of them matches either shape GitHub App auth
introduces into worker error text: the App private key itself (it appears in
key-parse and JWT-signing failures) and the bare `eyJ...` app JWT (GitHub
echoes it back in the body of the 401 that rejects it).
That text reaches `task.error` and worker-status.json's `lastFatalError`, both
of which the admin panel serves to a browser.
Both additions are linear, as the file's existing comment about CodeQL
js/polynomial-redos requires. The PEM label is `[A-Z]{0,9} ?` -- bounded, so
its backtracking is a constant factor -- rather than an open `[A-Z ]*`, which
would rescan a long run of capitals at every start position. The lazy body is
terminated by the END footer OR by end-of-string, so a message cut off mid-key
is redacted rather than passed through for want of a terminator. The JWT rule
is three dot-separated base64url runs whose character class excludes `.`.
Adopter request #45. A fine-grained PAT expires within a year, acts as the person who created it, and dies when they leave the organisation; a GitHub App's private key does not expire and installs across repositories. The cost is that an App issues ~1-hour installation tokens, which is why buildGitHubUrl() had to become async first (#319). The token stays first class. CanopyCMS is a public library and registering an App needs org-admin rights most adopters do not have, so `githubToken` is unchanged, undeprecated, warning-free, and still the documented default. Exactly one of the two must be configured; configuring neither is the only error, and that is now caught in the constructor rather than at the first push. The dependency lives in canopycms-cdk, NOT in canopycms: github-service.ts is in every adopter's server bundle (see the previous commit). Core therefore holds only the shapes, and the deployment entrypoint constructs the auth and injects it -- the seam refreshAuthCache already uses. `@octokit/auth-app` is a devDependency of canopycms-cdk, matching @aws-sdk/client-secrets-manager: the worker is shipped pre-bundled by esbuild at prepack. Four things worth reviewing on their own terms: - The PEM is normalized through `crypto.createPrivateKey(pem).export({ type: 'pkcs8' })`. GitHub issues App keys as PKCS#1 (`BEGIN RSA PRIVATE KEY`) and universal-github-app-jwt's WebCrypto path reads only PKCS#8, so which one works depends on the export condition esbuild picks for the bundled worker -- it can work locally and fail on the instance. The same call also accepts base64-wrapped and `\n`-escaped keys, which is where a multi-line secret usually ends up after a trip through configuration. - The credential is preflighted in start()'s try, before ensureRemoteGit(). Otherwise the first failure comes out of the bare clone, whose catch says "the GitHub repository may be empty, or the base branch may not exist" -- sending the operator after a problem that does not exist. start()'s catch records it as lastFatalError in worker-status.json. - A mint failure is NOT wrapped. isPermanentTaskFailure already classifies correctly on an Octokit RequestError's `.status` (4xx permanent, 5xx and status-less transient, 408/429/rate-limit-403 carved out), so the job here was to make sure nothing drops it: a `catch` rethrowing `new Error(getErrorMessage(err))` would silently turn every 401 transient. Tests pin 401 -> permanent, 503 -> transient, and the error identity itself. - The mint is bounded by an AbortSignal.timeout inside resolveGitToken rather than by threading the task's signal through pushBranchToGitHub. git-sync.ts's two resolutions do not run through executeTaskWithTimeout at all, so a threaded task signal would have bounded only one of the two call paths. Nothing caches a tokenized URL. pushBranchToGitHub still resolves exactly once per call (#319's invariant, still pinned); the new tests vary the token ACROSS separate calls, which is where a cache would show. Not in scope: the CDK props and the entrypoint wiring (next PR), and GitHubService/createGitHubService, which stay static-token-only and are filed as .claude/future-tasks/github-service-static-token-only.md.
…ionale Two corrections, both from measuring rather than reasoning. **A real bug.** normalizeGitHubAppPrivateKey unescaped `\n` only BEFORE unwrapping base64, so a PEM that was escaped and THEN base64-wrapped decoded to a body still full of literal backslash-n and threw. The two manglings compose in either order and each hides the other, so the unescape now runs on both sides of the unwrap. Found by the new canopycms-cdk test below, which is the first thing to put the normalizer's output in front of the real signer. The value is also re-trimmed between the passes: unescaping something that ended in a trailing `\n` escape -- which is what `base64 -w 64` output becomes after a single-line config field -- leaves a real trailing newline past the `=` padding, and decodeIfBase64's shape test is anchored at both ends. Trimming inside that function instead would need a `\s*$` tail whose whitespace overlaps the body class, i.e. the polynomial-backtracking shape redactCredentials avoids on purpose. The first version of the test for that case was NOT deterministic: whether a trailing newline appeared at all depended on whether the generated key's base64 length happened to be a multiple of 64. Measured -- it passed against an implementation with no between-passes trim. The trailing newline is now explicit, and the mutation goes red. **The rationale was wrong.** The claim was that an unconverted PKCS#1 key fails under the worker's bundle. Built with the worker's exact flags (`esbuild --bundle --platform=node --target=node22 --format=esm`) and ran it: esbuild resolves `main` -> universal-github-app-jwt@1.2.0's `dist-node` build, which signs through `jsonwebtoken`, and **PKCS#1 works**. Zero WebCrypto references in the bundle. The hazard is real but conditional, and the comment now says which conditions: the same package's `module`/`browser` entry is a WebCrypto build that throws "only PKCS#8 is supported", and @octokit/auth-app@7's universal-github-app-jwt@2 is WebCrypto-only and converts PKCS#1 solely under the `node` export condition (`#crypto` -> lib/crypto-node.js; the `default` sibling's converter is a no-op). So the conversion is insurance against a bundler flag or a dependency bump, not a fix for a failure we have -- and the present-tense value of the helper is the unwrapping and the loud local error. **New:** packages/canopycms-cdk/worker/github-app-auth.test.ts. canopycms cannot host this -- it must never depend on @octokit/auth-app -- so this is the only place the normalizer can be checked against the signer it exists for, and the only place the shared-instance token cache can be demonstrated (one instance mints once for two calls; two instances mint twice). No network: the mint is answered by a local `request` override, and the JWT signing is real. Also retired worker-context.ts's "no such credential exists yet" note, which this workstream has now made false.
**Medium — the JWT redaction rule was quadratic.** `\b` before `eyJ` with `-` inside the run class `[\w-]`: `-` is not a word character, so every `-eyJ` inside one long run started a fresh match attempt that rescanned the rest of the run for a `.` that never comes. Measured on `'-eyJ'.repeat(n)`: 190ms at 20KB, 831ms at 40KB, 3.1s at 80KB, 12.7s at 160KB. A `(?<![\w-])` lookbehind is linear -- the same 160KB input is under a millisecond -- and is now pinned by a timing test with a deliberately loose 1s bound (two orders of magnitude separate the spellings, so a slow CI box cannot flake it). This is exactly the CodeQL js/polynomial-redos shape the file's other rules were written to avoid, and the comment claiming linearity was wrong. **The PEM footer match ate the rest of the line.** `-----END[^\n]*` is greedy, so a single-line message lost everything after the key. Lazy to the closing dashes instead; the end-of-string fallback for a truncated key is unchanged. **A transient GitHub failure at boot was fatal under App auth, and not under token auth.** With a warm remote.git, a 502 during boot is absorbed on the token path (ensureRemoteGit short-circuits, Promise.allSettled swallows the initial syncGit) and the worker starts and retries. The preflight rethrew every error class, so the App path would exit and systemd would crash-loop the instance until GitHub recovered -- telling the operator to check their private key each time round. It now fails startup only for a permanent failure, using isPermanentTaskFailure, the same 4xx/5xx classifier the task runner uses, so the two agree about what "the credential is wrong" means. **The preflight-ordering test did not pin the ordering.** beforeEach seeds remote.git, so ensureRemoteGit short-circuits and the misleading catch was never reachable: moving the preflight below it left both tests green. Added a cold-workspace test where the clone really would speak first. Measured: with the call moved, the new test goes red and the old ones stay green. **gitTokenMintTimeoutMs was unvalidated.** AbortSignal.timeout throws a RangeError for NaN/0/negative/Infinity, and it would throw INSIDE the mint, where the rejection carries no `.status` -- so a config typo (and `parseInt(process.env.X ?? '')` is NaN) would read as transient and burn every push task's full retry budget. Validated at construction instead. **createCanopyOctokit spread its whole argument.** Excess-property checks stop an inline literal, but a widened variable type-checks and would hand Octokit a live `baseUrl`/`request`/`log`/`userAgent`; only `throttle` was protected, by ordering. The two auth fields are picked out explicitly now. Round 1 was code-only by the review-rounds protocol; its deferred prose list is carried to the claims pass and is not addressed here. Every fix above is break-and-rerun'd: seven mutations, each turning exactly the intended test(s) red.
**HIGH — round 1's preflight fix let a genuinely bad key boot silently.** `isPermanentTaskFailure` classifies only on an HTTP `.status` and returns false when there is none, which is right on the task path (bounded by maxRetries) and wrong for a boot check (bounded by nothing). Measured against the real @octokit/auth-app@6: a private key that parses but is not this app's key throws from jsonwebtoken with NO status -- so round 1's `!isPermanent` guard warned "this looks transient" and started the worker. It then wrote no lastFatalError, showed healthy in the admin panel, and failed every task and every sync. Strictly worse than the crash-loop round 1 was avoiding, because a crash-loop is at least visible. Replaced with `isTransientAuthFailure` in github-auth.ts, which fails CLOSED: transient only for 5xx/408/429, a Node network errno, or our own MintTimeoutError (now a named class so it can be recognised). A plain 403 is permanent here, unlike the task classifier's rate-limit carve-out -- at boot a 403 from the installation-token endpoint is a suspended or uninstalled app far more often than a rate limit, and exiting is recoverable by systemd where booting on a dead credential is not. The divergence from isPermanentTaskFailure is asserted in a test, not just described. **MEDIUM — round 1's timeout validation did not match what `AbortSignal.timeout` accepts.** Node wants an integer in [0, 2**32-1]. Measured: `0.5` and `2**32` throw RangeError (inside the mint, status-less -- the exact failure the validation was added to prevent), and 2**31 .. 2**32-1 are SILENTLY clamped to 1ms, so every mint would abort instantly while reporting the configured timeout. Now `Number.isInteger` and a 2**31-1 ceiling, checked whether or not an App is configured. **MEDIUM — the credential branch was resolved in the constructor**, which is outside every worker-status.json write path. This is the #198 shape the codebase has already paid for once, and ensureSettingsBranch()'s comment records it: a constructor throw lands before start()'s catch, so the AWS entrypoint (which constructs, then calls start(), with a main().catch() that only logs and exits) produces an invisible ~5s systemd crash-loop that `cdk deploy` reports as success while the admin panel shows the worker absent with no reason. Newly reachable here because `githubToken` became optional. Deferred into `ensureGitHubAuth()` in the ensureSettingsBranch() shape, called at the top of start()'s try. Octokit is built there too, and NOT overwritten if a test has already assigned one -- the worker-context INVARIANT is intact, and all 14 worker test files still pass. Reads go through `octokitClient()` rather than the field, because `rebaseActiveBranches()` is reachable without start() (apps/test-app's e2e route calls it directly) and its pollMergeState dispatch reads ctx.octokit(). **LOW — the lazy PEM footer could leak a following key.** `-----END[^\n]*? -----` stops on a label-less `-----END-----` and eats the five dashes that would have started the next header. The footer is now spelled out, mirroring the header, so anything else falls through to `$` and over-redacts instead. Both looser spellings now fail a test, one each. Round 2 was code-only; its deferred prose list goes to the claims pass. All six mutations verified red, and two test defects were found and fixed in the process: the first F4 test put an extra `-----` between the keys, which does not reproduce the leak (the second key is redacted on the next pass of the /g loop) and passed against the broken spelling.
Round 3 reported no finding at MEDIUM or above, so phase 1 of the review
protocol converges here. Two of its three LOW findings were real.
**The errno branch of isTransientAuthFailure read `code` at the wrong level.**
Measured on Node 24: a `fetch()` DNS failure is `TypeError: fetch failed`
whose own `.code` is undefined and whose `.cause.code` is ENOTFOUND. The
branch only looked at the top level, so it would have called a DNS blip
permanent and killed a booting worker over it. Now walks one level of `cause`.
Belt and braces for the stock strategy -- @octokit/request converts that
TypeError into a RequestError(…, 500), which the status branch already handles
-- but it is exactly what a mintInstallationToken built on raw fetch produces,
and an injection point invites that.
**`minting.catch(() => {})` was dead code, and its test pinned nothing.**
`Promise.race` subscribes a reject handler to every input, so the losing
mint's late rejection is already handled; measured both ways, with and without
the line, and no unhandled rejection appears either way. The line is gone and
the comment now says why there isn't one, since the same shape in
executeTaskWithTimeout does have one. The test is kept but relabelled as what
it is -- a property test that would catch a rewrite which stops racing, not a
mutation-pinned guard. (task-runner.ts's identical guard is pre-existing and
untouched.)
**The third finding does not hold.** Round 3 reported that the
`core-no-github-app-auth` dependency-cruiser rule "observes nothing" because
`lint:bundle` cruises only the two client entries. It is `lint:cycles` that
evaluates it, over all of packages/*/src -- run by CI (.github/workflows/ci.yml:173)
and by the pre-commit hook (lint-staged.config.mjs:12). Re-confirmed by
planting the import: lint:cycles goes red, naming the rule.
One batched pass verifying every assertion this branch makes -- code comments,
docs, commit messages, PR body -- against what the code actually does. Findings
in rough order of consequence.
**One of them is a code defect, and it breaks CI.** `Error.cause` is not in the
type surface under this repo's `target: ES2021` with no `lib` override, so the
round-3 `cause` walk made `pnpm typecheck` fail in both canopycms and
canopycms-cdk -- while the PR body claimed it was clean. My own check had piped
typecheck through `grep | head -3`, and the sandbox's certificate-warning lines
filled all three slots. Read through a structural cast instead, one file, no
repo-wide tsconfig change; verified by exit status this time, not by grep.
**The fail-closed rationale cited the wrong example, in three places.** It said
a private key "that parses but is not this app's key" throws status-lessly.
Measured against the real @octokit/auth-app@6.1.4: a valid RSA key belonging to
a DIFFERENT app signs fine locally and GitHub refuses it with a 401 -- which
has a status, and which both classifiers already call permanent. The
status-less cases are a key of the wrong TYPE (`"alg" parameter for "ec" key
type…`) and one too mangled to sign with (`secretOrPrivateKey must be an
asymmetric key…`). The argument survives; the example did not, and a
maintainer testing the quoted case against real GitHub would have concluded the
status-less branch was unreachable and deleted it.
**"Configuring neither is the only error" was wrong in four places** (this
doc, worker/AGENTS.md, the PR body, and an earlier commit). The code throws for
BOTH set as well as for neither, and has a test for each. An adopter reading it
would have left `githubToken` in place while adding `githubAppAuth`, expecting
the App to win, and got a worker that refuses to start.
**Stale after the round-2 refactor:** "called from CmsWorker's constructor",
"Octokit, wired once in the constructor", and "raised in the constructor rather
than at the first push" -- all now `ensureGitHubAuth()`, called from start().
Worse, `buildGitHubUrl`'s doc block had been left orphaned above
`ensureGitHubAuth`'s own block, so the seam rules documented the wrong method
and `buildGitHubUrl` had none on hover. Reattached.
**Five in-code copies said `canopycms-cdk/worker/index.ts` constructs the App
strategy.** That file is unchanged on this branch -- the wiring is the next PR.
All now say so, and name `canopycms-cdk/package.json` as what actually declares
the dependency.
**"lint:cycles evaluates the rule over all of packages/*/src"** -- it cruises
`packages/canopycms/src` and `packages/canopycms-next/src` only. The rule's
pattern spans every package but nothing else is ever cruised. Corrected in all
four copies, and the note now says explicitly that `lint:bundle` is NOT what
runs it.
**Two test-file counts, both wrong.** "six test files assign a mock over
octokit" -- there are two, and worker-context.ts names exactly those two, so
the new comment contradicted the file it cited. And worker-context.ts's own
"eight test files" has been wrong since before this branch; there are 14.
**Two claims about mechanisms that do not exist.** GitHub does not echo the app
JWT back in its 401 body (measured: `{"message":"Bad credentials"}`, and
@octokit/request-error redacts the authorization header anyway), and no site
today puts key material into an error text. Both rules are still worth having
as defense-in-depth, which is now what they say.
**Regex timings reproduce ~25% high on another machine**, so the three copies
quoting exact milliseconds now give ranges and lean on the quadrupling-per-
doubling, which is the part that is not machine-dependent.
**Smaller:** `AbortSignal.timeout(0)` does not throw (it fires immediately) --
an earlier commit said it did, and a live test title said so too; we reject 0
anyway, for a reason the comment now states. `universal-github-app-jwt@2`
selects its crypto backend through the package's `imports` map, not an
`exports` condition. The committed PEM fixture had a realistic-looking base64
body; replaced with obviously-synthetic text so no secret scanner has to decide.
**Docs.** CODEBASE_GUIDE.md had no `github-auth.ts` entry and still described
cms-worker.ts as 802 lines (it is ~990); both fixed. worker/AGENTS.md's four
new paragraphs restated rules that already live verbatim in code comments, so
three are gone and the file now carries only the one genuinely cross-cutting
invariant -- the dependency ban, which spans three files and a lint rule. Net
line deltas for this branch: worker/AGENTS.md +22, CODEBASE_GUIDE.md +1,
docs/adopter-migration.md +80 (a new Unreleased entry, which is what that file
is for). Nothing else grew.
Also recorded: with the documented `authStrategy: () => appAuth` closure,
Octokit mints through ITS request, not one configured on the createAppAuth
instance -- so a GitHub Enterprise `baseUrl` must be set on both halves. The
shared token cache is unaffected. Verified by running the documented snippet
against a real createAppAuth and a real Octokit.
jpslav
force-pushed
the
feat/worker-github-app-auth
branch
from
September 13, 2026 01:49
2ca9b1a to
f7ce1fa
Compare
jpslav
marked this pull request as ready for review
September 13, 2026 01:50
Both surfaced during PR #321's review rounds and were deliberately not fixed there. - `Error.cause` is untyped repo-wide (`target: ES2021`, no `lib` override), so the errno walk in worker/github-auth.ts reads it through a cast and its two tests build errors with `Object.assign`. The honest fix is a base-tsconfig `lib` bump verified across all eight typecheck projects, which is not a feature PR's business. - `executeTaskWithTimeout`'s `work.catch(() => {})` is dead — `Promise.race` already handles every input's rejection (measured) — and its comment claims otherwise. Pre-existing, and it is why the same pattern got copied into github-auth.ts on this branch before being removed again. Neither changes behaviour; both are comments or types that will be believed.
This was referenced Sep 13, 2026
jpslav
added a commit
that referenced
this pull request
Sep 13, 2026
…uth task The remaining work on the worker-credential epic moves to a different session, so this writes down what the diff cannot show. epic-202609-credential-sources-handover.md pins the scope as one clean range (a3fa411..int-202609-a; every merge since then belongs to this epic) and fixes the order of what is left: finish the rotation-window P2 FIRST, then run review-rounds and claim-check over the range including it. Landing that P2 afterwards would leave it outside the only combined pass. The part worth having is the map of where to look hardest. FOUR premises the orchestrating session asserted turned out false; three of them had already travelled into chip prompts, code comments, PR bodies and docs before a session that MEASURED caught them, and one would have shipped an inert feature. So confident mechanism claims in this range earn less trust than usual. It also records the two defect classes the epic produced (assertions that cannot fail; CI=1 log-spy mismatches, three instances), the measured sandbox baseline, and three hazards that bit. The fourth premise is this commit's own history and is recorded as such: the orchestrator fetched, read `git log origin/int-202609-a`, saw PR #333 in the list, and then branched WITHOUT updating the working tree. A find over that stale tree reported #333's follow-up task file as missing, and "it did not survive the merge" reached a PR body before anyone checked. The file was there all along. Reading a ref is not checking out a ref. So this no longer adds a duplicate of that file. Instead it adds one step the existing one was missing: leave the worker running past the installation token's ~1-hour expiry and publish again. That is the step nothing else approximates -- it is the reason request #45 exists, it exercises #321, #329 and #334 together, and its failure mode is silence an hour in rather than an error at setup.
jpslav
added a commit
that referenced
this pull request
Sep 13, 2026
The task filed by PR #333 covers the setup failures well -- a rejected JWT, an insufficient permission set, a GraphQL denial that answers HTTP 200 and so reads as transient -- and its narrowing step is what proves `verify` earns its place. What it did not cover is the boundary the feature exists for. An installation token lasts about an hour. Before this epic the worker read its credential once in main(), so an App token would have worked for an hour and then failed until the ASG replaced the instance; PR #334 added the refresh meant to prevent that, driven from the git-sync loop rather than from a task failure. So a worker left running past the hour and then asked to publish is the only step that exercises #321, #329 and #334 together -- and the only one whose failure mode is silence an hour in rather than an error at setup. Also notes it is worth running with a JSON-field private key, which composes #46 with #45 and is the shape an organisation keeping one credential document per environment actually uses.
This was referenced Sep 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR B2 of the adopter-requests waterfall. Base is
int-202609-a; PR B1 (#319) is merged.Closes nothing on its own — the CDK props and entrypoint wiring are PR B3.
What this adds
The worker can authenticate to GitHub as a GitHub App installation instead of with a
personal access token. A fine-grained PAT expires within a year, acts as the person who
created it, and dies when they leave the organisation; an App's private key does neither and
installs across repositories. An App issues ~1-hour installation tokens, which is why
buildGitHubUrl()becamePromise<string>in #319 — this PR is what that change was for.The token stays first class
CanopyCMS is a public library and registering a GitHub App needs organisation-admin rights
that a single-maintainer site does not have, so App auth is purely additive:
githubTokenis unchanged and undeprecated. No warning, no migration step, no nudge.@octokit/auth-appabsent from the install entirely — corenever imports it, so there is nothing to be missing.
neither. Raised at the top of
start()— deliberately not in the constructor, which isoutside every
worker-status.jsonwrite path (see the review section below).Where the dependency lives, and why it matters
@octokit/auth-app@^6(the major matching the installed@octokit/core@5/@octokit/rest@20)is a devDependency of
packages/canopycms-cdk, not ofcanopycms.github-service.tsisreachable from
services.ts:39and is therefore in every adopter's Next.js server bundle —including everyone on a PAT, which is most of them.
pnpm lint:bundleguards the clientboundary only and would not have caught a server-bundle regression.
So core holds only the shape:
createCanopyOctokitnow takes{ auth: string } | { authStrategy, auth }, structurally typed, and the cloud entrypointconstructs the strategy and injects it — the same seam
refreshAuthCachealready uses(
AuthCacheRefresher,cms-worker.ts, supplied bycanopycms-cdk/worker/index.ts).Two guards hold this, and both were verified to fire:
core-no-github-app-auth, evaluated by the existingpnpm lint:cyclesover all ofpackages/*/src— addingimport { createAppAuth } from '@octokit/auth-app'togithub-service.tsturns it red;packages/canopycms/package.json— declaring the dependency thereturns it red.
Devdependency rather than a dependency matches
@aws-sdk/client-secrets-manager, the siblingthe worker entrypoint already uses:
canopycms-cdkdeclares no runtimedependenciesat all,and
prepackbundlesworker/with esbuild.Four decisions worth reviewing individually
PEM normalization — and a correction to the premise this PR started from. The rationale
I was given was that an unconverted PKCS#1 key fails under the worker's bundle. It does not.
I built with the worker's exact flags (
esbuild --bundle --platform=node --target=node22 --format=esm) and ran it: esbuild resolvesmain→universal-github-app-jwt@1.2.0'sdist-nodebuild, which signs throughjsonwebtoken, and PKCS#1 works — zero WebCryptoreferences in the bundle. The hazard is real but conditional, and the comment now names the
conditions rather than asserting a failure we do not have:
auth-app@6→universal-github-app-jwt@1viamain(our bundle)module/browser(Vite, webpack, esbuild--platform=browser)auth-app@7→universal-github-app-jwt@2under thenodeexport conditioncrypto-native.js's converter is a no-op)So the PKCS#1 → PKCS#8 conversion is insurance against a bundler flag or a dependency bump,
not a fix. What
normalizeGitHubAppPrivateKeybuys today is unwrapping the manglings amulti-line secret picks up in a single-line config field, and failing loudly and locally on an
unusable key rather than as an opaque JWT error later.
And that unwrapping had a real bug, found by putting the normalizer in front of the actual
signer: escapes were undone only before the base64 unwrap, so a PEM escaped and then
wrapped decoded to a body still full of literal
\n. The two manglings compose in eitherorder and each hides the other; the unescape now runs on both sides. The first test I wrote for
the trailing-newline half of this was also not deterministic — whether a trailing newline
appeared depended on whether the generated key's base64 length was a multiple of 64, and it
passed against an implementation missing the between-passes trim. It is explicit now, and the
mutation goes red.
Preflight in
start()'s try, beforeensureRemoteGit(). Otherwise the first failure comesout of the bare clone, whose catch says "the GitHub repository may be empty, or the base
branch may not exist" — sending an operator holding a bad private key after a repository
problem that does not exist.
start()'s catch records it aslastFatalErrorinworker-status.json, where the admin panel reads it.Mint failures are not wrapped — verified, not assumed.
isPermanentTaskFailure(
task-runner.ts) already does the right thing on an OctokitRequestError: 4xx permanent(401 bad key, 403 suspended, 404 revoked installation all fail fast), 5xx and status-less
network errors transient, 408/429/rate-limit-403 carved out. No new error type was added
and no wrapper was introduced; the real risk was a
catchrethrowingnew Error(getErrorMessage(err)), which would silently make every mint failure transient.Tests pin 401 → permanent, 503 → transient, and that the thrown error reaches the classifier
by identity.
The mint timeout lives inside
resolveGitToken, not threaded throughpushBranchToGitHub/TaskRunnerContext/buildGitHubUrl.executeTaskWithTimeout's race doesnot bound
git-sync.ts's two resolutions at all — they run on the sync loop, not through thetask runner — so a threaded task signal would have covered one call path of two. A local
AbortSignal.timeoutcovers both; it is also handed to the injected provider so a strategythat can forward it to its HTTP client gets genuine cancellation (
@octokit/auth-app@6accepts no per-call signal, so with the stock strategy the local race is what bounds the wait —
stated here rather than claimed in a comment).
No tokenized URL is cached
pushBranchToGitHubstill resolves the URL exactly once per call — #319's invariant, stillpinned by its own test. The new tests therefore vary the minted token across separate calls,
which is where a cache would actually show; asserting it within one call would contradict B1.
Tests
New:
canopycms/src/worker/github-auth.test.ts(58),canopycms/src/worker/cms-worker-github-app-auth.test.ts(17), andcanopycms-cdk/worker/github-app-auth.test.ts(4), plus additions toutils/error.test.tsandgithub-service.test.ts. RSA keys are generated withnode:cryptoat test time — nokey-shaped fixture is committed.
The canopycms-cdk file exists because
canopycmscannot host it: it must never depend on@octokit/auth-app. It is therefore the only place the normalizer can be checked against thesigner it exists for, and the only place the shared-instance token cache can be demonstrated
(one instance mints once across two calls; two instances mint twice — which is why
GitHubAppAuth's two members must derive from onecreateAppAuth). No network: the mint isanswered by a local
requestoverride, and the JWT signing is real.Every new test was break-and-rerun'd: 38 separate mutations were applied to the
implementation and each turned exactly the intended test(s) red (PEM rule, JWT rule, both
exactly-one-of throws, the empty-token narrowing, a wrapped mint error, the timeout race, the
empty-minted-token guard, the PKCS#8 conversion, the
\nunescape, the base64 unwrap, amemoized token,
buildGitHubUrlreading config directly, the dropped preflight call, thedropped auth-strategy spread, both halves of the double unescape, the between-passes trim,
and the two dependency guards above). Three of the 38 came back green, and each led to a
fix rather than a shrug: a PEM test whose input did not actually reproduce the leak it named;
a non-deterministic one whose trailing newline depended on the generated key's length, and
which therefore passed against an implementation missing the trim it claimed to pin; and a
.trim()that turned out to be dead, removed rather than left unexercised.Verification
pnpm --filter canopycms test— 4424 passed. The 9 failures are the known sandbox baseline(
cli/init.integration.test.ts×7, tsx IPCEPERM;dev-content-watcher.test.ts×2, watchertiming), both unrelated and both pre-existing on a clean tree.
pnpm --filter canopycms-cdk test— 431 passed.pnpm typecheck,pnpm lint,pnpm lint:bundle,pnpm lint:cycles,pnpm lint:tasks,pnpm lint:docs— clean, checked by exit status. (An earlier revision of this body claimedtypecheck was clean when it was not: I had piped it through
grep | head -3and thesandbox's certificate warnings filled all three slots.
Error.causeneeds the ES2022 liband this repo targets ES2021; read through a structural cast instead.)
@octokit/auth-appis not inpackages/canopycms/package.json.Review
Rebased onto
int-202609-aafter #322 landed; one conflict indocs/adopter-migration.md,resolved by keeping both Unreleased entries.
/review-roundsran three code-only rounds, then one batched claims pass, then a mechanicalre-check. What each found, and where the defect came from — that provenance is what decided
when to stop:
Round 2 still found a MEDIUM in the original work, which is why round 3 ran; round 3 found
nothing at MEDIUM or above, so phase 1 stopped there.
The three that mattered:
\bbeforeeyJwith-inside the run class:
-is not a word character, so every-eyJin one long run started afresh match that rescanned the rest looking for a
.that never comes. Measured~200ms/900ms/13–16s at 20/40/160KB. A
(?<![\w-])lookbehind is linear — under amillisecond at 160KB — and is pinned by a timing test.
non-fatal for anything
!isPermanentTaskFailure. But that classifier defaults a status-lesserror to transient, which is right on the task path (bounded by
maxRetries) and wrong atboot (bounded by nothing). Verified by measurement against the real
@octokit/auth-app@6.1.4:a key of the wrong TYPE throws
"alg" parameter for "ec" key type…with no status at all, soa worker with a dead credential booted, wrote no
lastFatalError, and reported healthy whilefailing every task and sync — strictly worse than the crash-loop round 1 was avoiding.
Replaced with a fail-closed
isTransientAuthFailure.worker-status.jsonwrite path: the fix(worker): publish rewritten history, guard the sync cycle, resolve deploymentName #198 shape this repo has already paid for once, newlyreachable because
githubTokenbecame optional. Deferred intoensureGitHubAuth(), in theensureSettingsBranch()shape.One round-3 finding did not hold. It reported the
core-no-github-app-authrule asobserving nothing because
lint:bundlecruises only the client entries. It islint:cyclesthat evaluates it — CI (
.github/workflows/ci.yml:173) and the pre-commit hook. Re-confirmedby planting the import.
/claim-checkthen verified every assertion across all four surfaces. It caught aCI-breaking code defect (the typecheck failure above) plus 17 prose findings, the sharpest
being that the fail-closed rationale cited an example that is measurably wrong (a valid RSA key
belonging to a different app signs fine locally; GitHub refuses it with a 401, which has a
status) and that "configuring neither is the only error" was false in four places — the code
also rejects configuring both. Two test defects surfaced the same way: a PEM test whose input
did not reproduce the leak it named, and a non-deterministic one that passed against a broken
implementation because its trailing newline depended on the generated key's length.
The full list is in the
docs: the claims pass over the whole diffcommit.