fix: validate organization ownership for fetched proposals and proposal details #2389
Conversation
JivusAyrus
requested review from
Aenimus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughAdds a per-subgraph identifier Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-08T20:57:07.946ZApplied to files:
📚 Learning: 2025-09-10T09:53:42.914ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
🔇 Additional comments (2)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2389 +/- ##
=======================================
Coverage ? 31.14%
=======================================
Files ? 340
Lines ? 33837
Branches ? 241
=======================================
Hits ? 10537
Misses ? 22384
Partials ? 916 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (1)
166-176: Gate proposed-schema query on presence ofsubgraphIdto avoid spurious 404sBecause the backend now validates
req.subgraphIdagainstSubgraphRepository, calling this query whenactiveSubgraph?.subgraphIdis unset (e.g. newly created or deleted subgraphs that don’t have a persisted subgraph row yet) will always come back as"Subgraph not found".You’re already falling back to the
sdlprop whensdlData?.proposedSchemais empty, so you can avoid unnecessary failing calls by also requiring a concretesubgraphIdin theenabledflag:const { data: sdlData, isLoading: fetchingSdl } = useQuery( getProposedSchemaOfCheckedSubgraph, { checkId, checkedSubgraphId: activeSubgraph?.id, subgraphId: activeSubgraph?.subgraphId, }, { - enabled: !!activeSubgraph && !!activeSubgraphName, + enabled: !!activeSubgraph && !!activeSubgraphName && !!activeSubgraph?.subgraphId, }, );This keeps behavior for subgraphs without an id the same as before (use the
sdlprop) while still exercising the stricter backend path when an id is available.controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.ts (1)
11-42: Org-scoped subgraph lookup adds security, but RPC now requires non-nullsubgraphIdUsing
SubgraphRepository(logger, opts.db, authContext.organizationId)and validatingbyId(req.subgraphId)is solid for org-scoped access control.However, this introduces a backwards compatibility constraint: the repository query uses
eq(schema.schemaCheckSubgraphs.subgraphId, subgraphId), which will not match rows wheresubgraphIdis NULL. Since the database schema allowssubgraphIdto be nullable (withonDelete: 'set null'), old checks whoseschema_check_subgraphsrows have NULL subgraphId will silently fail to return data, even if the proposed SDL exists.Studio mitigates this risk by guarding the query with
enabled: !!activeSubgraph && !!activeSubgraphName, so the RPC won't execute unless activeSubgraph is populated. Since Studio is the only known consumer, the practical impact is limited. However, if other clients are added later, or if checks with NULL subgraphId exist in production, they will hit ERR_NOT_FOUND.
♻️ Duplicate comments (1)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
571-585: AdditionalsubgraphIdfilter further constrains lookups; watch out for rows with NULLsubgraphIdExtending
getProposedSchemaOfCheckedSubgraphto requiresubgraphIdand addingeq(schema.schemaCheckSubgraphs.subgraphId, subgraphId)in the WHERE clause is a good defense against mismatched(checkId, checkedSubgraphId, subgraphId)tuples.Given that
createSchemaCheckSubgraphsometimes insertssubgraphIdasundefined/NULLfor new subgraphs (no persisted subgraph row yet), the combination of this filter and the new handler logic means those rows will no longer be returned once a non-emptysubgraphIdis supplied. That’s aligned with the stricter backend behavior discussed ingetProposedSchemaOfCheckedSubgraph.ts; just make sure that’s acceptable for your existing data and clients, or consider treatingsubgraphId === ""as “no filter” if you deliberately want to keep the old behavior for those historical rows.
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/proposal/getProposalChecks.ts (1)
12-52: Org-scoped federated graph check correctly gates proposal checksCreating an org-scoped
FederatedGraphRepositoryand short-circuiting whenbyId(proposal.proposal.federatedGraphId)returns nothing is a good way to ensure callers only see checks for proposals whose federated graph belongs to their organization.One thing to consider (non-blocking): callers can distinguish between a truly missing proposal (
"Proposal with ID … not found") and a proposal whose federated graph is inaccessible ("Federated graph of the proposal not found"). If you want to avoid that enumeration difference, you could normalize both paths to a single"Proposal not found"-style detail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (7)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(2 hunks)controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.ts(2 hunks)controlplane/src/core/bufservices/proposal/getProposal.ts(1 hunks)controlplane/src/core/bufservices/proposal/getProposalChecks.ts(3 hunks)controlplane/src/core/repositories/SchemaCheckRepository.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
proto/wg/cosmo/platform/v1/platform.protoconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/repositories/SchemaCheckRepository.ts
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform_pb.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.ts
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/ProposalRepository.ts:562-572
Timestamp: 2025-09-10T11:15:52.157Z
Learning: The getLatestCheckForProposal function in controlplane/src/core/repositories/ProposalRepository.ts is only called during proposal creation or updates, so proposal match error checking (hasProposalMatchError) is not needed since the proposal is being modified itself rather than being matched against.
Applied to files:
controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposal.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.ts
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx
🧬 Code graph analysis (2)
controlplane/src/core/bufservices/check/getProposedSchemaOfCheckedSubgraph.ts (1)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
SubgraphRepository(84-2196)
controlplane/src/core/bufservices/proposal/getProposalChecks.ts (1)
controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(98-1836)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (4)
controlplane/src/core/bufservices/proposal/getProposal.ts (1)
41-49: Generic not-found message for federated graph is an improvementSwitching to the generic
"Federated graph of the proposal not found"detail avoids exposing the internal federatedGraphId while keeping behavior unchanged. Looks good.proto/wg/cosmo/platform/v1/platform.proto (1)
2736-2745: NewsubgraphId/proposedSchemafields are consistent with backend and UI usageAdding
string subgraphId = 3;toGetProposedSchemaOfCheckedSubgraphRequestandstring proposedSchema = 2;to the response cleanly reflects how the handler and UI now work: they pass an explicit subgraph identifier and receive a concrete SDL string.The field numbers are appended (1,2 → 3 and 1 → 2), so wire compatibility is preserved at the protobuf level; the behavioral tightening is entirely in the handler.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
21589-21596: Auto-generated protobuf field addition looks correct.The new
subgraphIdfield follows the established pattern for string fields in this generated file, with proper initialization and partial message support. This addition supports the organization ownership validation implemented in the service layer.
21601-21605: Field descriptor correctly added.The new field entry maintains sequential field numbering and uses the correct scalar type for strings.
StarpTech
left a comment
StarpTech
left a comment
There was a problem hiding this comment.
Can we test this easily?
…posal operations and ensure data isolation across organizations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
controlplane/src/core/repositories/ProposalRepository.ts (1)
633-649: Consider documenting the organizationId parameter usage.The
organizationIdparameter is accepted but only used forgetLinkedSchemaChecks(line 715), not for filtering the main query. This is safe because the caller validates the proposal's organization ownership before calling this method, but the inconsistency could be confusing.Consider either:
- Adding a comment explaining that proposal ownership is validated by the caller
- Or adding an explicit organization filter to the main query for defense-in-depth
controlplane/test/proposal/proposal-data-isolation.test.ts (2)
24-24: Consider using a more specific type instead ofany.The
clientparameter is typed asany, which reduces type safety. Consider using the actual client type from your test utilities or the generated protobuf types.
899-899: Clarify the duplicate comment.The comment on line 899 is identical to line 887, but the test scenarios are different. The first test (lines 887-897) attempts to approve Company A's proposal using Company A's federated graph name, while the second test (lines 899-909) attempts to approve using Company B's federated graph name. Consider updating the comment to reflect this distinction:
- // Try to approve Company A's proposal from Company B - should fail + // Try to approve Company A's proposal using Company B's federated graph - should fail
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
controlplane/src/core/bufservices/check/getCheckSummary.ts(1 hunks)controlplane/src/core/bufservices/proposal/configureNamespaceProposalConfig.ts(1 hunks)controlplane/src/core/bufservices/proposal/createProposal.ts(1 hunks)controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts(1 hunks)controlplane/src/core/bufservices/proposal/getNamespaceProposalConfig.ts(1 hunks)controlplane/src/core/bufservices/proposal/getProposal.ts(2 hunks)controlplane/src/core/bufservices/proposal/getProposalChecks.ts(3 hunks)controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(1 hunks)controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts(1 hunks)controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts(1 hunks)controlplane/src/core/repositories/ProposalRepository.ts(15 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(1 hunks)controlplane/test/proposal/proposal-data-isolation.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/ProposalRepository.ts:562-572
Timestamp: 2025-09-10T11:15:52.157Z
Learning: The getLatestCheckForProposal function in controlplane/src/core/repositories/ProposalRepository.ts is only called during proposal creation or updates, so proposal match error checking (hasProposalMatchError) is not needed since the proposal is being modified itself rather than being matched against.
Applied to files:
controlplane/src/core/bufservices/proposal/createProposal.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/proposal/getNamespaceProposalConfig.tscontrolplane/src/core/bufservices/proposal/configureNamespaceProposalConfig.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/test/proposal/proposal-data-isolation.test.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.tscontrolplane/src/core/bufservices/proposal/enableProposalsForNamespace.tscontrolplane/src/core/bufservices/proposal/getProposal.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/proposal/createProposal.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/proposal/getNamespaceProposalConfig.tscontrolplane/src/core/bufservices/proposal/configureNamespaceProposalConfig.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/test/proposal/proposal-data-isolation.test.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.tscontrolplane/src/core/bufservices/proposal/enableProposalsForNamespace.tscontrolplane/src/core/bufservices/proposal/getProposal.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.tscontrolplane/test/proposal/proposal-data-isolation.test.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/test/proposal/proposal-data-isolation.test.ts
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T09:59:17.257Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:59:17.257Z
Learning: When skipTrafficCheck is true in schema checks, the traffic inspection loop is completely bypassed (as shown in line 2040 of SubgraphRepository.ts with the continue statement), which means hasClientTraffic will be false. The traffic analysis doesn't run at all when skipTrafficCheck is true, unlike the previous understanding that it always ran to collect usage data.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T09:52:46.326Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:52:46.326Z
Learning: When skipTrafficCheck is true in schema checks, traffic analysis still runs to collect usage data, but hasClientTraffic can still be true if unsafe client traffic is detected. The difference is that traffic issues don't cause the overall check to fail, but the hasClientTraffic flag itself remains accurate based on the actual traffic analysis results.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T11:31:32.502Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/ProposalRepository.ts:0-0
Timestamp: 2025-09-10T11:31:32.502Z
Learning: When traffic checks or pruning checks are skipped in schema validation, the corresponding hasClientTraffic and hasGraphPruningErrors flags are automatically set to false by default. This means that checking clientTrafficCheckSkipped or graphPruningCheckSkipped flags in linked check failure conditions is redundant, as skipped checks will naturally have false values for their respective has* flags.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-07-01T13:53:54.146Z
Learnt from: wilsonrivera
Repo: wundergraph/cosmo PR: 1919
File: controlplane/src/core/repositories/OrganizationGroupRepository.ts:193-224
Timestamp: 2025-07-01T13:53:54.146Z
Learning: In the Cosmo codebase, database transactions are typically managed at the service layer (e.g., in buf services like deleteOrganizationGroup.ts), where repositories are instantiated with the transaction handle and all operations within those repositories are automatically part of the same transaction.
Applied to files:
controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts
🧬 Code graph analysis (12)
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/getNamespaceProposalConfig.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/configureNamespaceProposalConfig.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/check/getCheckSummary.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/getProposalChecks.ts (2)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(98-1836)
controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
controlplane/src/core/bufservices/proposal/getProposal.ts (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(21-845)
🔇 Additional comments (32)
controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts (1)
38-38: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context by passing
authContext.organizationId. This ensures that all proposal-related queries are scoped to the authenticated user's organization, implementing proper multi-tenant data isolation.This change is consistent across all affected files in this PR and aligns with the security objective of validating organization ownership for proposal operations.
controlplane/src/core/bufservices/check/getCheckSummary.ts (1)
39-39: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures proposal-related queries in the check summary flow are properly scoped to the authenticated organization.
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
42-42: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures that proposal creation operations are properly scoped to the authenticated organization, preventing cross-tenant data access.
controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts (1)
60-60: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures that proposal matching during subgraph publishing is properly scoped to the authenticated organization.
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1839-1839: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context within the
performSchemaCheckmethod. This ensures that proposal matching during schema checks is properly scoped to the repository's organization.controlplane/src/core/bufservices/proposal/getNamespaceProposalConfig.ts (1)
28-28: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures that proposal configuration retrieval is properly scoped to the authenticated organization.
controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts (1)
30-30: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures that enabling/disabling proposals for a namespace is properly scoped to the authenticated organization.
controlplane/src/core/bufservices/proposal/configureNamespaceProposalConfig.ts (1)
31-31: LGTM! Organization-scoped repository instantiation.The ProposalRepository is now correctly instantiated with the organization context. This ensures that configuring proposal settings is properly scoped to the authenticated organization.
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
41-41: Organization scoping correctly applied.The
ProposalRepositoryinstantiation now includesauthContext.organizationId, ensuring all proposal queries in this handler are properly scoped to the authenticated user's organization. This aligns with the IDOR fix pattern applied across the codebase.controlplane/src/core/bufservices/proposal/getProposalChecks.ts (2)
25-27: Organization scoping correctly applied to repositories.Both
ProposalRepositoryandFederatedGraphRepositoryare now instantiated with the authenticated user'sorganizationId, ensuring data isolation.
42-52: Defense-in-depth validation for federated graph.The additional federated graph lookup provides an extra layer of validation. Since
proposalRepo.ByIdalready filters byorganizationId, this check serves as defense-in-depth—if the proposal passes the org filter, its associated federated graph should also exist within the same organization. This is a sound security pattern.controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (2)
29-29: Organization scoping correctly applied.The
ProposalRepositoryinstantiation now includesauthContext.organizationId, ensuring proposal queries are properly scoped.
121-121: Simplified method signature aligns with repository refactor.The
getLatestCheckForProposalcall no longer requires an explicitorganizationIdparameter since the repository now stores it as an instance member. This is cleaner and ensures consistent organization scoping across all repository methods.controlplane/src/core/bufservices/proposal/getProposal.ts (3)
28-28: Organization scoping correctly applied.The
ProposalRepositoryinstantiation now includesauthContext.organizationId, ensuring all proposal queries are properly scoped to the authenticated user's organization.
46-46: Improved error message avoids leaking internal identifiers.The error message now uses a generic description instead of including the
federatedGraphId, which is a good security practice to avoid information leakage.
52-52: Simplified method signature aligns with repository refactor.The
getLatestCheckForProposalcall now uses the repository's internalorganizationIdmember instead of requiring it as a parameter.controlplane/src/core/repositories/ProposalRepository.ts (9)
22-25: Core security fix: organization scoping via constructor.Storing
organizationIdas an instance member and using it across all query methods is the right approach for enforcing multi-tenant data isolation. This ensures consistent organization scoping without requiring each method call to pass the parameter.
98-101: Organization filter correctly applied via join path.The query now joins through
federatedGraphs→targetsand filters bytargets.organizationId, ensuring proposals from other organizations cannot be accessed even if an attacker guesses a valid proposal ID.
163-172: Organization filter correctly applied to ByName query.The same join pattern through
federatedGraphs→targetswithorganizationIdfilter is consistently applied here.
229-260: Conditions array pattern with organization scoping.Using
(SQL<unknown> | undefined)[]for building dynamic conditions is a clean pattern. The organization filter is correctly included in theand()clause.
330-352: Count query consistently scoped by organization.The count query mirrors the list query's organization scoping pattern, ensuring accurate counts within the tenant boundary.
438-444: Proposal config now organization-scoped.The join to
namespacesand filter byorganizationIdprevents cross-tenant access to namespace proposal configurations.
604-607: Linked checks query now uses repository's organizationId.Using
this.organizationIdfor the linked schema checks query ensures consistent organization scoping without requiring the caller to pass it explicitly.
802-806: Organization filter correctly applied to getProposalByCheckId.The join path through
federatedGraphs→targetswithorganizationIdfilter prevents cross-tenant proposal lookups by check ID.
833-839: Organization filter correctly applied to proposal schema matches.Consistent application of the organization scoping pattern for this query method.
controlplane/test/proposal/proposal-data-isolation.test.ts (7)
41-140: LGTM! Well-structured cross-organization access test.This test properly validates that proposals from one organization cannot be accessed by another organization. The test setup, execution, and assertions are correct.
142-261: LGTM! Comprehensive cross-organization update test.This test effectively validates that proposals from one organization cannot be modified by users from another organization. The inclusion of a "maliciousField" in the update attempt clearly demonstrates the security concern being addressed.
263-368: LGTM! Thorough listing isolation test.This test properly validates that proposal listings are scoped to the organization. Creating multiple proposals (proposalNameA1 and proposalNameA2) in Company A ensures the test is comprehensive. The dual assertion checking both the error code and empty results list is appropriate.
370-469: LGTM! Proper isolation test for proposal checks.This test validates that proposal checks cannot be accessed across organizations. The comment on line 404 helpfully clarifies that creating a proposal automatically creates a schema check, which is important context for understanding the test.
471-511: LGTM! Clear namespace configuration isolation test.This test properly validates that namespace proposal configurations are organization-scoped. The comments clearly explain that Company B should see its own default namespace configuration (disabled) rather than Company A's configuration (enabled).
513-797: LGTM! Comprehensive federated graph isolation test.This test thoroughly validates that proposals are properly isolated between different federated graphs within the same organization. It covers multiple operations (create, list, get, update) and includes an important negative test (lines 774-794) that verifies a proposal cannot be updated using a different federated graph name.
799-924: LGTM! Thorough state transition isolation test.This test comprehensively validates that state transitions (approve and close) for proposals are properly isolated across organizations. The three negative test scenarios effectively cover different attack vectors:
- Using Company A's federated graph name (lines 887-897)
- Using Company B's federated graph name with Company A's proposal name (lines 899-909)
- Attempting to close the proposal (lines 911-921)
…09-wun-q425-15-insecure-direct-object-reference-via-proposals
JivusAyrus
deleted the
suvij/eng-8609-wun-q425-15-insecure-direct-object-reference-via-proposals
branch
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist