docs(planning): SaaS connectors & outbound integration hardening plan - #669
Open
ginccc wants to merge 2 commits into
Open
docs(planning): SaaS connectors & outbound integration hardening plan#669ginccc wants to merge 2 commits into
ginccc wants to merge 2 commits into
Conversation
Design record for connecting agents to third-party SaaS (Jira, Amplitude,
Google Drive) over REST/MCP/A2A with real auth, including per-user OAuth.
Establishes: eleven outbound paths with five independent credential
implementations, zero OAuth machinery, no per-user credential dimension.
Plans six phases: exposure hardening, tool-result governance, a unifying
ConnectionConfiguration resource with ${connection:} references and
SERVICE|PER_USER binding, stdio-via-sidecar, OAuth client-credentials +
RFC 9728 MCP discovery, and per-user authorization-code + PKCE.
Key guards: PER_USER connections refuse to deploy under
authorization.enabled=false (unverifiable principal = token theft);
OAuth connections require an active vault; connection resolution stays
out of SecretResolver to preserve the grant-enforcement design's caching
assumptions.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a changelog entry and a detailed plan for SaaS connectors and outbound integration hardening. It defines connection configuration, encrypted grants, OAuth flows, MCP transport work, security controls, phased delivery, metrics, tests, and unresolved design questions. ChangesSaaS connectors and outbound hardening
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
planning/saas-connectors-plan.md (2)
575-582: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftQualify the sidecar isolation claim.
A sidecar still executes the MCP server binary and communicates with EDDI over a network channel. Container separation reduces the blast radius; it does not eliminate process-execution or supply-chain risk.
Document image provenance, non-root execution, resource limits, network policy, and bridge authentication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@planning/saas-connectors-plan.md` around lines 575 - 582, Qualify the isolation claims in the “Phase 3a — sidecar bridge” section to state that container separation reduces blast radius but does not eliminate process-execution or supply-chain risks. Document requirements for image provenance, non-root execution, resource limits, network policy, and authentication between EDDI and the bridge.
734-745: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep metric tags bounded.
connectionandserverare user-configurable. Raw connection names and server URLs can create unbounded Micrometer series and expose tenant or provider details.Use bounded tags such as auth type, binding, source, and outcome. Keep specific identifiers in sanitized logs or exemplars.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@planning/saas-connectors-plan.md` around lines 734 - 745, Update the metric definitions in the observability metrics table to remove user-configurable connection names and server URLs from tags. Use only bounded categorical values such as auth type, binding, source, and outcome for connection and MCP metrics, while retaining specific identifiers only in sanitized logs or exemplars.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 19: Update the phase-count wording in the “Plan shape” entry to match the
listed phases 0 through 6: either call them seven phases or explicitly exclude
the deferred Phase 6 from the six delivery phases.
In `@planning/saas-connectors-plan.md`:
- Around line 468-480: Update the OAuth callback flow described in the state
validation/exchange step to atomically claim the persisted state with a
conditional database update requiring consumedAt IS NULL before exchanging the
code. Continue only when the update claims exactly one row; reject already
consumed or expired states, and retain the single-use guarantee under concurrent
callbacks.
- Around line 428-443: Update the “single-flight per grant, with optimistic
locking” design to acquire a cross-replica refresh lease or atomic
refresh_in_progress claim before calling the token endpoint. Ensure only the
claimant refreshes, while other replicas wait and re-read the resulting token;
retain the existing version CAS as the final write guard and preserve the stated
failure semantics.
- Around line 327-355: Update OAuthConfig validation and the baseUrlAllowlist
documentation to use one canonical origin format, including schemes as required.
Validate tokenUrl, authorizationUrl, and discoveryUrl separately against the
trusted provider or authentication-endpoint allowlist, while keeping API target
validation based on baseUrlAllowlist.
- Around line 759-790: Extend the behavioral-test checklist with coverage for
the declared storage and authorization boundaries: verify grants are excluded
from exports, connection references are traversed by VaultGrantChecker,
connection and grant writes cannot use operator scope, and tenant isolation
holds for both Mongo and Postgres. Add one test for each boundary before marking
Phase 2 and Phase 5 complete.
- Around line 507-519: Update the ConnectionStartupGuard validation to parse
publicBaseUrl and normalize it before constructing the redirect URI. Require a
valid HTTPS origin with an authority and no userinfo, path, query, or fragment
(except an allowable trailing root slash), while preserving the dev/test
behavior and fail-closed startup errors; use the normalized origin when
appending /connections/callback.
- Around line 115-120: Update the connector inventory and the referenced MCP,
A2A, and httpcalls flows so every connection or discovery request uses
SafeHttpClient, including RFC 9728 resource_metadata retrieval. Add URL
validation, safe redirect handling, and cross-origin credential stripping;
restrict discovered metadata to the configured server or trusted allowlist, and
validate the RFC 9728 resource value before consuming it. Apply the same
requirements to the related sections at the other referenced locations.
- Around line 323-345: Define StaticAuth so every credential-bearing field uses
a ${vault:...} reference rather than an inline secret, and update
OAuthConfig.extraAuthParams to represent only non-secret protocol parameters.
Ensure connection validation rejects secret-bearing extra parameters and
preserves C4, export scrubbing, and VaultGrantChecker enforcement.
- Around line 623-631: Remove the credential-bearing GET compatibility forms for
discover-tools and discover-endpoints in the discovery endpoints and Manager
callers. Retain only a credential-free migration response, or remove the GET
routes entirely when the POST APIs are released; do not accept or process
apiKey/apiAuth query parameters because rejection occurs after they have already
entered URL-based logging and tracing.
- Around line 642-655: Update the C6 operator write-scope exclusion coverage for
connection capabilities to include both ConnectionConfiguration writes and the
Phase 5 authorization, connect, and disconnect grant-lifecycle routes. Add an
authorization test for each excluded route, verifying operators cannot invoke
them through write scope.
- Around line 453-460: Update the service-account client_credentials plan
section to specify grant_type=client_credentials, the supported
client-authentication method, and application/x-www-form-urlencoded request
encoding. Add behavioral tests covering the complete token request contract.
- Around line 619-631: Add a dedicated /secretstore/* authorization path policy
requiring eddi-admin, and update the startup validation to reject
authorization-disabled launches when the secret store endpoint is reachable,
unless an explicit opt-out is provided. Implement this before Phase 2, using the
existing startup-guard and path-policy patterns rather than relying on
IRestSecretStore’s `@RolesAllowed` annotation or the catch-all policy.
---
Nitpick comments:
In `@planning/saas-connectors-plan.md`:
- Around line 575-582: Qualify the isolation claims in the “Phase 3a — sidecar
bridge” section to state that container separation reduces blast radius but does
not eliminate process-execution or supply-chain risks. Document requirements for
image provenance, non-root execution, resource limits, network policy, and
authentication between EDDI and the bridge.
- Around line 734-745: Update the metric definitions in the observability
metrics table to remove user-configurable connection names and server URLs from
tags. Use only bounded categorical values such as auth type, binding, source,
and outcome for connection and MCP metrics, while retaining specific identifiers
only in sanitized logs or exemplars.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7826954a-a111-44f2-9483-c8262ffc006c
📒 Files selected for processing (2)
docs/changelog.mdplanning/saas-connectors-plan.md
…es, tokenUrl allowlisting CodeRabbit review on #669: 14 findings, all valid, all applied. Three changed the design rather than the prose. /secretstore has no path-specific policy, so it falls to the catch-all authenticated policy — and DisabledAuthController disables that AND @RolesAllowed under the shipped authorization.enabled=false. Vault writes, DEK rotation and reset are unauthenticated out of the box. The draft flagged /mcp and missed the credential store itself. New Phase 0.8. Two TOCTOU races: token refresh relied on a version CAS checked at write time, after both replicas had already called the token endpoint and the provider had rotated the refresh token — now an atomic cross-replica claim before the network call. Same class in the OAuth callback, where validate-then-consume let two callbacks redeem one code. tokenUrl was excluded from allowlisting while receiving the vault-resolved clientSecret — a client-secret exfiltration path. Credential endpoints now validate against their own allowlist with one canonical origin format. Also: StaticAuth defined with reference-only secret fields; extraAuthParams restricted to non-secret params; deprecated GET routes drop the credential param entirely rather than rejecting it post-arrival; RFC 9728 discovery constrained to same-origin metadata and pre-approved authorization servers; grant-lifecycle routes excluded from operator write scope; client_credentials request contract specified; publicBaseUrl parsed not prefix-matched; sidecar isolation claim qualified; metric tags reduced to bounded categoricals; six storage/authorization boundary tests added.
rolandpickl
approved these changes
Aug 12, 2026
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.
Planning doc only — no code changes, no behavior change.
What this is
The design record for connecting EDDI agents to third-party SaaS platforms (Jira, Amplitude, Google Drive, …) over REST, MCP, or A2A with each platform's real auth model, including per-end-user OAuth.
What the investigation established
All
file:lineclaims were spot-checked against source (see the plan'sVerifiedheader), not taken from a summary:authorization_code/grant_type/refresh_token/redirect_uri/token_endpointinsrc/main/javareturns nothing.SecretReferenceis(tenantId, keyName);PropertySetterTask.autoVaultSecretkeys per agent, so two end users of one agent overwrite each other's secret.stdiois hard-rejected, thoughStdioMcpTransportships in the pinned langchain4j-mcp jar.Exposure findings that gate the connector work:
/mcp(33 tools) is unauthenticated under the shippedauthorization.enabled=falsedefault whileAuthStartupGuardeffectively never fires; two REST endpoints take credentials as query parameters, one echoing the credential back in its response body; tool results reach the LLM verbatim while only tool descriptions are sanitized;SecretScrubbermisses array elements and URL-embedded credentials on export.Plan shape
Six phases, sequenced so each is independently shippable:
McpStartupGuard, credential params → body, console redaction, scrubber holes)ConnectionConfigurationresource +${connection:name}reference withSERVICE | PER_USERbinding — the gate for everything belowDecisions worth reviewing
SecretResolver— that would break the two properties the vault grant-enforcement design depends on (no identity in the resolver;ChatModelRegistrycaches on unresolved params).permitpath (application-type=servicemeans no bearer on a browser redirect); a single-use, DB-persistedstateis its only guard; PKCE mandatory.PER_USERconnections refuse to deploy whenauthorization.enabled=false. Without a verified identity, any caller claiminguserId=aliceresolves Alice's tokens — the/v1adapter'strust-user-headerscaveat documents exactly this failure mode. The same guard refuses OAuth connections when the vault is inert, so refresh tokens can never hit theautoVaultSecretplaintext-fallback path.authorization.enableddefault stays untouched — per-surface guard + path policy, per the precedent inopenai-api-adapter-plan.mdandmcp-hitl-surface-plan.md.falsedefault is documented intent (application.properties:341-343), not an oversight.Rebase note
Rebased onto #668, which required two corrections: the MCP resource bridge (
exposeResources) is a second untrusted-content surface on the existing MCP connection (same credential, so not a new connector path — but resource text is verbatim and listing metadata is ungoverned, now folded into G5); and strict task-leveltoolApprovals. The plan's C11 constraint was re-checked againstToolApprovalGate.classifyand still holds.Open questions (plan §13)
§13.1 is the one that must be answered before Phase 5: in a group conversation, whose token does a member agent use?
GroupConversationService.discuss()creates per-member conversations, so the principal may not be a human at all.Summary by CodeRabbit