Skip to content

Commit 0c3b266

Browse files
authored
fix: an OAuth connection can no longer be pinned to read forever (#107) (#108)
* fix(oauth): let the owner decide what a connected app may do An OAuth connection was pinned to saddle:read at consent time and could never be raised. get_tier() returns min(site tier, credential ceiling), so a site set to "Managing the site" still ran every ChatGPT call at read and tools/list withheld every write tool — 83 of 160 offered, with the agent able to draft an SEO title and description and unable to save it. Three places named saddle:read as a constant. normalize_scope() fell back to it when the client sent no scope, and ChatGPT sends none: it registers dynamically and starts the flow without a scope parameter, so that was the live cause. The 401 challenge advertised it regardless of the site's level. The consent screen rendered Allow/Deny with no control that could grant more than was asked for. Reconnecting produced an identical grant. Now: tier_to_scope()/site_scope() derive the scope from the site's own level; the challenge advertises it; the authorize endpoint falls back to it ONLY when the client sent no scope at all, because widening a request a client actually made is both a spec violation and a safety regression; the consent screen offers the levels up to the site tier and clamps the posted choice server-side; and set_grant_scope() plus POST /oauth-connections/<id> repair a connection that already exists. The ceiling now takes the lower of the token's scope and its grant's, so lowering a connection's level lands on the next request instead of whenever its hour-long access token happens to expire. Raising still needs the token rewritten, which is the safe direction to be strict in. $grant was renamed $token_record because it never held a grant, and mistaking one for the other is exactly what makes this fail quietly. Also hardens update_settings(), which read JSON params only: a form-encoded tier passed the enum, passed can_manage, returned 200 and saved nothing. Refs #107 * feat(admin): change a connected app's level, and say when one is behind Connect now carries a level picker per signed-in app, capped at the site tier, so the connection that shipped at read can be raised without a reconnect — which would not have helped anyway, since an app that requests no scope comes back at the same level every time. Permissions names any connection sitting below the level saved there. That gap was invisible from the one screen where someone sets the level, which is how "I set this to Managing the site and my app still can't save anything" happens. The apply-bar note said already-connected apps keep the old tool list "until they reconnect". Wrong for apps that sign in themselves: reconnecting re-runs consent and re-grants the same level. They need a refresh, not a reconnect. Refs #107 * chore: rebuild the admin bundle, refresh the pot, changelog the fix The pot had gone stale again — it was generated against 1.0.0-rc3 and was missing most of a release's worth of msgids, not only this change's. Refs #107 * docs(status): the OAuth read-pin, its two traps, and the live-test question Refs #107
1 parent 56277d2 commit 0c3b266

17 files changed

Lines changed: 1984 additions & 269 deletions

STATUS.md

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,19 @@
11
## Last session
2+
2026-08-17 (**an OAuth connection could be pinned to `read` forever — root-caused, fixed, and the repair path built**) — Fahim's report: Permissions set to "Managing the site", ChatGPT reporting `read` access and **83 of 160** tools, every Waggle write tool missing, and "edit operation fails" on `waggle-update-seo-meta`. Refreshing the connector changed nothing. **It was ours, on both sides.**[#107](https://github.com/plugpressco/saddle/issues/107) / PR [#108](https://github.com/plugpressco/saddle/pull/108), and waggle [#181](https://github.com/plugpressco/waggle/issues/181) / PR [#182](https://github.com/plugpressco/waggle/pull/182). 594 tests (was 577), 0 lint errors. **Both PRs open, not merged** — over the 400-line ask-first threshold and OAuth code.
3+
4+
**The chain, in order.** `get_tier()` is `min(site tier, credential ceiling)`; site tier was `admin`, ceiling was `read`, so `filter_adapter_tools_list()` dropped every ability above `read` at dispatch. The ceiling was `read` because **three** places named `saddle:read` as a constant, and the live one is the least obvious: `normalize_scope()` fell back to `DEFAULT_SCOPE` when the client sent no scope — and **ChatGPT sends none at all**, it registers via DCR and starts the flow with no `scope` parameter ([confirmed in OpenAI's own developer forum](https://community.openai.com/t/how-are-the-scopes-that-the-chatgpt-connector-requests-when-authorizing-the-mcp-server-determined/1363723)). The 401 challenge advertised `saddle:read` regardless of the site's level, and the consent screen had a branch for "asked for more than the site allows" and none for "asked for less". So every ChatGPT grant landed on read, permanently, and **reconnecting produced an identical grant** — the one remedy the UI suggested was the one that could not work.
5+
6+
**The fix keeps the guardrail it looks like it breaks.** A scope still only ever *lowers* the site tier (pinned by a test). What changed is who proposes it: the consent screen now offers the levels up to `get_site_tier()` and clamps the posted choice server-side, and the authorize endpoint falls back to the site's scope **only when the client sent no scope at all** — an explicit `saddle:read` is still granted exactly that, because widening a request a client actually made is a spec violation and some clients compare the scope they get back. Plus `set_grant_scope()` + `POST /oauth-connections/<id>` + a level picker on Connect, so the connection that already exists is repairable without a reconnect.
7+
8+
**Two traps found inside the fix.** `Saddle_OAuth_Bearer::$grant` never held a grant — it held the access-token record, whose `scope` is frozen for `ACCESS_TTL`. Rewriting only the grant would have made a level change appear to do nothing for an hour, so `set_grant_scope()` rewrites both and the property is now `$token_record`. And `tier_ceiling()` now takes the *lower* of token and grant scope, so lowering a level lands on the next request while raising still needs the token rewritten — strict in the safe direction.
9+
10+
**Waggle's half.** The bundled `seo-write-article` skill told the agent to call `waggle/update-seo-meta` and six more like it. Saddle re-registers Waggle's abilities wrapped, so the only callable name is `saddle/waggle-*` — the agent was looking up tools that were not in its list, and would have failed even at the write tier. The existing test could not have caught it: every bare name is a substring of its wrapped form, so `assertStringContainsString( 'waggle/get-aeo-score' )` passed before and after. There is now one that checks for the bare form directly.
11+
12+
**Also swept:** `update_settings()` read `get_json_params()` only, so a form-encoded `tier` passed the enum, passed `can_manage`, returned 200 and saved nothing — invisible from the dashboard, indistinguishable from "it didn't stick", and this codebase already works around one host that rewrites bodies. The `.pot` was stale against 1.0.0-rc3 again, missing far more than this change.
13+
14+
**Open question for the live test, not a Saddle bug:** OpenAI's [developer-mode docs](https://developers.openai.com/api/docs/guides/developer-mode) say write actions work on every eligible plan with confirmation; several secondary sources and this file's own line below say fully write-capable custom connectors are Business/Enterprise/Edu only. The Connect tab now says so out loud. Settle it on the round-trip and record the answer here.
15+
16+
## Previous session
217
2026-08-16 (**the four pillars swept, Mark's ChatGPT bug root-caused and fixed, and every open PR resolved**) — Fahim's ask opened as "context, guardrails, tools and a system must work smoothly", turned into Mark Roach's ChatGPT report mid-session, and ended with "merge now" on the whole open-PR backlog. Eleven PRs merged (#90, #92, #94, #96, #98, #99, #66, #68, #69, #100, #101, #102); #70 closed as already-landed; **zero open PRs**. 575 tests, 0 lint errors.
318

419
**Mark's bug is fixed, and it was ours.** His two-client experiment isolated it: Claude Desktop via mcp-remote worked end to end against staging.kesuk.net while ChatGPT completed OAuth and then reported no callable actions. That rules out abilities, tiers, OAuth and the endpoint, leaving the handshake. `Saddle_MCP` violated three Streamable-HTTP MUSTs, and one sits between "connected" and `tools/list`: a notification must be answered **202 with no body**, and Saddle answered `200` with the JSON literal `null`. mcp-remote shrugs; a strict client treats the handshake as unfinished and never asks what tools exist. Also fixed: GET now 405 (was 404), DELETE 405, unsupported `MCP-Protocol-Version` 400, unparseable body 400, and `resources/*`/`prompts/list` answer empty lists instead of Method-not-found. **This was never only Mark** — since `bff1a99` the .org zip has no adapter, so that transport is the only one a .org install will ever have.
@@ -269,7 +284,7 @@ Earlier 2026-07-12 — **Audit-backlog execution (P0–P4)**. Cleared the audit
269284
- **Delete the v1.1.0 GitHub release + tag (one command):** `gh release delete v1.1.0 --repo plugpressco/saddle --cleanup-tag --yes && git tag -d v1.1.0`. Still outstanding, and #69 tried to reintroduce 1.1.0 this session — reverted in four places, including a `Stable tag` that was not a conflict and would have shipped silently.
270285
- **Run `php scripts/revendor-wp-mcp.php --check` before any release.** New pre-release guard: it exits non-zero if the vendored library was refreshed without re-applying Saddle's text domain.
271286
- **SUBMIT to WordPress.org.** Everything code-side is done and this session removed one of the reviewer-facing surfaces (#86). Fahim's three steps are in `WPORG-SUBMISSION.md`. Rebuild first: `npm run build && npx grunt build` — never `grunt release`, it bumps the version.
272-
- **Live ChatGPT round-trip on plugpress.co** — still the real gate for the OAuth work, and now also the confirmation that the handshake fix holds against the client that exposed it.
287+
- **Live ChatGPT round-trip on plugpress.co** — still the real gate for the OAuth work, and now carrying three questions at once: the handshake fix against the client that exposed it, [#108](https://github.com/plugpressco/saddle/pull/108)'s level picker (set the existing connection to "Managing the site" on Connect, refresh the connector, expect ~160 tools and `saddle-waggle-update-seo-meta` present), and the plan question — if the write tools are *listed* and ChatGPT still declines to call them, that is the Business/Enterprise/Edu gate, not us. Record the answer in this file either way.
273288
- **CI PHPUnit still red on GitHub Actions** (no WP core in the runner). Worth its own PR so future PRs get a real green; "green" currently means someone ran the suite locally and said so.
274289

275290
## Blockers / open questions

admin/build/index.asset.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
<?php return array('dependencies' => array('react', 'react-dom', 'wp-api-fetch', 'wp-element', 'wp-hooks', 'wp-i18n'), 'version' => 'c85823a67cb3c40b8700');
1+
<?php return array('dependencies' => array('react', 'react-dom', 'wp-api-fetch', 'wp-element', 'wp-hooks', 'wp-i18n'), 'version' => '410d31640c83864208ef');

admin/build/index.js

Lines changed: 44 additions & 41 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

admin/src/App.jsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -642,6 +642,7 @@ export default function App() {
642642
onConnect={ openWizard }
643643
onClientsChanged={ refreshClients }
644644
onClientRemoved={ removeClient }
645+
siteTier={ tier }
645646
/>
646647
) }
647648
{ tab === 'integrations' && (

admin/src/components/ConnectedClients.jsx

Lines changed: 94 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,12 @@ import {
2222
Card,
2323
CardContent,
2424
HelpTip,
25+
NativeSelect,
2526
useConfirm,
2627
toast,
2728
} from '@plugpress/ui';
2829
import { __, sprintf } from '@wordpress/i18n';
29-
import { saddleData, api } from '../api';
30+
import { saddleData, api, LEVELS, tierUnlocks } from '../api';
3031
import ConnectionHealth from './ConnectionHealth';
3132
import McpDiagnostics from './McpDiagnostics';
3233
import SetupGuideDrawer from './SetupGuideDrawer';
@@ -60,12 +61,16 @@ const isStaleNeverUsed = ( c ) =>
6061
c.created &&
6162
Date.now() / 1000 - c.created > NEVER_USED_STALE_SECONDS;
6263

64+
const levelTitle = ( key ) =>
65+
( LEVELS.find( ( l ) => l.key === key ) || {} ).title || key;
66+
6367
export default function Apps( {
6468
clients,
6569
loading,
6670
onConnect,
6771
onClientsChanged,
6872
onClientRemoved,
73+
siteTier,
6974
} ) {
7075
const confirm = useConfirm();
7176
const [ showAdvanced, setShowAdvanced ] = useState( false );
@@ -120,6 +125,44 @@ export default function Apps( {
120125
} );
121126
};
122127

128+
// Apps that sign in themselves are granted a level once, at the consent
129+
// screen, and used to be stuck with it — an app that requests no scope
130+
// (ChatGPT does not request one) landed on read and no screen anywhere could
131+
// raise it. This is that screen.
132+
const changeOauthLevel = ( c, level ) => {
133+
if ( level === c.level ) {
134+
return;
135+
}
136+
137+
// Optimistic, then reconcile — the same pattern revoke uses above.
138+
setOauthConnections( ( list ) =>
139+
list.map( ( x ) => ( x.id === c.id ? { ...x, level } : x ) )
140+
);
141+
142+
api( `oauth-connections/${ c.id }`, {
143+
method: 'POST',
144+
data: { level },
145+
} )
146+
.then( () => {
147+
toast.success(
148+
sprintf(
149+
/* translators: 1: the app name, 2: its new access level. */
150+
__(
151+
'“%1$s” is now set to “%2$s”. Refresh or reopen the app to pick up its new tools — apps don’t notice on their own.',
152+
'saddle'
153+
),
154+
c.name,
155+
levelTitle( level )
156+
)
157+
);
158+
refreshOauth();
159+
} )
160+
.catch( ( e ) => {
161+
toast.error( e.message );
162+
refreshOauth();
163+
} );
164+
};
165+
123166
const askRotate = async ( c ) => {
124167
const ok = await confirm( {
125168
title: sprintf(
@@ -447,15 +490,56 @@ export default function Apps( {
447490
{ c.name }
448491
</>
449492
}
450-
description={ sprintf(
451-
/* translators: 1: access level, 2: WordPress username. */
452-
__(
453-
'%1$s access, acting as %2$s',
454-
'saddle'
455-
),
456-
c.level,
457-
c.user_login
458-
) }
493+
description={
494+
<>
495+
<NativeSelect
496+
value={ c.level }
497+
aria-label={ sprintf(
498+
/* translators: %s: the app name. */
499+
__(
500+
'Access level for %s',
501+
'saddle'
502+
),
503+
c.name
504+
) }
505+
onChange={ ( e ) =>
506+
changeOauthLevel(
507+
c,
508+
e.target
509+
.value
510+
)
511+
}
512+
>
513+
{ LEVELS.filter(
514+
( l ) =>
515+
tierUnlocks(
516+
siteTier ||
517+
c.level,
518+
l.key
519+
)
520+
).map( ( l ) => (
521+
<option
522+
key={
523+
l.key
524+
}
525+
value={
526+
l.key
527+
}
528+
>
529+
{ l.title }
530+
</option>
531+
) ) }
532+
</NativeSelect>{ ' ' }
533+
{ sprintf(
534+
/* translators: %s: WordPress username. */
535+
__(
536+
'acting as %s',
537+
'saddle'
538+
),
539+
c.user_login
540+
) }
541+
</>
542+
}
459543
actions={
460544
<>
461545
<Badge

admin/src/components/Permissions.jsx

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@
55
* 18 tools lives behind a disclosure for anyone who wants to verify exactly
66
* what's included — invisible for everyone else. Nothing saves until you apply.
77
*/
8-
import { useState, useMemo } from '@wordpress/element';
8+
import { useState, useMemo, useEffect } from '@wordpress/element';
99
import {
1010
CardRadioGroup,
1111
Collapsible,
1212
ApplyBar,
13+
Notice,
1314
toast,
1415
PageHeader,
1516
Tooltip,
@@ -60,6 +61,25 @@ export default function Permissions( {
6061
savedDisabledSet( caps )
6162
);
6263

64+
// Apps that signed in themselves carry a level of their own, granted once at
65+
// the consent screen, and it can sit below the site's. From this screen that
66+
// gap is invisible — which is exactly how "I set this to Managing the site
67+
// and my app still can't save anything" happens. Name the app here instead of
68+
// leaving it to be discovered as a string of refusals.
69+
const [ underLevelled, setUnderLevelled ] = useState( [] );
70+
71+
useEffect( () => {
72+
api( 'oauth-connections' )
73+
.then( ( res ) =>
74+
setUnderLevelled(
75+
( res || [] ).filter(
76+
( c ) => ! tierUnlocks( c.level, savedTier )
77+
)
78+
)
79+
)
80+
.catch( () => setUnderLevelled( [] ) );
81+
}, [ savedTier ] );
82+
6383
// Free text filter across a tool's name/id/description. Empty matches all.
6484
const q = query.trim().toLowerCase();
6585
const matchesQuery = ( c ) =>
@@ -211,6 +231,31 @@ export default function Permissions( {
211231
} ) ) }
212232
/>
213233

234+
{ underLevelled.length > 0 && (
235+
<Notice tone="warning">
236+
{ sprintf(
237+
/* translators: 1: comma-separated app names, 2: the level each app is stuck at, 3: the site's level. */
238+
_n(
239+
'%1$s is connected at “%2$s”, so it is not offered the tools this “%3$s” level unlocks. Change it on the Connect tab — apps that sign in themselves keep the level they were approved with.',
240+
'%1$s are connected at “%2$s” or lower, so they are not offered the tools this “%3$s” level unlocks. Change them on the Connect tab — apps that sign in themselves keep the level they were approved with.',
241+
underLevelled.length,
242+
'saddle'
243+
),
244+
underLevelled.map( ( c ) => c.name ).join( ', ' ),
245+
(
246+
LEVELS.find(
247+
( l ) => l.key === underLevelled[ 0 ].level
248+
) || {}
249+
).title || underLevelled[ 0 ].level,
250+
(
251+
LEVELS.find(
252+
( l ) => l.key === levelKey( savedTier )
253+
) || {}
254+
).title || savedTier
255+
) }
256+
</Notice>
257+
) }
258+
214259
<Collapsible
215260
className="saddle-perm__all"
216261
open={ showAll }
@@ -416,11 +461,15 @@ export default function Permissions( {
416461
: null,
417462
// Most AI apps read the tool list once, when they
418463
// connect. Saving a change they can't see until
419-
// they reconnect is the kind of thing that reads
420-
// as "it didn't work".
464+
// they look again is the kind of thing that reads
465+
// as "it didn't work". "Reconnect" was the old
466+
// wording and was wrong for apps that sign in
467+
// themselves — reconnecting one re-runs consent and
468+
// re-grants whatever level it had, which is not
469+
// what this change did.
421470
dirty || abilitiesDirty
422471
? __(
423-
'Already-connected apps keep the old tool list until they reconnect.',
472+
'Already-connected apps keep the old tool list until you refresh or reopen them.',
424473
'saddle'
425474
)
426475
: null,

admin/src/connect-apps.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,13 @@ export const APPS = [
5151
'In ChatGPT on the web: turn on Developer mode (Settings → Apps & Connectors → Advanced settings), then create a connector. Paste the address, choose OAuth, and leave the client ID and secret blank. ChatGPT sends you here to approve it — the connector then works in the desktop app too.',
5252
'saddle'
5353
),
54+
// Worth saying on the screen rather than in a support email: OpenAI has
55+
// gated write-capable custom connectors to Business, Enterprise and Edu
56+
// workspaces at various points, and on a personal plan ChatGPT can end up
57+
// offered every tool and still decline to use the ones that change
58+
// anything. That looks identical to a Saddle permission problem.
5459
next: __(
55-
'Enable the connector in a ChatGPT chat and ask it about your site.',
60+
'Enable the connector in a ChatGPT chat and ask it about your site. If it can read but refuses to change anything, check your ChatGPT plan — write-capable custom connectors have been limited to Business, Enterprise and Edu workspaces.',
5661
'saddle'
5762
),
5863
},

0 commit comments

Comments
 (0)