Repository navigation
fix(ios): improve mobile keyboard UX and touch interactions - #250
Merged
Merged
Conversation
Keyboard UX improvements: - Add useMobileKeyboard hook for keyboard state tracking - Add IME composition handling to prevent accidental sends during predictive text - Dismiss keyboard on send and when AI streaming starts - Add keyboard-aware positioning for InputPositioner and popups - Add CSS utilities for iOS (16px font-size, keyboard padding) - Update FloatingCellEditor with keyboard-aware positioning Navigation improvements: - Auto-close sidebar sheets on navigation (Capacitor only) - Add touch-manipulation and onTouchEnd to PageTreeItem links Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a mobile keyboard management system (hook + helpers), IME composition guards, and keyboard-aware layout/positioning adjustments across chat inputs and floating editors; includes minor touch/styling tweaks and an Xcode development team entry. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant App as Capacitor Web App
participant CSS as DOM / CSS Vars
participant Hook as useMobileKeyboard
participant Component as UI Component (Chat / Editor)
participant Keyboard as On-Screen Keyboard
User->>Component: tap input
Component->>App: focus input
App->>Keyboard: show keyboard
App->>CSS: add `keyboard-open`, set `--keyboard-height`
CSS->>Hook: MutationObserver detects changes
Hook->>Component: isOpen=true, height
Component->>Component: adjust position/padding to remain visible
Component->>Hook: onSend / streaming start -> dismissKeyboard()
Hook->>App: blur active element (iOS Capacitor)
App->>Keyboard: hide keyboard
App->>CSS: remove `keyboard-open`, set `--keyboard-height=0`
CSS->>Hook: MutationObserver detects changes
Hook->>Component: isOpen=false, height=0
Component->>Component: reset position/padding
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a91b62172a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+160
to
+164
| // Calculate position, accounting for keyboard on iOS | ||
| const viewportHeight = typeof window !== 'undefined' | ||
| ? (window.visualViewport?.height ?? window.innerHeight) | ||
| : 0; | ||
| const availableHeight = viewportHeight - keyboardHeight; |
There was a problem hiding this comment.
On iOS/WKWebView, visualViewport.height already reflects the visible area when the keyboard is open. Subtracting the CSS --keyboard-height again makes availableHeight too small, so the floating editor can be pushed far above the cell (or even off‑screen) when the keyboard appears. Consider using visualViewport.height directly when available and only subtracting the CSS height when you fall back to window.innerHeight.
Useful? React with 👍 / 👎.
Comment on lines
107
to
+110
| // Handle vertical overflow - position above if not enough space below | ||
| if (top + popupHeight > window.innerHeight - 20) { | ||
| // Account for keyboard height on iOS | ||
| const availableHeight = viewportHeight - keyboardOffset; | ||
| if (top + popupHeight > availableHeight - 20) { |
There was a problem hiding this comment.
In calculateInlinePosition, getViewportHeight() already prefers visualViewport.height, which shrinks with the on‑screen keyboard on iOS. Subtracting --keyboard-height again double-counts the keyboard and can force suggestion popups to flip above or clamp off‑screen even when there is enough space. Use either visual viewport sizing or CSS keyboard offset, not both simultaneously.
Useful? React with 👍 / 👎.
- Remove unnecessary async from dismiss() callback - Add keyboard height caching with 100ms TTL to reduce getComputedStyle calls - Add comprehensive tests for useMobileKeyboard hook (18 tests) - Add tests for positioningService helpers (11 tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Owner
Author
Response to Codex Review CommentsRe: "Avoid double-subtracting iOS keyboard height" (FloatingCellEditor.tsx)Re: "Avoid double-counting keyboard height for popups" (positioningService.ts)Thanks for the review! However, these concerns don't apply to PageSpace's configuration. We use Keyboard: {
resize: KeyboardResize.Body,
resizeOnFullScreen: true,
}With
For fixed-positioned elements like FloatingCellEditor and inline popups, body padding doesn't help—we need to explicitly account for keyboard height via the No double-subtraction occurs because |
This was referenced Jan 30, 2026
This was referenced Feb 6, 2026
6 tasks
2witstudios
added a commit
that referenced
this pull request
Jul 6, 2026
…ression comment The inline // codeql[js/clear-text-storage-of-sensitive-data] comment added in b8b6592 did not actually suppress the alert -- the check still failed on the next analysis run. This repo's CodeQL setup evidently doesn't honor inline suppression comments (unlike the codeql-cli's local suppression feature). Dismissed alert #250 directly via the code-scanning API with an explicit false-positive reason/comment instead, and replaced the non-functional directive with a plain explanatory comment.
2witstudios
added a commit
that referenced
this pull request
Jul 6, 2026
…1878) * fix(oauth,mcp-tokens): require step-up auth for credential minting Closes a verified privilege-escalation path: an account-scoped OAuth access token, mintable silently via refresh-token replay (no browser, no human) from any local process holding the CLI's stored refresh token, could mint brand-new mcp_* tokens and approve OAuth consent — both durable, drive-scoped credentials — with zero live user action. - New `webauthn_stepup` + `stepup_grant` verificationTokens types with a pure decision core (step-up-decisions.ts, 27 unit tests, zero mocking) and a thin IO wrapper (step-up-service.ts). Every grant is bound to a specific pending request via computeActionBindingHash, so a grant obtained for one mint request can't be replayed against a different one. - Magic-link fallback for passkey-less users: reuses the existing verifyMagicLinkToken verifier: a fresh, single-use, action-bound link emailed to the user's own registered address. - New ceremony routes: POST /api/auth/step-up/webauthn/{options,verify}, POST /api/auth/step-up/magic-link/request, GET /api/auth/step-up/magic-link/verify. - POST/PATCH /api/auth/mcp-tokens(/[tokenId]): dropped 'oauth' from the write allow-list (session only) and require a consumed step-up grant bound to the name+drive-scopes being minted/widened. PATCH was audited and found to be the same escalation shape as POST (widens an existing token's scope) so it got the identical treatment; DELETE (revoke-only, narrows access) is unaffected. - POST /api/oauth/authorize: consent Allow requires a step-up grant bound to client_id+redirect_uri+scope+state before minting an authorization code. ConsentActions.tsx runs the WebAuthn ceremony inline before Allow, falling back to the magic-link flow when the user has no passkey. - Audited every apps/web/src/app/api route accepting OAuth bearer auth; no other credential-minting/escalation path found. All non-oracle: ceremony/grant failures collapse to one constant-shape error per stage so a caller can't distinguish missing/expired/wrong from the response. More commits from Tasks 2-6 of this pipeline (CLI wiring, named credential profiles, and further hardening) are coming to this same branch before this is ready for final review/merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013m2L2Mc2JFrCK9hd2rBNXG * feat(cli): named credential profiles (Phase 8 task 3) Credentials file schema bumps to v2: `{ version: 2, hosts: { [host]: { profiles: { [name]: HostCredential } } } }`, replacing the old one-unnamed- credential-per-host shape. A v1 file is migrated automatically and losslessly on read — each host's credential becomes that host's "default" profile, so existing users see zero behavior change. - serialize.ts: v1->v2 migration, DEFAULT_PROFILE_NAME, and getHost/upsertHost/removeHost/listSummaries all gain a profile-name parameter (default "default"). - file-store.ts / store.ts (CompositeCredentialStore): get/set/delete/list thread the profile name through to both the file store and the keychain. - keychain.ts: new keychainAccountKey/parseKeychainAccountKey. The "default" profile keeps the plain host as its account key (backward compatible with every existing keychain entry); a named profile gets a distinct key. Uses a NUL separator rather than a printable one like ":" since hosts are full origins ("https://...") that already contain colons. - resolveAuth gains a profileName parameter, keyed one level deeper into `profiles[host][profileName]`; new resolveProfileName resolves the name itself (--profile flag > PAGESPACE_PROFILE env > "default"), same precedence shape as the existing token resolution. - argv/parse.ts: new --profile / --profile=value flag. - login.ts, logout.ts (incl. --all), whoami.ts, and run.ts's generic resolver all resolve and thread the profile name through their store calls. `tokens create`'s own --profile/--save-as-profile wiring is Task 2's responsibility on a separate branch. Extends (does not replace) existing test coverage in serialize/file-store/ store/keychain/resolve/parse/login/logout/whoami/run test files, plus new migration, multi-profile, and precedence tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AsP734YDz9cGFh9kuUfcpS * feat(cli): route tokens create through browser consent (Phase 8 task 2) `tokensCreateHandler` no longer POSTs directly to /api/auth/mcp-tokens with whatever ambient credential resolveAuth finds - it now calls runLoopbackLogin (the same function `pagespace login` uses) with a `drive:<id>:<role> offline_access` scope, opening the browser to the OAuth consent screen Task 1's step-up gate now protects. Minting a token from the CLI is a deliberate, human-approved action every time, never a silent agent-runnable call. - buildTokenScope: pure --drive/--role -> OAuth drive-scope grammar mapper, validated against the resource-id pattern; drift-guarded in tests against @pagespace/lib's canonical parseScopeList/formatScopeSet (never imported at runtime, matching auth/client.ts's precedent). - resolveTokenProfileName: --save-as-profile if given, else the sole drive's id; requires an explicit name when scoping multiple drives. - The resulting refresh token is persisted under that named profile via Task 3's profile storage, never the "default" login slot, with the same overwrite protection (--yes) as `login`. - Removed --name and the direct-POST path entirely; no fallback. - Updated README/SDK README/migration guide: the CLI no longer prints a portable token, so CI/headless provisioning goes through Settings -> MCP instead. Tests: rewrote tokens/create's test file to mock the loopback flow like login.test.ts, replacing the old direct-POST assertions with scope- building, profile-persistence, and consent-flow coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01963CSrHDKVqeRjF1tLAnhz * docs(cli): fix stale --name references in comments after Task 2 parse.ts and drive-flag.ts still cited the now-removed `tokens create --name` flag as an example in their doc comments. Update to --drive, which is what the command actually takes post-885626a64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gEyxzBGqhJ9D64L7JvkYy * fix(cli): pagespace mcp requires an explicit token (Phase 8 task 4) Every other command's ambient fallback (`--token` flag > `PAGESPACE_TOKEN` env > stored default profile, `auth/resolve.ts`) is legitimate convenience for a human already authenticated at a prompt. `mcp`'s whole purpose is being invoked unattended by an automated MCP client, so that same fallback silently hands an agent the human's full personal account credential whenever a scoped-token config is missing or misconfigured. `createMcpHandler` now runs `hasExplicitCredential` - a pure predicate over the parsed flags/env, independent of `resolveAuth`/`resolveProfileName` (which both intentionally fall through to the "default" profile) - before building the operation registry or touching the transport at all. With no `--token`/`PAGESPACE_TOKEN`/`--profile`/`PAGESPACE_PROFILE`, it writes a message pointing at `pagespace tokens create ... --save-as-profile <name>` and exits without ever calling `createTransport`, so the stdio server never starts and no partial initialization occurs. `hasExplicitCredential` routes its token check through `resolveEnvToken` rather than a raw env read, so the legacy `PAGESPACE_AUTH_TOKEN` var still counts as explicit and `npx pagespace-mcp` keeps working unchanged. Lives in `auth/resolve.ts`, not `commands/mcp.ts`, so the command module never has to reference `PAGESPACE_TOKEN`/`PAGESPACE_PROFILE` itself, preserving the `single-auth-path.test.ts` structural guard. This is a one-command fix: no other command's auth resolution changes, and `run.ts`'s `AUTH_EXEMPT_HANDLERS` is untouched. Tests: TDD red-green - fail-closed test first (no explicit credential -> EXIT_RUNTIME_ERROR, createTransport never called, no partial stderr/stdout output), then explicit-token/profile/env/legacy-env happy paths. Existing mcp.test.ts cases updated to pass an explicit --token, matching the new required behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01448LkPLwRuoKjAFvjr6JCJ * fix(cli): gate pagespace mcp's ambient fallback in run.ts, not just mcp.ts Review of 0340219 found that hasExplicitCredential's fail-closed check lived only inside createMcpHandler, which run.ts dispatches to AFTER enforceAuth already runs for any non-exempt command (mcp was correctly left off AUTH_EXEMPT_HANDLERS). With no explicit --token/--profile but a stored default profile (from a prior `pagespace login`), that meant: resolveAuth -> kind:'profile' (ambient default) -> buildAuthProvider -> OAuthTokenProvider -> enforceAuth calls auth.getAccessToken() -> real discovery + refresh_token network exchange against the host, using the human's personal refresh token, which onTokensUpdated then rotates in the credential store -> only THEN does mcpHandler's hasExplicitCredential gate fire and refuse to start the transport Confirmed with a probe test: discoverCalls=1, refreshCalls=1, and the stored refresh token was rotated, before mcp ever refused. That's exactly the fallback Task 4's acceptance criteria says must not happen "under any circumstance" - the transport never started, but the personal credential was already used and mutated. On a network hiccup instead of success, the same path deletes the user's stored default-profile credential entirely (auth-context.ts's isAuthenticationError branch), an unrelated side effect of a misconfigured MCP client. Moves the gate to run.ts, right after route resolution and before enforceAuth, so the ambient source is never materialized into a live network call for mcp without an explicit credential. mcp.ts keeps its own copy for defense-in-depth (its unit tests call the handler directly, bypassing run.ts). Updates the one existing test whose assertion depended on the old host-leaking failure message for zero-credential mcp (now correctly replaced by the host-agnostic noExplicitCredentialMessage) to instead exercise an explicit-but-unresolvable --profile, preserving its original intent of proving PAGESPACE_API_URL flows end to end. Adds a regression test reproducing the stored-default-profile scenario directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UbeQGeTZYcusgNTodNLJPW * fix(lib): delimiter-proof canonical encoding for computeActionBindingHash An unescaped key=value&... join let a client-supplied value smuggling "&key=" collapse two different binding inputs to the same hash (e.g. {a:'x&b=y', b:''} vs {a:'x', b:'y&b='}), so a step-up grant minted for one pending request could be spent on a different one. Encode the sorted [key, value] pairs as JSON instead, which escapes delimiters inside values. Fixes review finding 1 (Critical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * fix(cli): exempt tokens create from ambient enforceAuth in run.ts Stage 3 made tokens create mint through its own browser-consent OAuth flow, but run() still enforced ambient auth first: a fresh CLI with nothing stored failed before ever reaching the consent flow, and a stored default profile got needlessly refreshed/rotated as a side effect. Add tokensCreateHandler to AUTH_EXEMPT_HANDLERS alongside login/logout/whoami, with run()-level tests through the real entry point. Fixes review finding 2 (High). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * fix(cli): treat prototype-named profiles/hosts as ordinary data in serialize.ts Profile names are user-supplied, and bare bracket//in lookups let --profile __proto__ resolve Object.prototype members: getHost returned a truthy non-credential, removeHost matched any host, and listSummaries crashed reading .refreshToken off a prototype function. Use Object.hasOwn for every existence check, and build parsed objects with Object.fromEntries so a __proto__ key from JSON becomes an own data property instead of silently setting the object's prototype (which broke round-tripping such a profile). Fixes review finding 3 (Major). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * fix(cli): reject --save-as-profile default in tokens create Nothing stopped a scoped token from being stored in the exact profile slot pagespace login uses, where a later login would silently overwrite it (or vice versa — a scope downgrade without warning). Refuse the reserved name with an actionable error; the single-drive fallback and multi-drive-requires-explicit-name logic are unchanged. Fixes review finding 4 (Major). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * fix(web): step-up token handoff via URL fragment + no-store; kill empty-string oracle Finding 5: the magic-link verify redirect placed the single-use step-up token in the Location query string (capturable by server/proxy access logs) and was the only step-up route missing Cache-Control: no-store. Hand the token back in the URL fragment instead — fragments never leave the browser — and mark every response (success, failure, redirect) no-store like the sibling routes. Finding 6: stepUpToken's z.string().min(1) made an empty-string value fail zod with a distinct field-named 400, contradicting the adjacent comment's no-oracle intent; drop .min(1) so the handler's falsy check reports the same 401 step_up_required for missing and empty alike. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * test(cli): cover runLoopbackLogin's default-profile fallback The suite only asserted the explicit profile: 'work' path; add the omitted-profile case proving the credential is stored under "default". Fixes review finding 7 (Minor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * chore(cli): align typecheck target with the ES2022 build target tsconfig.build.json already compiles at ES2022; tsc --noEmit inherited the root's es2020 and rejected Object.hasOwn (finding 3's fix). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fbZpbpFYjgwkvCQ4nMDEx * fix(cli): reject auto-derived "default" profile name in tokens create resolveTokenProfileName only checked the explicit --save-as-profile flag against DEFAULT_PROFILE_NAME. The single-drive auto-derive fallback (--drive default --role member with no --save-as-profile) resolved to "default" unchecked, silently landing a scoped token in the reserved personal-login slot. Now the check runs against whichever branch produced the final name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01534bnuttZDZRE351tsQQt2 * fix(lib): JSON-encode computeMcpTokenActionBinding's drive scope list Replaced the ad-hoc ":"/","-delimited string join with JSON-encoded sorted [id, role, customRoleId] tuples, matching the safe encoding computeActionBindingHash already uses (and warns callers to use) after this branch's earlier delimiter-collision fix. id and customRoleId are unvalidated strings at the HTTP layer for non-CLI callers, so an unescaped join could collapse two different drive-scope sets to the same action binding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01534bnuttZDZRE351tsQQt2 * fix(web): drop .min(1) on oauth/authorize's stepUpToken schema mcp-tokens/route.ts already dropped .min(1) so an empty-string stepUpToken fails the same falsy check as a missing one instead of producing a distinct zod-shaped 400. oauth/authorize/route.ts wasn't updated to match, reintroducing the empty-string oracle on this route. Now both routes report the identical 401 error shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01534bnuttZDZRE351tsQQt2 * fix(cli): guard resolveAuth's profile lookup with Object.hasOwn profiles[host]?.[profileName] was a bare bracket read. A --profile __proto__ (or PAGESPACE_PROFILE=__proto__) for a host with no profile literally named __proto__ resolved to Object.prototype (truthy for any plain object lacking that own key), producing a bogus "profile" AuthSource with no real credential fields instead of falling through to "none". Now guarded the same way serialize.ts's profile lookups already are. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01534bnuttZDZRE351tsQQt2 * test(web): update mcp-tokens step-up tests for computeMcpTokenActionBinding's new JSON encoding computeMcpTokenActionBinding (packages/lib/src/auth/mcp-token-scopes.ts) now JSON-encodes the driveScopes action-binding component instead of an ad-hoc delimiter join (94d5d45). Update the three step-up gate tests that asserted on the old string format to expect the new one. * fix(lib): add operation discriminator to computeMcpTokenActionBinding Round-3 review finding 3: the mcp-token step-up action binding covered `{ name, driveScopes }` alone, with no discriminator between minting a NEW token (POST) and widening an EXISTING one (PATCH, which reuses the `name` slot for the target tokenId). A step-up grant obtained for minting a token named identically to some real token's id, with matching drive scopes, would hash to the same binding as the grant for widening that real token — undercutting the guarantee that a grant for one mint/update request can never be spent on a different one. computeMcpTokenActionBinding now requires `op: 'mint' | 'update'`, threaded through both call sites in a later commit. Regression test proves the two ops produce different bindings for identical name + drive scopes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012R9m4F6P23DJS6QeYmvXxh * fix(cli): guard resolveAuth's host lookup with Object.hasOwn too Round-3 review finding 2: the prior round guarded the `profiles[host][profileName]` lookup with `Object.hasOwn`, but left `profiles[host]` itself as a bare bracket read one line above. A prototype-named host (`__proto__`, `constructor`, `toString`) bracket-reads to a real prototype-chain value (`Object.prototype`, the `Object` function, or `Function.prototype.toString`) rather than `undefined` — each of which owns real properties (`toString`, `name`) that `Object.hasOwn` on the *profile name* would happily find, returning that unrelated prototype member as a bogus `credential`. Not currently reachable from `run.ts` (which only ever constructs `profiles` as `{}` or a safe single-key literal), but the same invariant the rest of this codebase now treats as load-bearing. Regression test demonstrates the concrete collision (e.g. `resolveAuth({}, {}, {}, '__proto__', 'toString')` previously returned `Object.prototype.toString` as a credential) and confirms it now resolves to `none`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012R9m4F6P23DJS6QeYmvXxh * test(lib): cover step-up-service's replay-guard race branches Round-3 review finding 4: the atomic single-use guards in step-up-service.ts — `consumedChallenge.length === 0`, `counterUpdate.length === 0`, and `consumed.length === 0` — are the actual TOCTOU/replay defense (the "someone else already consumed this between decide and consume" race), but had no test exercising them at the service layer. The existing mock always returns a row from `.returning()`, so these branches were only covered by implication via the pure decision layer's tests, not directly. Adds three tests that override the mock to return an empty array on the relevant `.update().set().where().returning()` call, simulating a concurrent request winning the race, and asserting the correct STEP_UP_INVALID / STEP_UP_REQUIRED failure. Also adds an integration-level regression test proving a grant minted for `computeMcpTokenActionBinding({ op: 'mint', ... })` is rejected by `consumeStepUpGrant` when presented against the `op: 'update'` binding for the identical name + drive scopes (round-3 finding 3, service-layer proof to complement the pure-function unit test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012R9m4F6P23DJS6QeYmvXxh * fix(web): close mcp-tokens PATCH step-up gate gaps, dedup the gate Round-3 review findings 1 and 5: 1. PATCH /api/auth/mcp-tokens/[tokenId] still had `z.string().min(1)` on `stepUpToken`, reopening the empty-string-vs-missing oracle that an earlier round closed on POST /api/auth/mcp-tokens and POST /api/oauth/authorize. An empty string now fails the same falsy runtime check as a missing field, with a regression test asserting both produce the identical error shape. 5. POST and PATCH each duplicated the ~15-line step-up gate block (missing-token check, audit log, consumeStepUpGrant call, invalid-result check, audit log) nearly verbatim — exactly how finding 1 happened, since only one copy got the `.min(1)` fix. Extracted into a shared `requireStepUpGrant` helper (mcp-tokens/step-up-gate.ts) that both routes now call, so a future fix to this logic can't land on only one copy again. Both routes also now pass the `op: 'mint'` / `op: 'update'` discriminator into `computeMcpTokenActionBinding` (prior commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012R9m4F6P23DJS6QeYmvXxh * docs: document that tokens create no longer supports --json Round-3 review item 6: confirmed `tokens create --json` was silently dropped in the browser-consent rewrite (885626a), not carried forward by accident, then left undocumented at the blanket "every command supports --json" claim. It's an intentional consequence of that rewrite — the command now blocks on an interactive browser consent screen and no longer has a portable token to print in JSON at all — so `--json` is simply ignored rather than erroring. The per-command flag list in README.md already omitted `[--json]` for `tokens create` (unlike `tokens list [--json]`), but the blanket "every command supports --json" line contradicted that. Carves out the exception there and adds a note to the existing CHANGELOG entry for the browser-consent security fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012R9m4F6P23DJS6QeYmvXxh * fix(web): check scope authority before burning the OAuth step-up grant consumeStepUpGrant single-use-burns the step-up token regardless of what happens next. It ran before checkGrantAuthority's scope-cap check, so a request that only failed the unrelated scope check still destroyed a legitimate, just-completed WebAuthn/magic-link ceremony — forcing a full step-up redo for no security reason. Reorder so the scope-cap check runs first and rejects early with no step-up call at all; only burn the grant once every other check has passed. Add a test proving a scope-cap-rejected request never calls consumeStepUpGrant, and that the grant is still valid for a subsequent, correctly-scoped retry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U4va9g21Y9wRCMkHZ8pETb * fix(web): reset consent step-up state on any WebAuthn cancellation stepUpStatus only reset out of 'in_progress' for the no_passkey fallback path. If the user cancelled or dismissed the WebAuthn prompt for any other reason, the Allow button stayed stuck on "Confirming…" indefinitely and the stale token state was never cleared. Reset stepUpStatus to 'idle' and clear stepUpToken on any ceremony failure, not just no_passkey, so a retry starts a genuinely fresh ceremony. Add a regression test covering a generic cancellation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U4va9g21Y9wRCMkHZ8pETb * test(cli): cover the remaining tokens-create consent-flow outcomes createTokensCreateHandler's switch over runLoopbackLogin's result has 8 branches; only success and discovery_failed had handler-level tests. The underlying state machine is well-covered in loopback-flow.test.ts, but this command's own dispatch/error-surfacing wasn't, on a command this PR just made security-critical. Add handler-level tests for the remaining 6 branches (access_denied, timeout, state_mismatch, authorize_error, token_exchange_failed, port_bind_failed), each asserting the exit code, user-facing message, and that no credential is stored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U4va9g21Y9wRCMkHZ8pETb * fix(cli): make removeHost use the same __proto__-safe assignment as upsertHost removeHost rebuilt the hosts map with a bare `hosts[host] = value` bracket assignment while every other writer in this file (upsertHost, the parse/ migrate constructors) builds mutated objects via a computed-key object literal specifically to avoid triggering Object.prototype's `__proto__` setter. Not exploitable at this exact call site today (reaching it requires `Object.hasOwn(file.hosts, host)`, which in every current constructor implies `__proto__` is already an ordinary enumerable own property, so `{ ...file.hosts }` always carries it forward as data) — but it was correctness-by-accident, not by design. Switch removeHost to the identical `{ ...file.hosts, [host]: value }` form upsertHost already uses. Adds a regression test that reconstructs the one case where the old code was reachable (a non-enumerable own "__proto__" key, built with Object.defineProperty so `{ ...x }` can't carry it forward) — confirmed red against the prior bracket-assignment code, green after the fix. * fix(cli): reject NUL bytes in keychainAccountKey host/profile at the boundary keychainAccountKey/parseKeychainAccountKey join host and profile with a raw NUL byte, unescaped. Two distinct (host, profile) pairs can encode to the same account key — e.g. host="A\0B", profile="C" and host="A", profile="B\0C" both produce "A\0B\0C" — so one set() would silently overwrite the other's keychain entry. This was previously low-priority because CLI argv/env can never carry an embedded NUL byte, but keychainAccountKey and CredentialStore are re-exported as public library API from index.ts, so any non-argv caller (a test, an embedder, a future library consumer) could construct the collision directly. Throw at the keychainAccountKey boundary when host or profile contains the separator, closing the collision for every caller through this single choke point. Adds tests proving both previously-colliding pairs are now rejected outright rather than silently overwriting one another. * feat(web): connected-apps visibility + revocation in Settings > Account Adds a session-authenticated "Connected Apps" surface: lists every active OAuth grant (client name, human-readable scope, created date) for the current user, and lets them revoke any grant from the web instead of only from the machine holding the credential. - packages/lib: isGrantOwnedByUser (pure ownership predicate, collapses not-found and not-yours to the same false) and describeGrantScopes (pure scope-to-text summary reusing describeScopeForConsent) in auth/oauth/grant-ownership.ts and auth/oauth/grant-scope-summary.ts. - apps/web/src/lib/repositories/oauth-repository.ts: listActiveOAuthGrantsForUser, findOAuthGrantById (lookup by id, excludes already-revoked rows), and revokeOAuthGrantFamily (thin wrapper reusing the existing revokeTokenFamily helper RFC 7009 revocation already uses). - GET /api/account/oauth-grants: session-only listing endpoint, resolves drive/custom-role names for scope descriptions. - DELETE /api/account/oauth-grants/[grantId]: session-only, step-up gated (reuses requireStepUpGrant from the mcp-tokens step-up gate) revoke-by-id, scoped to rows owned by the authenticated user with no oracle between "not found" and "not yours". - ConnectedAppsList: lists grants and revokes via the same WebAuthn/magic-link step-up ceremony pattern the OAuth consent screen uses, reconciling the magic-link fallback's redirect-back through sessionStorage + the URL hash. - Wired into Settings > Account below the existing device-management section. Tests prove: the ownership check collapses foreign and missing grants to the identical 404 (written before the happy path, per this stage's TDD requirement), the step-up gate rejects a missing/invalid/empty stepUpToken identically, and an end-to-end repository test that revoking a grant makes a subsequent refresh_token grant with that token fail. Stage 5 of 6 in the Phase 8 credential-minting-security-correction pipeline (pu/phase8-credential-security). Stage 6 (docs) is next. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZHETKVidWGSHuvxbsi6n1 * fix(web): reset revoke button state when magic-link fallback request fails The catch block in ConnectedAppsList's handleConfirmRevoke called requestMagicLinkStepUp without its own try/catch. If that network call failed, the exception went unhandled and the Revoke button got stuck disabled at "Revoking…" forever, mirroring the bug ConsentActions.tsx already guards against for its own step-up ceremony. Also defer writing the pending sessionStorage key until the email send actually succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHPFY1uRXXdbSKY5FG79pE * docs(cli,web): Phase 8 Task 6 — agent-access doc, scope messaging, help regrouping - New packages/cli/docs/agent-access.md: states plainly that a scoped tokens-create credential limits what a leaked/misused credential can do, not who else on the same machine can use it — a process with real shell access reads whatever its OS user can read, credential store included. Recommends a dedicated OS user/container/VM plus PAGESPACE_TOKEN as the actual isolation boundary for any agent with real bash/shell access. - README and the Settings > MCP page now say explicitly that `login` is for you, personally, and `tokens create --drive <id> --save-as-profile agent` is for an agent — replacing copy that recommended `login` for local MCP setups. - `pagespace login` (and `login --device`) now print the granted scope on success, bringing them to parity with `whoami`. - `pagespace help` is grouped by resource (Auth, Drives, Pages, Search, Tasks, Agents, Tokens, MCP, Other) with one runnable example per group, via a new pure `groupHelpCommands` function, replacing the flat list. - Consolidates the CHANGELOG's Phase 8 Added/Changed sections to cover this stage without duplicating Stage 4/5's existing entries. * feat(web): wire step-up into MCP token create/edit UI (Phase 8 Task 7) MCPSettingsView's create/edit MCP token dialogs never sent a stepUpToken, so Task 1's server-side gate on POST/PATCH /api/auth/mcp-tokens would have returned an unconditional 401 on the existing "create token"/"edit token" buttons — a regression, not a deferred gap. - Extract the WebAuthn-attempt-then-magic-link-fallback ceremony (previously duplicated in ConsentActions.tsx and ConnectedAppsList.tsx) into a shared attemptStepUp() in apps/web/src/lib/auth/step-up-ceremony.ts. Both existing components refactored to use it with zero behavior change (their test suites pass unmodified). - Add mcp-token-step-up.ts: pure buildMintActionBinding/buildUpdateActionBinding functions that reuse the server's own computeMcpTokenActionBinding (packages/lib/src/auth/mcp-token-scopes.ts) so the client can never drift from the exact binding shape the step-up gate checks. - Wire the ceremony into MCPSettingsView's createToken/updateTokenScopes, including sessionStorage-backed resume after a magic-link redirect (mirroring ConnectedAppsList's revoke-resume pattern) and "Confirming…" / "Check your email…" UI states in both dialogs. Closes the last known gap in Phase 8 (PR #1878) before it's mergeable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EyDVT3y3WEk58Tvf4qNmsN * fix(cli): move NUL-byte account-key validation to a choke point in CompositeCredentialStore keychainAccountKey's NUL-byte throw (round 5's fix) was caught by CompositeCredentialStore's generic keychain-error fallback and silently degraded to the file store, which had no equivalent check — so a malformed (host, profile) pair still got written, just via the file path instead of being rejected. Extract the check into assertValidAccountKeyInputs (keychain.ts) and call it at the top of CompositeCredentialStore's get/set/delete, before either backend is attempted, so both paths reject identically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgCQTz8B4KeEtRZiBNCunT * fix(web): gate MCP token creation against reentrancy, incl. Enter-key createToken() had no guard against being invoked twice concurrently, and the token-name input's Enter-key handler called it unconditionally regardless of in-flight state — so a second Enter press while a WebAuthn ceremony was already running fired a second concurrent ceremony/mint request. The Create button's disabled attribute never protected the Enter path since Enter never checked it. Guard at the top of createToken() itself (matching QuickCreatePalette's established `if (isCreating) return;` pattern), which covers both the button and the Enter handler with one check. Also adds coverage for the generic-WebAuthn-cancellation reset path for both createToken() and updateTokenScopes() (already correct in code, previously untested). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgCQTz8B4KeEtRZiBNCunT * fix(web): finish the step-up-ceremony extraction — one shared hash/no-passkey implementation readStepUpTokenFromHash/stripStepUpTokenFromHash/isNoPasskeyError were triplicated: consent-step-up.ts and step-up-hash.ts each carried their own copy alongside the canonical one in lib/auth/step-up-ceremony.ts (which only MCPSettingsView actually used). Three copies of the same reset-on-cancellation-adjacent logic meant a fix only had to be verified against whichever copy happened to be touched — which is how MCPSettingsView's cancellation-reset path went untested despite ConsentActions having an equivalent test since round 4. ConsentActions.tsx and ConnectedAppsList.tsx now import the hash helpers from step-up-ceremony.ts, matching MCPSettingsView. Deletes the now-redundant step-up-hash.ts module and test file; trims consent-step-up.ts down to buildConsentActionBinding (still unique to the consent screen's binding shape). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgCQTz8B4KeEtRZiBNCunT * fix(cli): align help's summary column globally, not per group commandLines() computed its padEnd width from only the commands in the group being rendered, so each resource group's summary column started at a different offset — e.g. Auth's short names aligned differently than Tokens' longer ones, breaking the flat list's previously-consistent columns once Task 6 introduced grouping. Compute the width once across every command in every group. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgCQTz8B4KeEtRZiBNCunT * fix(cli): print the actually-granted login scope, not the requested constant login.ts/login-device.ts printed DEFAULT_LOGIN_SCOPE — the scope *requested* — as if it were confirmation of what was granted. Per RFC 6749 §5.1 a server may return a narrower scope than requested; whoami already accounts for this by preferring the server's live scope over the stored/assumed one, but the login success message didn't. The existing test fixture's granted scope happened to equal DEFAULT_LOGIN_SCOPE, so the mismatch went uncaught. Thread the token exchange's actual `scope` through LoopbackLoginResult/DeviceLoginResult's success variant and print that instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgCQTz8B4KeEtRZiBNCunT * fix(web,cli): round-6 remediation - updateTokenScopes reentrancy, NUL-byte test encoding updateTokenScopes() had no reentrancy guard, unlike createToken() (round 6 gave createToken() one but round 5 had flagged both as sharing the bug). Apply the same editingScopes/editStepUpStatus guard, and cover it with a test mirroring the createToken double-Enter regression test. store.test.ts's NUL-byte tests embedded 8 raw null-byte control characters directly in the source instead of source-level unicode escapes, making the file register as binary to git/GitHub (every future diff would show Bin instead of a reviewable line diff). Replaced each with the standard escape sequence; the runtime behavior under test (host/profile NUL rejection) is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TayQ2DpB6dsosVsGQeqSiN * fix(cli,web): round-7 remediation - device-login profile isolation, revoke-dialog close, keychain parse-vs-outage scoping - login-device.ts silently ignored --profile/PAGESPACE_PROFILE, so a device-flow login always wrote to the "default" profile regardless of what was asked, defeating Phase 8 task 3's profile isolation. Now resolves the profile the same way login.ts does and threads it through DeviceLoginDeps/device-flow.ts (mirroring loopback-flow.ts). - ConnectedAppsList's handleConfirmRevoke now explicitly nulls confirmingGrant in the awaiting_email branch, matching the intent documented for the equivalent MCPSettingsView flow, instead of relying implicitly on Radix's default close-on-click behavior for AlertDialogAction. - CompositeCredentialStore.get()/list() no longer fold a malformed keychain entry's parse failure into the same catch that decides whether the whole store degrades to the file-store fallback. get() now throws a distinct, accurately-worded error for that one lookup; list() skips the bad entry with a stderr warning and keeps returning every other entry, without flipping `degraded` for the rest of the process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FBJ2HGXZg6YKSDriD7fyqn * fix(web): suppress false-positive CodeQL clear-text-storage alert sessionStorage.setItem(PENDING_REVOKE_STORAGE_KEY, grantId) stores an opaque OAuth grant row id used to resume revocation after a magic-link redirect, not a credential or token. CodeQL's js/clear-text-storage-of- sensitive-data flags it purely because it's derived from useOAuthGrants. Suppress with an inline codeql[] comment (existing repo convention, see apps/web/src/app/api/user/integrations/callback/route.ts) explaining why. * docs(review): record round-8 convergence pass on Phase 8 PR #1878 Triaged the 3 remaining GitHub review threads (2 stale CodeRabbit comments, 1 genuine CodeQL false positive) against current HEAD after merging master, and cross-checked the PageSpace epic board's tracked findings against source. * fix(web): dismiss CodeQL alert #250 as false positive, drop dead suppression comment The inline // codeql[js/clear-text-storage-of-sensitive-data] comment added in b8b6592 did not actually suppress the alert -- the check still failed on the next analysis run. This repo's CodeQL setup evidently doesn't honor inline suppression comments (unlike the codeql-cli's local suppression feature). Dismissed alert #250 directly via the code-scanning API with an explicit false-positive reason/comment instead, and replaced the non-functional directive with a plain explanatory comment. * fix(web): reconcile Phase 9 manage-keys carve-out tests with Phase 8 step-up gate Master's Phase 9 foundation (8f1e243) added manage-keys-scope.test.ts expecting a manage_keys-only OAuth credential to mint (POST) or escalate (PATCH) mcp_* tokens directly via bearer auth. Phase 8, on this branch, independently locked POST/PATCH down to session-only + step-up (AUTH_OPTIONS_WRITE/AUTH_OPTIONS_PATCH allow only 'session', matching DELETE and GET which correctly keep 'oauth' since revocation/listing never escalate). The merge combined both without reconciling: the tests assumed a bearer-OAuth mint path that Phase 8 deliberately closed. Minting/escalating is exactly the operation Phase 8's step-up gate exists to protect, and it applies uniformly regardless of credential shape -- a manage_keys OAuth bearer token is itself an ambient secret with no special exemption. The epic's own scope note is explicit that Phase 9's key-management flows call through Phase 8's existing session + browser-consent minting path, not a raw bearer POST. Updated both test files' POST/PATCH assertions from expecting 200/403 (mint succeeds / scope-rejected) to 401 step_up_required (blocked before any scope check runs), matching the actual, intentional route behavior. GET/DELETE assertions (which already exercise the real oauth+manage-keys carve-out) are unchanged. * docs(review): record round-9 CI-failure root cause (Phase 8/9 merge-boundary conflict) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Changes
New Hook:
useMobileKeyboarddismiss()function to programmatically close keyboardIME Composition Handling
Keyboard Dismiss Behaviors
Layout Positioning
CSS Utilities
.keyboard-animateand.keyboard-aware-bottomutilitiesNavigation Improvements
touch-manipulationandonTouchEndto PageTreeItem linksTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.