fix(oauth): harden provider and token lifecycles - #3234
Conversation
a1389d6 to
7e1178f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1389d6dfd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
21c5e71 to
f562840
Compare
09f9fdc to
2a2ac4e
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR hardens Veryfront’s OAuth and token-storage surfaces by tightening validation at public construction boundaries, strengthening state and redirect bindings, and improving retry/abort lifecycles so OAuth/token refresh behavior fails closed under misconfiguration and concurrency.
Changes:
- Add stricter OAuth URL, redirect, scope, state, and token validation (plus dispatcher support for shared callback routes) and update OAuth provider configs/manifests to match the hardened runtime contracts.
- Harden Veryfront token storage adapters and API transport with bounded retry policies, request cancellation, response size limits, and safer token-store lifecycle semantics.
- Improve retry/abort primitives to compose caller cancellation with per-attempt timeouts and to cancel backoff delays safely.
Verification
- Not run in this review environment.
- Recommended:
deno task typecheck,deno lint,deno test --no-check --allow-all(and any focused suites mentioned in the PR description).
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/abort.ts | Adds helpers to normalize abort reasons and await abortable work reliably. |
| src/platform/adapters/veryfront-api-transport.test.ts | Exercises retry bounds, cancellation composition, URL authority checks, and response size redaction/limits. |
| src/platform/adapters/token/veryfront/types.ts | Tightens token adapter config validation, adds retry/timeout normalization, and introduces key/prefix/value assertions. |
| src/platform/adapters/token/veryfront/types.test.ts | Tests retry/timeout boundaries and rejects blank credentials/selectors. |
| src/platform/adapters/token/veryfront/memory-adapter.ts | Validates key/value inputs and clears in-memory tokens on dispose. |
| src/platform/adapters/token/veryfront/memory-adapter.test.ts | Adds tests for adapter isolation, empty-key rejection, and dispose clearing. |
| src/platform/adapters/token/veryfront/api-client.ts | Bounds/reads response bodies, validates JSON payloads, validates identifiers, and supports cancellation. |
| src/platform/adapters/token/veryfront/api-client.test.ts | Tests payload validation, retry enforcement, base-path behavior, and body size cancellation. |
| src/platform/adapters/token/veryfront/adapter.ts | Coalesces concurrent initialization and prevents disposed adapters from being resurrected by late async work. |
| src/platform/adapters/token/veryfront/adapter.test.ts | Adds construction retry-boundary checks and initialization concurrency lifecycle tests. |
| src/platform/adapters/token/integration.ts | Coalesces singleton creation and makes reset invalidate in-flight creation; uses shared env getter. |
| src/platform/adapters/token/integration.test.ts | Tests singleton creation coalescing and reset invalidation. |
| src/platform/adapters/token/factory.ts | Ensures adapters dispose on initialization failure via a shared initializer helper. |
| src/platform/adapters/token/factory.test.ts | Adds coverage for invalid retry configs and defaulting behavior. |
| src/oauth/url-validation.ts | Introduces strict OAuth URL text and endpoint/redirect validation helpers. |
| src/oauth/url-utils.ts | Centralizes OAuth response headers, origin resolution, callback URL construction, and safe completion redirects. |
| src/oauth/types.ts | Extends token store contracts with optional revision/lock capabilities and introduces RefreshCapableTokenStore. |
| src/oauth/token-utils.ts | Adds safe token snapshot/token normalization without invoking accessors, and refresh-capability guards. |
| src/oauth/token-store/memory.ts | Implements revisioned CAS + refresh locking, bounds state lifetimes, and hardens keying/normalization. |
| src/oauth/token-store/memory.test.ts | Expands coverage for revisions, locking semantics, state validation, detachment, and bounds. |
| src/oauth/token-store/index.ts | Re-exports new token snapshot/refresh-capable types. |
| src/oauth/text-validation.ts | Adds ASCII control-character detection used by multiple validators. |
| src/oauth/state-utils.ts | Adds canonical state normalization, metadata snapshotting, and strict state binding helpers. |
| src/oauth/state-metadata.ts | Snapshots transaction metadata to bounded, data-only JSON. |
| src/oauth/scope-utils.ts | Adds bounded scope token and scope set normalization consistent with RFC wire formats. |
| src/oauth/schemas/oauth.schema.ts | Hardens public schemas (URLs, identifiers, headers/params, PKCE, bounds, and invariants). |
| src/oauth/schemas/oauth.schema.test.ts | Tests schema acceptance/rejection and ensures built-in providers satisfy the hardened schema. |
| src/oauth/schemas/index.ts | Re-exports additional schema getters for validated construction boundaries. |
| src/oauth/providers/protocols.test.ts | Verifies provider-specific wire contracts (form vs JSON, basic auth, PKCE usage, scope separator). |
| src/oauth/providers/microsoft.ts | Updates Microsoft service configs (PKCE mode, scopes, and config isolation). |
| src/oauth/providers/microsoft.test.ts | Ensures runtime scopes match connector manifests for Microsoft services. |
| src/oauth/providers/index.ts | Exposes new services/types (including docs-google) and refresh-capable token-store types. |
| src/oauth/providers/google.ts | Adds Google Docs service, updates scopes, and isolates nested auth param maps. |
| src/oauth/providers/google.test.ts | Verifies Google Docs exposure and connector scope alignment. |
| src/oauth/providers/config-isolation.test.ts | Ensures nested additionalAuthParams objects are not shared across services. |
| src/oauth/providers/common.ts | Updates common provider configs for PKCE/headers/scope separators and endpoint changes. |
| src/oauth/providers/common.test.ts | Validates provider-specific protocol declarations, fail-closed runtime support, and connector scope alignment. |
| src/oauth/providers/atlassian.ts | Aligns Atlassian configs with JSON token exchange and other contract flags; adjusts Bitbucket. |
| src/oauth/providers/atlassian.test.ts | Tests Atlassian JSON token protocol, callback scaffolds, and connector scope alignment. |
| src/oauth/limits.ts | Introduces centralized OAuth bounds for lengths, timeouts, and response sizes. |
| src/oauth/index.ts | Updates public module exports and examples to reflect hardened handler/token-store requirements. |
| src/oauth/index.test.ts | Locks down the veryfront/oauth runtime export surface. |
| src/oauth/handlers/token-store-policy.ts | Enforces explicit token-store configuration outside dev/test (fail closed). |
| src/oauth/handlers/init-handler.ts | Hardens init/status/disconnect handlers (method checks, redirect binding, store policy, safe JSON responses). |
| src/oauth/handlers/index.ts | Re-exports callback dispatcher and updated handler option types. |
| src/oauth/handlers/callback-handler.ts | Hardens callback handling (parameter bounds, state binding, PKCE enforcement, dispatcher support, safe redirects). |
| src/oauth/handlers/callback-dispatcher.test.ts | Tests dispatcher allowlisting, shared redirect binding, PKCE requirements, parameter ambiguity limits, and concurrency. |
| src/oauth/data-properties.ts | Adds safe snapshotting of caller-owned config without invoking accessors. |
| src/oauth/core-runtime.test.ts | Tests cross-runtime validation and token-store detachment/connected semantics. |
| src/oauth/config-validation.ts | Implements reserved parameter/header detection and safe snapshotting of config records. |
| src/integrations/_data.ts | Updates generated connector metadata (scopes, token auth methods, setup guides, env vars). |
| src/integrations/_data.test.ts | Adds assertions for updated connector scopes and setup guide correctness. |
| src/errors/error-handlers.ts | Adds abort-signal support to retryWithBackoff, composes signals, and cancels backoff sleeps. |
| src/errors/error-handlers.test.ts | Tests aborting pending backoffs and ensures timeout reasons use the captured native Error constructor. |
| cli/templates/integrations/outlook/connector.json | Aligns Outlook scopes and tool description copy with runtime changes. |
| cli/templates/integrations/jira/connector.json | Adds optional Jira cloud-id env var and aligns auth contract fields. |
| cli/templates/integrations/docs-google/connector.json | Removes deprecated scope and replaces long setupGuide string with structured steps/notes. |
| cli/templates/integrations/confluence/connector.json | Aligns Confluence token auth method and adds optional cloud-id env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a2ac4e567
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/oauth/handlers/init-handler.ts:373
- The disconnect handler clears tokens from the configured TokenStore, which may be durable/remote. The success message currently says "locally stored", which is misleading for non-memory stores.
return createOAuthJsonResponse({
success: true,
message: `Removed locally stored ${service.displayName} OAuth tokens`,
});
src/oauth/token-store/memory.ts:313
- getConnectedServices() percent-encodes the serviceId/userId components, but the doc comment still claims it returns raw
${serviceId}:${userId}. Update the comment so callers/debug tooling understand the encoding.
revokeToken posted only the token to the revocation endpoint, with no client authentication. RFC 7009 section 2.1 requires confidential clients to authenticate on revocation, so providers reject the unauthenticated call and revocation silently no-ops. Reuse the client-credential path exchangeToken already takes: Basic auth header for useBasicAuth providers, client_id/client_secret body params otherwise. Return false without a request when credentials are missing.
The transport rewrite dropped three behaviors the bounded-response tests in operations.test.ts pin: - maxResponseBytes budgets only the non-value bytes, but it was passed as the hard document ceiling, so a document whose selected string used its full worst-case escape expansion was rejected. Derive the ceiling with maximumJsonStringDocumentBytes and pass the non-value budget separately. - Bounded JSON field options were only validated once a response arrived, so a malformed selector reached the network and was retried. Validate before any I/O, alongside maxResponseBytes. - The invalid-content error was reworded, breaking its documented message.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/oauth/handlers/callback-handler.ts:335
- When the token response omits
scope, the callback handler synthesizes it by joiningstoredState.scopeswith a hard-coded space. This breaks provider-specific scope serialization for services configured withscopeSeparator: ","(for example Slack/Linear), and the synthesized value is then persisted and carried forward during refresh (OAuthServicecopies priortokens.scopewhen refresh responses omit it).
const tokens = {
...result.tokens,
...(result.tokens.scope === undefined && storedState.scopes.length > 0
? { scope: storedState.scopes.join(" ") }
: {}),
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49717e5a29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 155 out of 156 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/oauth/providers/microsoft.ts:56
- Spelling: "share point" should be "SharePoint" (product name).
src/utils/abort.ts:5 - This file duplicates
createAbortErrorandthrowIfAbortedfromsrc/agent/runtime/error-utils.tsverbatim. Having two identical implementations risks the helpers drifting and makes it unclear which one is canonical. Consider moving the shared helpers to one module and re-exporting from the other to keep a single source of truth.
src/oauth/url-utils.ts:20 isExplicitLocalOAuthEnvironment()allocates a newSeton every call, which is unnecessary overhead on a hot path (handler construction and potentially per-request validation). This can be a simple pair of string comparisons without any allocation.
src/oauth/providers/microsoft.ts:72- Spelling: "one drive" should be "OneDrive" (product name).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 156 out of 157 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/platform/adapters/token/veryfront/memory-adapter.ts:56
get/set/delete/listcan throw synchronously (viaassertTokenStorageKey/assertTokenStoragePrefix) before returning a Promise. That makes the async adapter contract harder to consume safely (for example,adapter.get(key).catch(...)will never run becauseget()throws). Make these methodsasyncso validation failures become Promise rejections and keep behavior consistent with the other async adapters.
src/utils/abort.ts:5- This module duplicates
createAbortError/throwIfAbortedthat already exist insrc/agent/runtime/error-utils.ts(same behavior and tests). Having two copies risks subtle drift (for example if abort normalization changes in one place). Consider consolidating on a single shared implementation and importing/re-exporting it from the other location.
src/oauth/url-utils.ts:20 isExplicitLocalOAuthEnvironmentallocates a newSeton every call. This is a hot-path helper (used during handler construction / request handling) and can be implemented without allocations.
The docs-google connector requested documents.readonly alongside documents, which already includes read access, so the narrower scope granted nothing. Google's OAuth verification review flagged the request as failing least privilege. The template docs-client also still listed the auth/docs scope that #3234 removed from the connector contract; align it with connector.json. Docs connect grants now request documents + drive.readonly only.
Summary
Diffstat
61 files, 9,183 additions, 835 deletions (10,018 changed lines).
Dependency
Stacked on #3233. Merge #3233 first. This PR uses codex/platform-runtime-contracts-20260802 as its base.
Validation
No root Deno config, lockfile, generated integration registry, or generated runtime artifacts are included.