The OIDC login bypasses the hook's injected fetch — the seam exists but is not in the path #57

New issue

Open

opened 2026-08-17 22:46:34 +00:00 by forgejo-admin · 0 comments

forgejo-admin commented

2026-08-17 22:46:34 +00:00

Owner

Two findings. The second is the one that matters.

1. `authentik-token.ts` has #54's `preconnect` defect

scripts/smoke/authentik-token.ts:42:

/** Optional fetch override, default global fetch. */
fetchImpl?: typeof fetch;

Identical to the ApiSmokeOptions.fetchImpl defect #54 fixes in api-smoke.ts: typeof fetch carries preconnect, and the injectable fetchers this seam exists for do not have it. celilo's own type says so:

Fetcher — the HTTP seam used by capability functions and hooks. Narrower than typeof fetch so mocks don't need to supply Bun-only side methods like preconnect.

This one was not among #52's 18 errors purely because nothing currently passes a Fetcher to it — which is finding 2.

2. `mintSmokeToken` never passes the injected fetch down

celilo/scripts/health-check.ts. The hook receives fetch: Fetcher from celilo and threads it into the reachability check, the page-render checks, and the API smoke check. It does not thread it into the token mint:

const token = await mintToken({
  authentikUrl,
  clientId: OIDC_CLIENT_ID,
  redirectUri: `https://www.${domain}/auth/callback`,
  username: 'smoketest_bot',
  password,
  // no fetchImpl
});

mintAccessToken then falls back to opts.fetchImpl ?? fetch — the global. So the entire OIDC authorization-code + PKCE login (roughly six requests: the auth redirect, the flow-executor hops, the token endpoint) runs outside the injected fetcher.

This is not a style point. The seam exists specifically so HTTP done by a hook can be observed and controlled, and the one flow that authenticates against production is the flow that is not in it. Concretely:

Untestable without mocking the module boundary. #53 had to inject at mintToken instead, which is the right seam for testing the gating but leaves the real OIDC request sequence — the part that breaks when a redirect URI or a flow slug changes — with no way to be driven by createMockFetcher.
Unobservable in production. Whatever celilo's fetcher does (logging, timeouts, instrumentation), the login path silently opts out of it.
Inconsistent within one function. Four HTTP call sites in lunacycleHealthCheck, three on the injected fetcher and one not, with nothing marking the difference.

This is the same shape as the drift #52 found: a seam that is declared and not wired, which no gate can see because nothing checks that a dependency is actually reaching its consumer.

Fix, when #54 is settled

Retype MintAccessTokenOptions.fetchImpl to the call signature the function actually uses, exactly as #54 does for ApiSmokeOptions — the two should land the same way rather than diverge.
Thread the hook's fetch into mintSmokeToken and on into mintAccessToken.
Add a test that drives the real mintAccessToken through createMockFetcher and asserts the request sequence: the authorize redirect, the flow-executor POSTs, and the token exchange. That is the part with no coverage today — #53 covers the gating around it, not the flow itself.

Ordering matters: step 1 before step 2, or passing a Fetcher in is a type error under #54's new gate.

Found while doing #53. Written up rather than fixed: it is the same defect class as #54, and opening a third PR on this file family while #54 is still unmerged would undo the conflict-free position #53/#54 currently hold. Not to be started until #54 is decided. Two findings. The second is the one that matters. ## 1. `authentik-token.ts` has #54's `preconnect` defect `scripts/smoke/authentik-token.ts:42`: ```ts /** Optional fetch override, default global fetch. */ fetchImpl?: typeof fetch; ``` Identical to the `ApiSmokeOptions.fetchImpl` defect #54 fixes in `api-smoke.ts`: `typeof fetch` carries `preconnect`, and the injectable fetchers this seam exists for do not have it. celilo's own type says so: > `Fetcher` — the HTTP seam used by capability functions and hooks. **Narrower than `typeof fetch` so mocks don't need to supply Bun-only side methods like `preconnect`.** This one was not among #52's 18 errors purely because nothing currently passes a `Fetcher` to it — which is finding 2. ## 2. `mintSmokeToken` never passes the injected fetch down `celilo/scripts/health-check.ts`. The hook receives `fetch: Fetcher` from celilo and threads it into the reachability check, the page-render checks, and the API smoke check. It does **not** thread it into the token mint: ```ts const token = await mintToken({ authentikUrl, clientId: OIDC_CLIENT_ID, redirectUri: `https://www.${domain}/auth/callback`, username: 'smoketest_bot', password, // no fetchImpl }); ``` `mintAccessToken` then falls back to `opts.fetchImpl ?? fetch` — the **global**. So the entire OIDC authorization-code + PKCE login (roughly six requests: the auth redirect, the flow-executor hops, the token endpoint) runs outside the injected fetcher. This is not a style point. The seam exists specifically so HTTP done by a hook can be observed and controlled, and the one flow that authenticates against production is the flow that is not in it. Concretely: - **Untestable without mocking the module boundary.** #53 had to inject at `mintToken` instead, which is the right seam for testing the *gating* but leaves the real OIDC request sequence — the part that breaks when a redirect URI or a flow slug changes — with no way to be driven by `createMockFetcher`. - **Unobservable in production.** Whatever celilo's fetcher does (logging, timeouts, instrumentation), the login path silently opts out of it. - **Inconsistent within one function.** Four HTTP call sites in `lunacycleHealthCheck`, three on the injected fetcher and one not, with nothing marking the difference. This is the same shape as the drift #52 found: a seam that is *declared* and not *wired*, which no gate can see because nothing checks that a dependency is actually reaching its consumer. ## Fix, when #54 is settled 1. Retype `MintAccessTokenOptions.fetchImpl` to the call signature the function actually uses, exactly as #54 does for `ApiSmokeOptions` — the two should land the same way rather than diverge. 2. Thread the hook's `fetch` into `mintSmokeToken` and on into `mintAccessToken`. 3. Add a test that drives the real `mintAccessToken` through `createMockFetcher` and asserts the request sequence: the authorize redirect, the flow-executor POSTs, and the token exchange. That is the part with no coverage today — #53 covers the gating around it, not the flow itself. Ordering matters: step 1 before step 2, or passing a `Fetcher` in is a type error under #54's new gate.

forgejo-admin added the

labels

2026-08-17 22:46:34 +00:00

forgejo-admin referenced this issue

2026-08-17 23:04:25 +00:00

chromium.launch() is unguarded — a browser-cache miss crashes health_check instead of recording a check #56