[pull] main from oomol-lab:main - #12
Merged
Merged
Conversation
## Summary - Accept numeric strings (e.g. `"3600"`) and zero for OAuth `expires_in`. - Previously only `typeof === "number"` was accepted, and `0` was treated as missing via truthiness, so many tokens never got `expiresAt` and were never proactively refreshed. ## Test plan - [x] `npm test -- src/oauth/oauth-token.test.ts` --------- Co-authored-by: Kevin Cui <bh@bugs.cc>
## Summary - disable Hono's application-level `/api/*` compression for the Cloudflare Workers entrypoint - keep API compression enabled by default for Node.js and other deployments - add a regression test proving `/api/auth/session` remains readable JSON when the client advertises gzip support ## Root cause PR #173 added `compress()` to all `/api/*` responses. On a production Cloudflare Workers deployment of v1.3.1, `/api/auth/session` returns HTTP 200 but the browser receives a gzip payload (`1f 8b`) that it cannot decode as JSON. `web/src/api.ts` consequently resolves the failed JSON parse to `null`, and the dashboard crashes while reading `authSession.authenticated`. Cloudflare already negotiates and applies response compression at the edge. Avoiding the extra `CompressionStream` layer in the Workers entrypoint prevents the encoded-body/header mismatch while preserving the catalog-size improvements from #173. Other runtimes retain the existing Hono compression behavior. Cloudflare documentation: https://developers.cloudflare.com/workers/runtime-apis/fetch/#how-the-accept-encoding-header-is-handled ## Validation - `vitest run src/server/cloudflare.test.ts` - `npm run fix-check` ## Release request Thanks for the excellent dashboard performance work in #173; the smaller catalog and lazy loading are substantial improvements. @l1shen, could you please publish a patch release containing this fix after it is merged? The current v1.3.1 release leaves Cloudflare-hosted dashboards unable to authenticate or load their API data, so a repaired release would help existing release-based deployments recover without carrying a local patch. --------- Co-authored-by: Kevin Cui <bh@bugs.cc>
Closes #189. ## What the issue claimed, and what I found Confirmed. `src/server/api/auth.ts` matched the configured deployment secrets with a plain `===` against the whole header: - `hasRequestToken` (line 169) — `authorization === \`Bearer ${token}\`` - `installAdminCookieForBearer` (line 140) — same shape The inconsistency the issue points at is real too: cookie signatures in the same file already go through `constantTimeEqual`, and stored runtime tokens go through `timingSafeEqual` on their hashes (`runtime-token-service.ts`). The env-token path was the only inbound bearer comparison without that treatment. I swept `src/server/`, `src/oauth/` and `src/mcp.ts` for other secret comparisons — these two were the only ones. (The thousands of `Bearer ${...}` hits across `src/providers/` are all outbound request headers.) On severity I agree with the issue's own framing: over remote HTTP the leaked timing is nanosecond-scale and drowned by network jitter, so this is hardening rather than an exploitable bug. It is still worth doing — `OOMOL_CONNECT_ADMIN_TOKEN` is a long-lived, highest-privilege secret and the fix costs nothing. ## The change Both call sites now go through `matchesConfiguredToken`, reusing the existing `constantTimeEqual` helper. No `node:crypto` import — `auth.ts` deliberately sticks to WebCrypto so it keeps working on workerd. `readBearerCredential` returns the credential **verbatim** after the `Bearer ` scheme (no trim), so matching stays byte-for-byte exact and the semantics are unchanged from `=== \`Bearer ${token}\``. The existing `readBearerToken` is now `normalizeToken(readBearerCredential(...))`, preserving its trimming behaviour for the runtime-token lookup path. A length mismatch still returns early, which leaks the token length. That matches what the issue suggested ("reject on length mismatch") and matches the constraint Node's own `timingSafeEqual` imposes. ## Tests Added `matches configured tokens byte-for-byte after the bearer scheme`, covering an equal-length near miss (so a length check alone cannot pass it), truncation, extra suffix, extra internal whitespace, a missing scheme, and that the admin and runtime tokens do not unlock each other's surface. Full suite: **56 files / 524 tests passing**. `tsc -p src/tsconfig.json` clean, `oxlint` and `oxfmt --check` clean.
…196) ## Summary - `RuntimeTokenService.resolveToken` awaited `store.markUsed` before returning the grant, so a failed `last_used_at` write rejected an otherwise valid `oct_...` runtime token. - `last_used_at` is best-effort audit metadata. It is now recorded through a private `recordLastUsed` helper that logs write failures and still returns the grant. Closes #187. ## Problem Nothing between `resolveToken` and the auth middleware catches, so a `markUsed` rejection propagates from `resolveToken` (`runtime-token-service.ts`) through `hasValidRuntimeToken` / `hasValidToken` (`api/auth.ts`) into the global `app.onError` handler. Verified with a throwaway test that drives the real middleware plus `ConnectServer`'s error handler against a store whose `markUsed` throws `D1_ERROR: network connection lost`: ``` STATUS: 500 BODY: {"error":{"code":"internal_error","message":"Internal server error."}} ``` Two corrections to the issue report: - The observed failure is **500 `internal_error`**, not 401. The availability impact is the same, but the symptom to grep for in logs is different. - The throw also skips the `verifyRuntimeJwt` fallback further down `hasValidRuntimeToken`. ## Changes - `RuntimeTokenService` takes an optional `RuntimeLogger` (the existing storage-layer logging abstraction — compatible with both pino and the Workers `console` shim); `createConnectApp` passes the app logger through. - `resolveToken` calls `recordLastUsed`, which swallows and logs write failures as `runtime token last use update failed`. - Regression test: a store whose `markUsed` rejects still yields the grant, and the failure is logged. ## Not included: `waitUntil` The issue also suggests `waitUntil(markUsed(...))` on Workers so auth latency is not tied to the write. Left out deliberately: - It is a latency optimization, not the reported defect — the issue's *Expected* behaviour is fully satisfied by this change. - There is no `ExecutionContext` available today: `src/server/cloudflare.ts` discards `_ctx` and calls `app.fetch(request, env)` without it. Wiring it up means touching the Workers entry point *and* adding a per-request defer hook to `LocalAuthOptions.resolveRuntimeToken`, leaking a Workers concept into the shared auth layer and making `last_used_at` eventually consistent on Workers. Happy to do it as a separate PR. ## Test plan - [x] `npx vitest run` — 56 files / 524 tests pass - [x] `npx oxlint .` and `oxfmt --check .` clean - [x] `tsc -p src/tsconfig.json` clean
## Summary Adds a **Response Compression** section to `docs/cloudflare.md` covering two things an operator otherwise has to reverse-engineer: 1. why the Worker does not compress in the application (Cloudflare compresses on egress, and `encodeBody` defaults to `"automatic"`, so an application-compressed body gets encoded twice), while Node deployments still do 2. that `/api/actions/:actionId/agent.md` is served uncompressed, because Cloudflare's default compressible content types list `text/x-markdown` but not the `text/markdown` this endpoint returns ## Why not just change the content type `text/markdown` is the registered type from [RFC 7763](https://www.rfc-editor.org/rfc/rfc7763.html); `text/x-markdown` is the pre-registration legacy spelling. Switching the response to the legacy type purely to match one CDN's list would make the API less correct for every deployment. The payoff would also be small. Measured across all 11,181 actions in the generated catalog: | | raw | gzip | |---|---|---| | median agent guide | 1.5 KiB | — | | mean | 1.8 KiB | — | | largest (`alpha_vantage.get_technical_indicator`) | 19.9 KiB | 1.7 KiB | So the doc says what the behavior is and points at the Compression Rule that changes it, and leaves the wire format alone. ## Depends on #193 The first paragraph describes the Worker leaving compression to the edge, which is what #193 implements. **Merge after #193.** Until then `main` still runs Hono's `compress()` on Workers, which is the bug #193 fixes. ## Validation - `oxfmt --check .` clean - docs-only change; no code, no tests affected
## Summary
- `readJson` no longer collapses a failed JSON parse into `null` typed
as the payload
- a 2xx response whose body has bytes that are not JSON now raises
`ApiError` with the status
- the tolerant paths that were intentional stay: non-JSON error bodies
still fall back to the status message, and an empty 2xx body still
resolves to `null`
## Why
```ts
const payload = (await response.json().catch(() => null)) as unknown;
if (!response.ok) { throw new ApiError(...); }
return payload as T; // null, typed as T
```
The `catch(() => null)` exists so that an error response carrying HTML
or an empty body still produces a usable `ApiError`. But it applies to
successful responses too, so any transport-level corruption under a 200
is handed back as `null` wearing the payload's type. Nothing fails at
the boundary; the app crashes later, wherever a caller first reads a
property.
That is not hypothetical. #193 fixes a Cloudflare deployment where the
Workers runtime re-encoded an already-gzipped body, so
`/api/auth/session` returned 200 with `1f 8b` bytes. The symptom
operators actually saw was a `TypeError` reading `authenticated` —
several frames away from the cause, with a green network tab.
This is independent of #193 and does not overlap with it: that PR stops
producing the bad body, this one stops the client from disguising a bad
body as a valid payload.
## Notes
- The body is read once via `response.text()` and parsed locally, so the
2xx and non-2xx paths can differ. `parseJson` returns `undefined` for a
non-JSON body, which `JSON.parse` can never produce for a valid one.
- No `/api/*` endpoint returns an empty 2xx body today (every `DELETE`
handler responds with JSON), but empty bodies keep resolving to `null`
so a future `204` is not a behavior change.
## Validation
- `npx vitest run` — 57 files / 528 tests pass
- new `web/src/api.test.ts` covers all five paths; the malformed-2xx
case fails against the previous implementation (`expected null to be an
instance of ApiError`)
- `npm run lint`, `oxfmt --check .`, `tsc -p src/tsconfig.json
--noEmit`, and `tsc -p web/tsconfig.json --noEmit` are clean
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )