fix(data-warehouse): scope pending source credentials to their creator - #71250
Conversation
A team member could list every pending credential id via stored_credentials and consume a teammate's stashed secrets through create or setup, turning metadata-only ids into bearer capabilities within the project. Both the list and the resolve paths now require created_by=request.user. Also restores the @extend_schema that #71056 accidentally moved from create onto the inserted _resolve_stored_credential helper, reverting create's generated response type regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ential-creator-ownership-f1e353
|
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Stored credentials win over inline keys so an agent can't override what the user entered. | ||
| return {**payload, **credential.payload}, credential, None | ||
|
|
||
| @extend_schema(request=ExternalDataSourceCreateSerializer, responses=ExternalDataSourceSerializers) |
There was a problem hiding this comment.
A successful create returns only {"id": new_source_model.pk}, but this annotation makes generated clients expect the full ExternalDataSourceSerializers object. Callers can access type-checked fields such as status, source_type, or schemas and receive undefined; the endpoint should either return the serialized source or declare an ID-only response serializer.
Context Used: docs/published/handbook/engineering/type-system.md (source)
There was a problem hiding this comment.
Fixed in 16afe40: create now declares an id-only ExternalDataSourceCreateResponseSerializer matching the actual {"id": pk} body, and the regenerated frontend/MCP types now expect exactly that. Worth noting the inaccuracy predates this PR — the pre-#71056 annotation already claimed the full serializers shape — but since this PR restores the decorator it's the right place to correct it.
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.79 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.3 KiB | ../node_modules/.pnpm/posthog-js@1.402.3/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 222.9 KiB | ../node_modules/.pnpm/posthog-js@1.402.3/node_modules/posthog-js/dist/module.js |
| 164.0 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | ../packages/quill/packages/quill/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — 🔺 +376 B (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1312.86 MiB · 🔺 +376 B (+0.0%)
ℹ️ MCP UI apps size — 31 app(s), 16561.5 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 598.2 KB | 179.2 KB |
| action | 456.5 KB | 179.2 KB |
| action-list | 563.0 KB | 179.2 KB |
| cohort | 455.4 KB | 179.2 KB |
| cohort-list | 562.0 KB | 179.2 KB |
| email-template | 455.3 KB | 179.2 KB |
| error-details | 471.1 KB | 179.2 KB |
| error-issue | 456.1 KB | 179.2 KB |
| error-issue-list | 562.9 KB | 179.2 KB |
| experiment | 560.1 KB | 179.2 KB |
| experiment-list | 563.8 KB | 179.2 KB |
| experiment-results | 561.8 KB | 179.2 KB |
| feature-flag | 565.8 KB | 179.2 KB |
| feature-flag-list | 569.5 KB | 179.2 KB |
| feature-flag-testing | 459.4 KB | 179.2 KB |
| insight-actors | 560.8 KB | 179.2 KB |
| invite-email-preview | 454.7 KB | 179.2 KB |
| llm-costs | 558.1 KB | 179.2 KB |
| session-recording | 457.2 KB | 179.2 KB |
| session-summary | 462.5 KB | 179.2 KB |
| survey | 457.0 KB | 179.2 KB |
| survey-global-stats | 560.9 KB | 179.2 KB |
| survey-list | 563.7 KB | 179.2 KB |
| survey-stats | 560.8 KB | 179.2 KB |
| trace-span | 455.8 KB | 179.2 KB |
| trace-span-list | 562.9 KB | 179.2 KB |
| workflow | 455.8 KB | 179.2 KB |
| workflow-list | 562.4 KB | 179.2 KB |
| query-results | 743.7 KB | 179.2 KB |
| render-ui | 824.0 KB | 179.2 KB |
| visual-review-snapshots | 460.3 KB | 179.2 KB |
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
…ential-creator-ownership-f1e353
…te response
mypy rejects User | AnonymousUser in the created_by lookups. Also replaces
create's restored responses=ExternalDataSourceSerializers annotation, which
never matched the actual {"id": pk} body, with an id-only serializer
(Greptile P1 on the PR).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r-ownership-f1e353' into claude/pending-credential-creator-ownership-f1e353
2 participants
Problem
Pending source credentials (the connect-link stash behind
store_credentials/stored_credentials/setup/create) were only scoped by team.Any member of the same team could call
stored_credentialsto enumerate every pending credential id,then submit a teammate's id to
createorsetupand have that user's external-service credential installed into a source the caller controls and can sync and query.That turns metadata-only credential ids into reusable bearer capabilities within the project.
Follow-up to #71056, which added the
createconsumption path.Changes
_resolve_stored_credential(shared bycreateandsetup) now requirescreated_by=request.userin the lookup.A teammate's id gets the same opaque "not found or expired" 400 as a nonexistent one, so nothing about the row leaks.
stored_credentialsnow filters the list bycreated_by=request.user, closing the enumeration half.connect_linkinstructions, and the MCP tool descriptions intools.yamlnow state that stored credentials are visible to and consumable by their creator only, and that the connect page must be filled by the same user the agent session authenticates as.@extend_schema(request=ExternalDataSourceCreateSerializer, responses=ExternalDataSourceSerializers)that feat(data-warehouse): let the create endpoint use a stored credential_id #71056 accidentally left on the inserted helper instead ofcreate.That regression had flipped
create's generated response type toExternalDataSourceCreate; the regenerated types in this PR flip it back.Note
There is no legitimate cross-user path through this stash today.
The documented connect-link flow is same-user end to end: the agent acts as user X, X opens the link in their own browser session, and X's agent consumes the credential.
If handing a connect link to a different user ever becomes a real need, it should be an explicit grant model, not team-wide ids.
How did you test this code?
test_consuming_another_team_members_credential_returns_400, parameterized oversetupandcreate: a same-team teammate's credential id returns 400, creates no source, and leaves the teammate's stash untouched. This is the bearer-capability regression itself; no existing test had two users in one team.test_stored_credentials_list_filters_by_source_type_and_hides_expired_other_teams_and_other_userswith a teammate-created credential that must not be enumerable.created_byto the requesting user, so the existing consume tests double as proof the owner path still works.ruff,hogli build:openapi, andhogli ci:preflight --strictafter merging master in.Automatic notifications
Docs update
No user-facing docs under
docs/cover this endpoint behavior; the agent-facing copy lives in the OpenAPI descriptions andtools.yaml, both updated here.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Daniel directed this from a review finding on #71056.
I (actually Claude) considered whether the connect-link flow intentionally supported cross-user handoff (one user generating the link, another filling it) before locking it down;
nothing in the code, docs, tool descriptions, or the #71056 discussion supports that flow, and it is exactly the reported attack shape, so a strict owner filter won over a sharing policy.
Skills invoked:
/improving-drf-endpointsand/writing-tests.While in there I found the orphaned
@extend_schemadecorator (visible as a response-type flip in the #71056 bot regen commit) and fixed it since this PR touches those exact lines.