Feat/connectors - #23
Merged
Merged
Conversation
…rigger identity
Adds a Connectors tab: Gmail, Calendar, Drive, Sheets, Outlook, Slack, Notion, Linear,
Atlassian, GitHub, HubSpot and Airtable. Click Connect, sign in on the vendor's page,
approve — the connector expands into ordinary Forge rows (an AuthProvider, a ToolSet, one
Tool per action), so its actions appear in Tools, on the canvas, and in agents with no
separate connector runtime for the rest of the app to know about.
Forge stays independent. The catalog is a directory of JSON manifests read at import time
with zero network I/O (test-enforced), so this works identically on an air-gapped install.
No third-party connector service, SDK, or hosted registry is involved.
Credentials come from the deployment, never the UI
A catalog connector's vendor OAuth app is read only from FORGE_CONNECTOR_OAUTH_APPS,
keyed by credential group (one "google" entry covers Gmail/Calendar/Drive/Sheets). No
route accepts a pasted credential for a catalog install; an unconfigured vendor reports
itself unavailable and names the env key rather than degrading into a form that asks an
end user for a client secret. Slack/Notion/Linear/Atlassian need no entry at all — they
publish OAuth metadata and Forge registers a client dynamically (RFC 9728/8414/7591).
Every account is personal
Catalog connectors are per-user: tokens are stored under the connecting user's identity,
every status is answered for the caller, and disconnecting affects only you. Pasted
("custom") manifests keep the credential form and the shared-account option — which is
where a service account or an internal API with an API key belongs.
Triggers carry an identity and an owner
A webhook or schedule fires with nobody signed in, so a per-user connector had no token
to resolve. Trigger.run_as_user_id names whose accounts an unattended run uses (the
editor who saved the workflow); Trigger.scope says whether the automation is the
project's or that person's, defaulting by role and driving who sees it. Neither is ever
overwritten by a later edit.
Also
- $mime body directive builds and base64url-encodes RFC 2822 messages server-side.
Gmail's send endpoint accepts nothing else, and asking a model to base64-encode by
hand produced an opaque 400.
- "Refresh actions" upgrades installed connectors in place (tool ids preserved, no
re-consent), so a corrected manifest can reach projects that already installed it.
- MCP client-side OAuth: httpx.Auth resolves provider headers per request, so tokens
rotate without reconnecting; discovery asks as the consenting user.
- Token endpoints: Accept: application/json (GitHub) and client_secret_basic (Airtable).
- describe_mcp_error unwraps anyio ExceptionGroups, which reported every MCP failure as
"unhandled errors in a TaskGroup".
- Playground's workflow picker moved into the empty state, where it is seen before the
first question.
Migrations 0011 (connector_installs, mcp_clients.auth_provider_id), 0012 (run_as_user_id),
0013 (scope) — all idempotent and additive; existing rows keep current behaviour.
…sed name `sheets_read_range` addressed a spreadsheet by an opaque key that only exists inside the sheet's URL. Asked to "get testsheet", the model passed the NAME as the id and Google answered 404 — which reads like a permissions problem rather than "that argument was never knowable". Same family as the Gmail `raw` bug: a tool demanding something the model cannot know invites it to invent one. Adds a declarative `extract` regex to request fields: if the supplied value matches, the captured group is used. Paste the link, get the id; a bare id still passes straight through untouched. Applied to google-sheets `spreadsheet_id` and google-drive `file_id`, whose descriptions now also say plainly that a title is not an id — and point at drive_search_files, which already carries the Drive scope, as the way to resolve a name. A no-match deliberately leaves the value alone rather than substituting a best guess: a wrong id that 404s beats one that silently reads someone else's document. Only the first 4KB of a value is searched, since the pattern is authored but the value is model-supplied. Patterns compile at manifest-parse time, so a typo fails at install rather than on someone's first tool call.
…s take rows
Two defects behind the 400 on "add random 100 row".
1. An embedded {{token}} resolving to a list or dict was rendered with str(), which is
Python's repr — single quotes, True/None. `{"values":[['a','b']]}` is not JSON, so the
body failed to parse and was sent as raw text; the API rejected it. This only ever
surfaced on STRING data: `[[1, 2]]` is valid JSON by coincidence, which is why a numeric
first test passed and a text one didn't. Containers now render with json.dumps; scalars
keep str(), so a bare token in a query string still gives "False"/"0".
This affected every REST tool interpolating an array into a JSON body, not just Sheets.
2. sheets_append_row took a 1-D "cells of the one new row" and wrapped it in the template.
Asked for 100 rows the model naturally sent 2-D, the wrap made it 3-D, and the API
rejected that too. It now takes `rows` — always a list of rows — exactly like
sheets_update_range, so one row and many rows are the same shape and 100 rows go out in
one call. insertDataOption defaults to INSERT_ROWS so appending cannot overwrite content
below the table.
Body shape verified against Google's spreadsheets.values.append reference: `values` is a
2-D array, valueInputOption is required, insertDataOption is optional.
Verified all 15 write actions across the eight REST connectors against the published API
references. Three more defects, all the same shape as the Sheets one — a template producing
text the API cannot parse, surfacing as an opaque 400.
* JSON string escaping. A model-written value containing a newline, a double quote or a
backslash terminated its JSON string early, so the body stopped being JSON and was sent as
raw text. A note body of "Line one\nHe said \"hi\"" was enough. Body templates now render
with substituted strings JSON-escaped, falling back to literal rendering when the result
isn't JSON, so form-encoded and plain-text templates are untouched.
* Outlook could only ever send to ONE recipient — toRecipients was a hardcoded single object
— and had no cc/bcc at all. `to`, `cc` and `bcc` are now lists expanded via $each into the
array of {emailAddress:{address}} that Graph documents.
* HubSpot's hs_timestamp was interpolated unquoted as an integer. HubSpot accepts epoch-ms or
ISO 8601 and a model reaches for ISO, which rendered as bare text and broke the JSON. Now a
quoted ISO 8601 string.
Also: Google Calendar start/end now say an RFC3339 UTC offset is required, since a naive
local time is rejected. Confirmed correct and left alone: Airtable's single-record
{"fields":…} body, Outlook's sendMail envelope and calendarView's required window, Gmail
modify, GitHub issue/comment, HubSpot search, Calendar freeBusy.
New test sweeps every catalog write action with deliberately awkward input and asserts the
body parses — the check that would have caught all three by construction.
Nine findings from an xhigh review of the branch, all fixed.
Permissions
POST /connectors/{slug}/sync was opened up so whoever actually signed in could discover a
per-user MCP connector's tools. That was right for MCP but wrong for REST, where the same
call re-applies the manifest over project Tool rows — a viewer could rewrite every Gmail
tool's config. The gate is now split by backend: MCP discovery stays open, REST refresh
requires editor.
Resource leaks
The MCP client cache is keyed per user and every catalog MCP connector is per-user, so it
grew with distinct PEOPLE and never shrank — the TTL only replaces an entry when that key
is asked for again, so an idle user's transport was pinned for the process lifetime. Added
a 64-entry ceiling with TTL sweep and oldest-first eviction, and every drop now closes what
it removes (invalidate_client included, which previously popped without closing).
discover_tools deliberately bypasses the cache, so the client it built was never closed at
all — it now closes in a finally.
Correctness
The 401 retry re-applied auth to the already-mutated request, merging query params twice
and concatenating the cookie jar (`?api_key=X&api_key=X`), which some servers reject —
turning a recoverable 401 into a hard failure. The retry now restores the pre-auth URL and
Cookie first.
Concurrent first-connects on a discovery connector both ran dynamic client registration and
raced the stored credential, leaving the loser's in-flight consent unusable. Serialized per
provider with a re-check inside the lock.
discover() accumulated endpoints across authorization_servers, so an authorize_url from one
server could be paired with a token_url from another. It now only accepts a server that
yields a complete pair.
Refresh recreated any manifest action missing from the live tools, silently restoring a
capability a project had deleted on purpose. It now distinguishes deleted (in the frozen
manifest) from new-in-the-upgrade (not in it).
Efficiency
_auth_for ran twice per MCP resolution — once for the cache-key suffix, once inside
_connection_for — doing two AuthProvider reads on the path every agent turn takes. The
resolved auth is now passed through.
Body templates were rendered twice to discover whether they were JSON; the opening
character decides it instead.
…aller issues
The first item is a regression from the previous commit; the rest are gaps both review
passes had left uncovered.
Body-template escaping
Deciding "is this JSON?" from the first character misread a form-encoded template whose
first token is a placeholder: `{{input.q}}=1¬e={{input.n}}` starts with `{`, so its
values were JSON-escaped and a quote became a literal backslash in the form body. That was
the safety net the two-pass render used to provide, removed in the name of not rendering
twice. A leading `{{` now disqualifies the template.
MCP connection cache
Two concurrent misses for the same key both built a client and the second assignment
replaced the first, orphaning a transport nothing would ever close — the exact leak the
previous commit set out to remove. Builds are now serialized per cache key.
Eviction no longer closes transports on the request path: the dict shrinks synchronously so
the ceiling is honoured immediately, and the closes go to a background task rather than
billing one agent turn for every connection the cache decided to shed.
The 401 retry now restores the whole pre-auth header set, not just Cookie, so a provider
that changed shape between attempts can't leave its first header behind.
Connectors router
A stored manifest with an explicit null backend made the sync gate raise AttributeError
instead of falling through to the editor-gated branch.
The discovery lock is in-process, like the OAuth refresh locks it mirrors — the comment now
says so, and a re-check of the stored client id before writing makes concurrent
registrations across replicas converge on one app instead of overwriting each other.
Listing connectors fetched an auth provider per install; one IN query now covers them all.
Install
Refresh starts the tool-id list from rows that still exist, so ids of deleted tools stop
accumulating in the receipt and in every subsequent IN clause.
…, install rollback Fifteen findings from the xhigh review, fixed at the mechanism rather than the symptom where the previous rounds had patched the symptom. MCP connection pooling (four findings, one root cause each): * The pooled connection's cache key varied only on `per_user_context_keys`, while the `_ProviderAuth` attached to it captures the WHOLE run context. A provider taking its token inline from the run (`token_ctx_key`, and its deployment-wide fallback) therefore pooled the first caller's credential and authenticated everyone else with it for the TTL. `AuthResolver` added that dimension to its own cache key for exactly this reason; `auth_cache_dims` now keeps the two in step, with a test that fails if they drift again. * Eviction closed clients other coroutines were still running against, and picked its victim by build time - so the hot shared connection was shed first. Entries now carry `created` (expiry) and `used` (LRU) separately, and a shed connection is retired with a grace period instead of closed on the spot. * The deferred close went through `spawn`, which REJECTS and closes the coroutine at its in-flight ceiling - stranding every transport it was meant to close. Retirement is now a list drained by `_reap` on the next cache operation, so nothing depends on a background task being accepted. * The per-key build lock lived in a `KeyedLocks` registry that never evicts, growing one lock per (server, person) forever - the same unbounded growth `_CACHE_MAX` exists to prevent. The lock now lives in the cache entry, so it is bounded by the cache, and eviction skips entries that are mid-build. Install pipeline: * A failed install left the vendor client id/secret in the project store with no install pointing at it, which `group_has_credentials` then read as "this group is already configured". Rollback now clears them - and only the ones the install CREATED, so a sibling sharing the vendor app is untouched. * Two people clicking Connect at once both passed the read-based duplicate check; the loser hit the unique constraint and got a 500. That is now reported as InstallError, and connect-on-demand joins the winner rather than failing the person who lost the race. * Refreshing a REST connector overwrote the stored manifest, including the credential group that uninstall reads to decide whether a sibling still needs the shared vendor app. The group is now pinned across upgrades. Triggers: * A workflow saved by a machine principal stamped `service` or `apikey:<uuid>` as the run-as identity. The latter is 43 characters into a String(36) column, and `_sync_triggers` swallows exceptions - so on Postgres the symptom was webhooks and schedules silently never being registered. * An editor could make a trigger personal that runs as someone else (or as nobody), which hid it from their own screen with no way back. Only the person it runs as, or an admin, can now do that, and the button is only rendered when it is yours. Also: the token exchange and the authorize leg now compute `redirect_uri` from one helper (OAuth requires them to match exactly); the escaping decision for a body template reads the declared `body_encoding`/Content-Type instead of sniffing the first character, so it cannot disagree with how the body is actually serialized; `.env.example` and config.py said to register a Google "Desktop app" client, which has no redirect-URI field and reproduces the `redirect_uri_mismatch` this branch already fixed; the connector screen's focus listener is detached on unmount; connection status is computed in one place instead of two; and the unreachable "mcp" screen route is removed. 637 tests pass (18 new), ruff and tsc clean.
…ner's credentials `_rollback` was taught to clear the secrets an install created (0e52b55), and the duplicate-install path reused it unchanged. But two racers on the one-click path both read the credential group as empty before either writes, so both record the group's client id/secret as "secrets I created". The loser's rollback then blanks the very credential the winning install now depends on, and every later Connect fails with "client_id secret is not set". Pass an empty list on the IntegrityError path: the winner owns those credentials. Other failure modes still clear what they created.
…rvive past an hour None of the four Google manifests set `authorize_params`, so the authorize URL went out without `access_type=offline`. Google returns a refresh_token only when that parameter is present, and re-issues it reliably only with `prompt=consent`, so every install stored a bundle with refresh_token=None. `AuthResolver._oauth2_auth_code` gates refresh on that key, so once expires_at passed it silently kept returning the dead access token: Gmail, Calendar, Drive and Sheets all 401 about an hour after sign-in, for every user, curable only by Disconnect + reconnect. Outlook already asks via the `offline_access` scope; Airtable, HubSpot and GitHub refresh or never expire. Google was the only gap. Two tests, because one alone would not have caught it: a catalog sweep asserting every accounts.google.com manifest opts in, and a walk of the real Connect chain (_auth_config -> build_authorize_url) reading the query string Google would receive.
build_authorize_url had a special case reading top-level cfg["access_type"] / cfg["prompt"], which _auth_config never writes and no UI offers - so it was dead on every path a connector takes, while looking authoritative enough that four Google manifests shipped without asking for offline access. `authorize_params` is the documented, generic mechanism; the special case is gone. Tests pin the collapse: the top-level spelling is ignored, authorize_params is honoured, and extras still cannot trample redirect_uri, state or code_challenge_method.
…ild lock Two problems in the same function, both introduced by the previous round's leak fix. Retired clients were drained only by `_reap`, which ran only from `_evict` (inside a cache BUILD) and from `close_all`. A burst that pushed the cache over its ceiling retired ten transports; if traffic then settled into a steady state where every call hit a live entry, no build happened and nothing closed them until a key expired or the process shut down - far past the 30s grace. Verified against the old code: five shed transports stayed open indefinitely with no cache activity. And `_evict` awaited `_reap` while `_client_and_tools` held the per-key build lock, putting an unbounded network teardown in front of a concurrent caller for that same key. Both are fixed by one change of shape: a single long-lived reaper task closes retired transports near their deadline, and `_evict` becomes synchronous - so it is structurally incapable of awaiting anything under the lock. The reaper exits when the queue drains and `_retire` starts it again; `close_all` cancels it and force-drains. Not a task per retirement: that is what leaked before, since `spawn` rejects and closes its coroutine at an in-flight ceiling. `_reap`'s docstring claimed "every build reaps first", which was never what the code did.
`install` appended every connector host to the project's allow-list and nothing ever removed them. Install Gmail, Sheets, HubSpot and Stripe to evaluate them, uninstall all four, and gmail.googleapis.com / sheets.googleapis.com / api.hubapi.com / api.stripe.com stay reachable forever with nothing referencing them - so any hand-written tool a project editor adds later can reach them, which is what default-deny exists to prevent. Removing `manifest.hosts()` on the way out would be wrong in the other direction: a host somebody allow-listed by hand before the connector arrived is not the install's to take away, and afterwards the two are indistinguishable. So `_allow_egress` now returns the hosts it genuinely added and the install records them (new `created_egress_hosts` column, migration 0014). Rows predating the column read as "unknown" and are left alone. `_revoke_egress` subtracts what surviving installs still need, comparing on their MANIFEST hosts rather than their receipts - the first Google connector records oauth2.googleapis.com and the second records nothing, so receipts alone would revoke a host a sibling is using. A host that survives has its receipt handed to that sibling, otherwise removing Gmail and then Calendar would orphan the host with no install left that remembers adding it. Also revoked on install ROLLBACK: the allow-list is widened before the row is written, so a failure after that point left the same permanent widening with no connector to blame.
…ne read The AuthProvider lookups were batched in an earlier round, but `_connected_for` still did one `SecretStore.read_ref` per install, sequentially - a DB round trip, a decrypt and an audit session each. A project with the full catalog installed paid that a dozen times on every paint, and the connect flow calls reload() on window focus, so it repeated every time someone came back from a consent window. Adds `SecretStore.read_refs`: same choke point, same audit trail, one SELECT. Auditing goes through a new `AuditService.log_many` so N reads don't become N audit sessions - batching the read and then serialising the audit would have moved the cost, not removed it. `_connection_state` now takes the pre-read bundle, with `_bundle_name_for` naming the secret for both the per-user and shared cases so the list and status routes agree on which one decides. The status route is unchanged and still reads its single bundle itself.
`list_installed` counted tools that still exist. `connector_status` called `_install_out` with
no count and fell back to `len(created_tool_ids)`, so it reported actions the user had deleted:
remove two of Gmail's five from the Tools screen and the gallery correctly showed 3 while the
detail panel - which is the thing that polls `/{slug}/status` - insisted on 5.
The count now comes from one `_live_tool_counts` helper, and `tool_count` is a REQUIRED
argument to `_install_out`. The optional fallback was the bug: a call site that forgot it got
the stale number silently. The two install routes pass the receipt explicitly, which at that
moment is the live count.
The header amends what shipped, but §7's decisions table was left intact - so anyone landing
on the table read D4 ("ship no OAuth client credentials; each install registers its own app")
and D6 ("per-user MCP is a Phase 1.5 follow-on") as current decisions. Both were overruled:
the vendor app is deployment-wide via FORGE_CONNECTOR_OAUTH_APPS, and per-user MCP shipped in
Phase 1 because catalog connectors are per-user always.
Both rows are struck with a "Shipped?" column saying what replaced them and why. Also fixes
the header's "three ways", which has listed five for two commits.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name=name, value="", kind="connector", | ||
| ) | ||
| except Exception as e: # noqa: BLE001 - a missing secret is already the goal state | ||
| log.debug("connector uninstall: could not clear secret %s (%s)", name, e) |
| name=name, value="", kind="connector", | ||
| ) | ||
| except Exception as e: # noqa: BLE001 - an unwritable secret is not worth masking | ||
| log.warning("connector install rollback could not clear secret %s: %s", name, e) |
| try: | ||
| manifest = parse_manifest(body.get("manifest") if "manifest" in body else body) | ||
| except ManifestError as e: | ||
| return {"ok": False, "error": str(e)} |
| except Exception as e: # noqa: BLE001 - report, don't 500: the usual cause is "not connected yet" | ||
| from forge.tools.mcp import describe_mcp_error | ||
|
|
||
| return {"ok": False, "error": describe_mcp_error(e)} |
| return {"ok": False, "error": f"Could not connect: {e}"} | ||
| # Unwrapped, an anyio task group reports every failure as "unhandled errors in a | ||
| # TaskGroup", which hides the 401/DNS/TLS cause the user needs to see. | ||
| return {"ok": False, "error": f"Could not connect: {describe_mcp_error(e)}"} |
| # The per-user dims this consent was for, carried through the signed state. Discovery below | ||
| # has to ask the server AS THAT USER - it is the only credential that exists. | ||
| note = await _finish_connector_connect(session, tenant_id, project_id, ap_id, claims.get("ctx")) | ||
| return HTMLResponse(f"<h3>✅ Connected</h3><p>You can close this window and return to Forge.{note}</p>") |
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
2 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.
What & why
Type of change
Checklist
ruff check forge migrationsis clean andpytest -qpasses (fromapps/api)mypy-cleanpnpm --filter web buildpasses (from repo root)packages/schemas) updated if node/tool config changedCHANGELOG.mdupdated under Unreleased (for user-facing changes)Notes for reviewers