Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

Five out-of-subject findings from PR #770's tier-3 review (credential-revoking tests, single-user boot gate, OAuth client rotation, shared-use atomicity, untyped server/tests)

Aperta
  • Qualcosa non va?
#786 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

@tuzengji ci sta già lavorando.

Dal 15/10/2026.

  • #795 di @tuzengji — aperta

Valutazione

Difficoltà
4/5
Tempo stimato
Più di una settimana
Idoneità per principianti
22/100
Tipo di issue
Bug
Chiarezza
Abbastanza chiara
Stato di attività
Attiva
Stack tecnologico
bun, typescript
Ambito
authentication, backend, build-system, security, testing

Direzione di ricerca

This bundles five independent defects, so split them before starting. Each has a file and line: server/tests/plugin-credential-rotation.integration.test.ts:96, server/src/config.ts:577 (reachOf), server/src/plugins/store.ts:2486-2641, server/src/plugins/shared-use-store.ts:250 and :637-663, and the server/tests type errors. Done means each fix has a test that fails before and passes after, and the typecheck covers server/tests. Ask a maintainer which item to take first, since several touch security-sensitive code.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Five load-bearing findings surfaced by the tier-3 code review of #770 (80 reviewers, ~450 raw
findings). All five are real and all five are out of that PR's subject — it is about adding
Arcade as a second broker and its per-action OAuth lifecycle, and none of these are. They are filed
here rather than absorbed into #770, which would have conflated two subjects in one diff.

Each is recorded with the evidence that established it, so none needs rediscovering.

1. Two integration tests permanently revoke real OAuth client credentials

server/tests/plugin-credential-rotation.integration.test.ts:96 and
server/tests/plugin-user-credential.integration.test.ts:188.

Both declare const serverId = "google-drive" — a fixed literal, not suite-scoped — then UPDATE credentials SET revoked_at = now() for every live mcp_oauth_client row matching it. Neither
restores revoked_at: the rotation suite's afterAll only nulls the credential_id pointer and
deletes rows the run created.

The comment sitting on the rotation suite's revocation reads:

This run's own client key, so a deployment's real Drive registration is never touched.

That is the opposite of what the code does. The TEST_DATABASE_URL guards keep the blast radius off
a dev database (they refuse the openbot database by name), so this is not a live incident — the
defect is that the source asserts a scoping it does not have, which is what makes it survive review.

Verified: both literals read directly; grep revokedAt over the rotation suite shows the set at
:99 and no restore anywhere.

2. reachOf classifies a scheme-less public address as private

server/src/config.ts:577.

new URL("example.com:3010") parses — protocol example.com:, hostname "" — so it never reaches
the "unparseable counts as public" guard its own docblock promises. The empty hostname then falls
through to the !bare.includes(".") branch and returns "private".

Consequence: OPENBOT_SINGLE_USER with a public OPENBOT_PUBLIC_URL / OPENBOT_APP_URL /
TRUSTED_ORIGINS written without a scheme warns instead of refusing boot, leaving no-sign-in
admin access on a routable address.

Verified (bun 1.4.2): new URL("example.com:3010").hostname === ""; "example.com" throws (so
it is correctly public); "http://example.com" resolves normally. Two reviewers found this
independently.

3. A transient fault rotates away the live OAuth client

server/src/plugins/store.ts:2625-2641 and :2486-2498.

storedOAuthClient / heldOAuthClient use a bare catch { return null }, so a DB or decrypt
failure is indistinguishable from "no client registered". ensureOAuthClient then re-registers —
rotating away the live client and invalidating every existing consent.

4. Shared-use approval writes are not atomic, and one 500s from a request body

server/src/plugins/shared-use-store.ts:250 and :637-663.

  • decide() flips request status in one statement and writes the approval in another. A failure
    between them leaves a permanently approved request with no approval, unretryable — the claim
    requires pending.
  • setApproval inserts approval members without de-duplicating, so a repeated (kind, value)
    violates the members primary key and 500s. Reachable from a request body (asSharedUseApproval
    never dedupes) and from exposureOf, which emits a group twice when a Bot is both published to
    and assigned to that group.

5. server/tests is outside tsc, and it is hiding real type errors — FIXED, and the figure below was wrong

Corrected 2026-10-14. The text below said four errors were proven. That was the size of the
sample taken at the time, not the size of the problem. When server/tests was actually brought into
a typed program it reported 277 errors — roughly seventy times the number quoted.

The gap itself is now closed. bun run typecheck:tests type-checks server/tests against a
frozen baseline (server/tests/typecheck-baseline.txt) and fails on any error not already recorded.
The baseline is 264 today: lower than when it was frozen, because some were repaired rather than
merely recorded. It has already earned its place, catching breaks that appeared only when
independently-correct branches were merged and that every individual author had missed.

Status of the instances named below: mcp-grant-removal.integration.test.ts:25 (TS2739) is
fixed; :35, :61, server-side-tools.integration.test.ts:47 and four plugin-routes.test.ts
entries remain open and are now gated. The remaining items are recorded as debt rather than hidden —
the baseline file is explicit that every line is a real defect, that deleting one by fixing it is
always correct, and that adding one is not.

The original text follows, unaltered.

Not a style point — four were proven by building a scratch tsconfig that pulls the files into the
typed program:

  • mcp-grant-removal.integration.test.ts:25 — the ConnectedAppBroker literal is missing
    connectionFields, connectWithFields, revokeAccount (TS2739).
  • :35 — ensureAuthConfig: async () => undefined violates Promise<AuthConfigOutcome> (TS2322),
    silently forcing store.ts:3488's non-"standing" branch so the config-reuse path is never
    exercised.
  • :61 — the credentials stub is not assignable to CredentialSecretReader & CredentialStore.
  • server-side-tools.integration.test.ts:47 — the credentials seam omits required
    create/updateSecret/revoke.

And plugin-routes.test.ts:50 documents a Partial<PluginStore> + fixed-tuple arrangement whose
stated purpose is "a compile error rather than a green test" — unenforced, because
server/tsconfig.json excludes the file. Its own serverRecord() omitting three ServerRecord
fields is the proof.

This is the same gap that hid a missing required option during #770's merge with main: nine of
main's tests died at runtime in toolkitOfFor because a plugin store was built without
brokerProvider, and typecheck stayed green throughout (fixed in 75614fb2).

Related

  • #785 — server/tests connection leak (55 files open a pool and never close it).
Lingua principale
TypeScript
Stelle
6.6k
Fork
914
Merge medio
19h 22m
PR unite (30g)
140

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Ha un modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di CopilotKit/OpenBot

Tutte le issue di CopilotKit/OpenBot

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.