feat: disallow reviewing proposals not created by Cosmo#2218
Conversation
wilsonrivera
requested review from
Aenimus,
JivusAyrus,
StarpTech,
endigma,
jensneuse and
thisisnithin
as code owners
WalkthroughAdds a ProposalOrigin enum and origin field across protobufs, DB, backend services, CLI, UI, and tests; enforces origin-based gating on proposal updates; adds DB migration and conversion helpers; includes minor Python calculator method/signature and variable name changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
Router-nonroot 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
🧹 Nitpick comments (7)
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
347-353: Persisting origin on create.Looks good. Minor: make the mapping exhaustive to surface unknown enum values rather than defaulting silently to COSMO.
Example improvement (in util.ts):
export function toProposalOriginEnum(value: ProposalOriginPB): ProposalOriginDB { switch (value) { case ProposalOriginPB.HUB: return 'HUB'; case ProposalOriginPB.COSMO: return 'COSMO'; default: // optionally log/metrics here return 'COSMO'; } }studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
304-404: Gate “Review Changes” to COSMO‑origin drafts.Matches the requirement to disallow reviewing non‑Cosmo proposals. Consider a small UX hint when hidden, so users know why actions aren’t available.
Minimal UI hint example:
- <div className="flex flex-1 items-center justify-end gap-1"> - {state === "DRAFT" && origin === ProposalOrigin.COSMO && ( + <div className="flex flex-1 items-center justify-end gap-1"> + {state === "DRAFT" && origin === ProposalOrigin.COSMO ? ( <DropdownMenu> ... </DropdownMenu> - )} + ) : state === "DRAFT" ? ( + <Badge variant="outline" title="Review is only available for Cosmo-origin proposals"> + Origin: {origin === ProposalOrigin.HUB ? "Hub" : "Cosmo"} + </Badge> + ) : null} </div>controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
154-173: Fix typo in error message; consider status code.Message has a double “be”. Also consider ERR_FORBIDDEN if available; NOT_FOUND is acceptable if you’re intentionally hiding existence.
Apply this diff to fix the typo:
- details: `The proposal ${req.proposalName} cannot be be updated directly.`, + details: `The proposal ${req.proposalName} cannot be updated directly.`,Optional: add a debug/info log when blocking to aid supportability.
proto/wg/cosmo/platform/v1/platform.proto (1)
2690-2691: Additive field looks correct; add a doc comment for clarity.Proto3 defaults enum fields to 0, so this will read as COSMO when not set. Consider adding a brief comment documenting that behavior.
Apply this diff to document intent:
- ProposalOrigin origin = 11; + // Origin of this proposal. Defaults to COSMO when unset (proto3 enum default = 0). + ProposalOrigin origin = 11;controlplane/src/db/schema.ts (1)
2466-2468: New indexes LGTM.Helpful for common lookups by createdById and federatedGraphId.
If you’ll frequently filter by origin, consider a small composite index like (federatedGraphId, origin).
controlplane/src/core/util.ts (2)
658-667: Simplify mapping via lookup tables (keeps behavior identical).Functionally fine; a small table-based mapper trims boilerplate while preserving the COSMO fallback.
Apply this diff:
-export function toProposalOriginEnum(value: ProposalOrigin): ProposalOriginEnum { - switch (value) { - case ProposalOrigin.HUB: { - return 'HUB'; - } - default: { - return 'COSMO'; - } - } -} +const toDbOriginMap: Record<ProposalOrigin, ProposalOriginEnum> = { + [ProposalOrigin.COSMO]: 'COSMO', + [ProposalOrigin.HUB]: 'HUB', +}; +export function toProposalOriginEnum(value: ProposalOrigin): ProposalOriginEnum { + return toDbOriginMap[value] ?? 'COSMO'; +}
669-678: Mirror the above simplification for reverse mapping.Keeps forward compatibility fallback to COSMO.
Apply this diff:
-export function fromProposalOriginEnum(value: ProposalOriginEnum): ProposalOrigin { - switch (value) { - case 'HUB': { - return ProposalOrigin.HUB; - } - default: { - return ProposalOrigin.COSMO; - } - } -} +const fromDbOriginMap: Record<ProposalOriginEnum, ProposalOrigin> = { + COSMO: ProposalOrigin.COSMO, + HUB: ProposalOrigin.HUB, +}; +export function fromProposalOriginEnum(value: ProposalOriginEnum): ProposalOrigin { + return fromDbOriginMap[value] ?? ProposalOrigin.COSMO; +}Add a quick unit test to assert both directions for COSMO/HUB and default fallback.
📜 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 (15)
cli/src/commands/proposal/commands/create.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(5 hunks)controlplane/migrations/0132_nappy_hardball.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/proposal/createProposal.ts(2 hunks)controlplane/src/core/bufservices/proposal/getProposal.ts(2 hunks)controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(1 hunks)controlplane/src/core/repositories/ProposalRepository.ts(11 hunks)controlplane/src/core/util.ts(2 hunks)controlplane/src/db/models.ts(2 hunks)controlplane/src/db/schema.ts(2 hunks)controlplane/src/types/index.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(3 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#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/updateProposal.ts
🧬 Code graph analysis (11)
controlplane/src/types/index.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
connect-go/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
cli/src/commands/proposal/commands/create.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
controlplane/src/db/models.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)controlplane/src/db/schema.ts (1)
proposalOriginEnum(2445-2445)
controlplane/src/core/bufservices/proposal/getProposal.ts (1)
controlplane/src/core/util.ts (1)
fromProposalOriginEnum(669-678)
controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (1)
controlplane/src/core/util.ts (1)
fromProposalOriginEnum(669-678)
controlplane/src/core/util.ts (1)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
controlplane/src/core/util.ts (1)
toProposalOriginEnum(658-667)
controlplane/src/core/repositories/ProposalRepository.ts (1)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
⏰ 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)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (28)
controlplane/migrations/meta/_journal.json (1)
929-935: Journal entry appended correctly (idx 132).Idx, tag, and version look consistent with prior entries. No action required.
controlplane/src/types/index.ts (2)
3-10: Importing ProposalOrigin into shared types is appropriate.This keeps DTOs aligned with DB-backed enum values. Watch for name clashes with the protobuf enum in other modules.
If there are files importing both the DB type and the protobuf enum, ensure imports are aliased to avoid confusion (e.g., ProposalOriginDB vs ProposalOriginPB).
782-783: ProposalDTO now carries origin.Good addition; downstream API responses and UI can gate actions accordingly.
controlplane/src/db/models.ts (2)
18-19: Imported proposalOriginEnum for typing.OK.
39-40: Type alias for ProposalOrigin.Union type derived from enum values is correct and future‑proof.
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
25-25: Convert protobuf origin to DB enum at the boundary.The util import is appropriate here.
controlplane/migrations/0132_nappy_hardball.sql (1)
1-2: Migration adds enum and not‑null column with default.Solid: existing rows get COSMO and future inserts are enforced. No further changes needed.
cli/src/commands/proposal/commands/create.ts (2)
4-4: Importing ProposalOrigin from protobuf.OK.
83-84: CLI sets origin to COSMO.Matches product intent; server still has a safe default.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
73-76: Importing ProposalOrigin for UI gating.Good.
223-231: Reading origin from payload.OK.
proto/wg/cosmo/platform/v1/platform.proto (2)
2701-2705: Enum definition is proto3‑friendly (0 == COSMO).Good choice reserving 0 for the internal default. No issues.
2717-2718: Creation API: defaults to COSMO — no action requiredcreateProposal maps req.origin via toProposalOriginEnum (defaults to 'COSMO'); CLI sets origin to ProposalOrigin.COSMO; DB schema has origin default 'COSMO'. Verified in: controlplane/src/core/bufservices/proposal/createProposal.ts (uses toProposalOriginEnum(req.origin)), controlplane/src/core/util.ts (toProposalOriginEnum default -> 'COSMO'), cli/src/commands/proposal/commands/create.ts (origin: ProposalOrigin.COSMO), controlplane/src/db/schema.ts (origin default 'COSMO').
controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (2)
12-12: Import enrichment LGTM.Pulling in fromProposalOriginEnum here keeps mapping logic centralized.
156-157: Resolve — default‑to‑COSMO is intentional and enforced by migrationMigration controlplane/migrations/0132_nappy_hardball.sql creates enum proposal_origin and adds origin with DEFAULT 'COSMO' NOT NULL; keeping fromProposalOriginEnum defaulting to COSMO is correct.
controlplane/src/core/util.ts (1)
22-24: Import aliasing avoids type name collision.Clear separation between proto enum (numeric) and DB enum (string). Good.
controlplane/src/db/schema.ts (2)
2445-2446: DB enum created and used correctly in migrationcontrolplane/migrations/0132_nappy_hardball.sql creates public.proposal_origin AS ENUM('COSMO','HUB') and then ALTER TABLE adds the origin column referencing it; controlplane/src/db/schema.ts export matches — no change required.
2462-2462: Resolve — migration already adds DEFAULT + NOT NULLcontrolplane/migrations/0132_nappy_hardball.sql contains:
ALTER TABLE "proposals" ADD COLUMN "origin" "proposal_origin" DEFAULT 'COSMO' NOT NULL;
Existing rows will be valid with the default.connect/src/wg/cosmo/platform/v1/platform_pb.ts (5)
363-381: Enum addition and registration LGTM.
ProposalOriginvalues andsetEnumTypewiring are correct and consistent with typical proto3 enum patterns. No issues here.
21167-21168: Field descriptor entry LGTM.Using
proto3.getEnumType(ProposalOrigin)for field 11 is correct and matches the enum registration.
21295-21296: Verify generated bindings for 'origin' tag numbersproto/wg/cosmo/platform/v1/platform.proto shows Proposal.origin = 11 (line 2690) and CreateProposalRequest.origin = 6 (line 2717). Search of generated outputs (connect-go/gen/proto and connect/src/wg/cosmo/platform/v1/platform_pb.ts) did not find an 'origin' field with tag 11. Verify that generated Go/TypeScript bindings for message Proposal use field number 11 — regenerate stubs or fix protos if they do not.
21144-21147: No bare-truthiness usage found; verify one suspicious string comparisonRepo scan found no truthy checks (if/proposal.origin, &&, ternary, switch). Found explicit uses and one questionable comparison — verify and fix if needed:
- controlplane/src/core/bufservices/proposal/updateProposal.ts:154 —
if (proposal.proposal.origin !== 'COSMO')→ confirm this matches the enum representation (useProposalOrigin.COSMOor the mapped string consistently).- controlplane/src/core/bufservices/proposal/getProposal.ts:92 —
origin: fromProposalOriginEnum(proposal.proposal.origin)- controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts:156 —
origin: fromProposalOriginEnum(proposal.proposal.origin)- controlplane/src/core/repositories/ProposalRepository.ts:275 —
origin: proposal.originAction: Replace any truthiness checks with explicit comparisons (e.g.,
proposal.origin === ProposalOrigin.COSMO) and correct the string-vs-enum comparison in updateProposal.ts if it's mismatched.
21277-21281: Confirm producers and server handling of origin — action: verify and set origin where needed.
- connect/src/wg/cosmo/platform/v1/platform_pb.ts defines CreateProposalRequest.origin default = ProposalOrigin.COSMO; cli/src/commands/proposal/commands/create.ts explicitly sets origin: ProposalOrigin.COSMO.
- controlplane util maps unknown/strings to ProposalOrigin, defaulting to COSMO. Tests call client.createProposal(...) widely but do not appear to pass origin in the call sites shown.
- Risk: non-CLI producers (tests, jobs, other services) that omit origin will be attributed COSMO (proto default / server mapping). Ensure producers that should be HUB (or other) explicitly set origin, or make server-side handling explicit (normalize/validate origin on server).
Actions for author:
- If intent is to attribute missing origin to COSMO, no change required.
- If intent is to require explicit origin: add server-side validation or change generated default; update createProposal producers (tests/jobs/services) to pass origin explicitly where appropriate — audit createProposal call sites in controlplane/test and services and set origin.
controlplane/src/core/repositories/ProposalRepository.ts (4)
85-86: ById: origin selection and DTO mapping — OK.Joins and serialization keep origin intact.
Also applies to: 117-118
148-149: ByName: origin selection and DTO mapping — OK.Consistent with ById.
Also applies to: 181-182
229-230: ByFederatedGraphId: origin selection and DTO mapping — OK.End-to-end origin surfaced alongside batched subgraph retrieval.
Also applies to: 275-276
17-23: Origin persisted — migration verifiedFound in controlplane/migrations/0132_nappy_hardball.sql: it creates type "proposal_origin" and runs ALTER TABLE "proposals" ADD COLUMN "origin" "proposal_origin" DEFAULT 'COSMO' NOT NULL.
controlplane/src/core/bufservices/proposal/getProposal.ts (1)
13-13: Incorrect enum-mapping direction — current code is correctfromProposalOriginEnum maps DB/domain -> proto (correct for building response protos); toProposalOriginEnum maps proto -> domain (correct for storing). Evidence: controlplane/src/core/util.ts — toProposalOriginEnum(value: ProposalOrigin): ProposalOriginEnum (line 658), fromProposalOriginEnum(value: ProposalOriginEnum): ProposalOrigin (line 669). Current usages are consistent: createProposal.ts uses toProposalOriginEnum(req.origin) (line 352); getProposal.ts (line 92) and getProposalsByFederatedGraph.ts (line 156) use fromProposalOriginEnum(proposal.proposal.origin). No change required.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
controlplane/test/proposal/create-proposal.test.ts (1)
118-118: Assert that origin persists (and defaults) to strengthen coverage.You set origin: COSMO on every create. Add one assertion on getProposal to verify proposal.origin === ProposalOrigin.COSMO, and add a single test that omits origin to confirm server-side defaulting to COSMO.
Apply this minimal assertion in one happy-path test after fetching the proposal:
expect(proposalResponse.response?.code).toBe(EnumStatusCode.OK); expect(proposalResponse.proposal?.name).toBe(`p-1/${proposalName}`); expect(proposalResponse.proposal?.federatedGraphName).toBe(fedGraphName); + expect(proposalResponse.proposal?.origin).toBe(ProposalOrigin.COSMO);Optional: add a small test "should default origin to COSMO when omitted" that calls createProposal without origin and asserts the same.
Also applies to: 213-213, 258-258, 337-337, 447-447, 549-549, 653-653, 700-700, 815-815, 1011-1011, 1033-1033, 1120-1120, 1148-1148, 1226-1226, 1294-1294, 1377-1377, 1480-1480, 1575-1575, 1620-1620, 1685-1685, 1751-1751, 1772-1772, 1860-1860, 1887-1887
controlplane/test/proposal/update-proposal.test.ts (3)
52-56: Helper: default origin is good; mark unused param to reduce noise.options.subgraphSchemaSDL is not used inside createTestProposal. Either use it or underscore-prefix to signal intentional ignore.
Apply a low-impact tweak:
-async function createTestProposal( +async function createTestProposal( client: any, options: { federatedGraphName: string; proposalName: string; subgraphName: string; - subgraphSchemaSDL: string; + subgraphSchemaSDL: string; // currently unused updatedSubgraphSDL: string; origin?: ProposalOrigin, }, ) { - const { federatedGraphName, proposalName, subgraphName, subgraphSchemaSDL, updatedSubgraphSDL, origin } = options; + const { federatedGraphName, proposalName, subgraphName, subgraphSchemaSDL: _subgraphSchemaSDL, updatedSubgraphSDL, origin } = options;Also applies to: 61-61
517-517: Also assert origin in update flows and keep it immutable.Add an origin assertion after getProposal in at least one update path, and (optionally) a negative test proving origin cannot be changed via updateProposal.
Suggested assertion:
expect(getProposalResponse.response?.code).toBe(EnumStatusCode.OK); expect(getProposalResponse.proposal?.subgraphs.length).toBe(2); +expect(getProposalResponse.proposal?.origin).toBe(ProposalOrigin.COSMO);Optional follow-up test: attempt to pass any origin-like field in updateAction (should be rejected/not possible) and assert origin remains COSMO on readback.
Also applies to: 685-685, 1510-1510, 1786-1786
517-517: Add a HUB-origin scenario test (separate file is fine).Given the PR goal (“disallow reviewing proposals not created by Cosmo”), add a test creating a proposal with origin: HUB, then attempt a review action and expect ERROR_NOT_AUTHORIZED.
I can draft a focused test (e.g., controlplane/test/proposal/review-proposal-origin-gate.test.ts) that:
- creates proposal with origin HUB,
- calls review endpoint,
- asserts ERROR_NOT_AUTHORIZED,
- repeats with origin COSMO and asserts success.
Want me to open an issue and provide the test skeleton?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/test/proposal/create-proposal.test.ts(26 hunks)controlplane/test/proposal/update-proposal.test.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controlplane/test/proposal/create-proposal.test.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
controlplane/test/proposal/update-proposal.test.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)controlplane/test/test-util.ts (1)
DEFAULT_NAMESPACE(52-52)
⏰ 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). (11)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
controlplane/test/proposal/create-proposal.test.ts (2)
3-3: Importing ProposalOrigin is correct and consistent.Matches the new proto surface and keeps tests explicit.
118-118: Repo-wide check: createProposal calls missing origin — confirm defaultingScan found most call sites include an explicit origin; these test calls do not: controlplane/test/proposal/proposal-schema-match.test.ts:67–81, 98–111, 1430–1443. The handler forwards req.origin (toProposalOriginEnum(req.origin)). Confirm the tests intentionally rely on the server default mapping or add an explicit origin.
controlplane/test/proposal/update-proposal.test.ts (2)
4-4: Importing ProposalOrigin is correct.Keeps tests aligned with the proto enum.
4-4: Repo-wide verification — origin coverage & immutabilityAutomated scans failed (ripgrep/awk errors). Run these locally and paste results so I can confirm:
# 1) Any updateProposal payloads that include 'origin' rg -nP -U --hidden --no-ignore --type=ts '\bupdateProposal\s*\(\s*{[^}]*\borigin\b' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true # 2) Show createProposal call contexts so we can check for missing 'origin' rg -nP --hidden --no-ignore --type=ts '\bcreateProposal\s*\(' -n --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | \ while IFS= read -r match; do f="${match%%:*}"; ln="${match##*:}" echo "---- $f:$ln ----" sed -n "${ln},$((ln+80))p" "$f" echo done
JivusAyrus
left a comment
JivusAyrus
left a comment
There was a problem hiding this comment.
We should have a proposal origin check in the updateProposal rpc.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
controlplane/test/test-util.ts (2)
371-381: Broadened interceptor logic is good; update the comment to match behavior.The code now handles both cosmo-cli and cosmo-hub. The comment still mentions only cosmo-cli; tighten it to avoid drift.
Apply:
- // Interceptor to handle cosmo-cli user-agent + // Interceptor to handle cosmo-{cli,hub} client indicator - // Check if x-cosmo-client header is set to cosmo-cli + // Check if x-cosmo-client header is set to cosmo-cli or cosmo-hub
375-378: LGTM; minor hardening idea (optional).Consider accepting any value starting with "cosmo-" to ease future client additions (e.g., cosmo-studio), while still deleting x-cosmo-client.
Example:
- if (cosmoClient === 'cosmo-cli' || cosmoClient === 'cosmo-hub') { + if (cosmoClient && cosmoClient.startsWith('cosmo-')) {controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
162-181: Add server-side observability on origin mismatches.Return ERR_NOT_FOUND to callers (good for not leaking), but log a structured warn so ops can spot misrouted clients.
Example:
- if (proposal.proposal.origin !== expectedOrigin) { + if (proposal.proposal.origin !== expectedOrigin) { + logger.warn( + { proposalId: proposal.proposal.id, name: proposal.proposal.name, expectedOrigin, actualOrigin: proposal.proposal.origin }, + 'proposal origin mismatch on updateProposal' + ); return { response: { code: EnumStatusCode.ERR_NOT_FOUND, details: `Proposal ${req.proposalName} not found`, }, ... }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/bufservices/proposal/updateProposal.ts(2 hunks)controlplane/test/proposal/update-proposal.test.ts(8 hunks)controlplane/test/test-util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/test/proposal/update-proposal.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#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/updateProposal.ts
🧬 Code graph analysis (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
🔇 Additional comments (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
11-11: Import looks correct.Type import for ProposalOrigin is appropriate.
…-from-the-hub-cosmo-should-not # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
154-175: If you keep caller-origin matching, don’t sniff only User-Agent; prefer x-cosmo-client + tolerant match.This was raised earlier and still applies; UA is spoofable and often unavailable from browsers. Read x-cosmo-client first, fallback to UA, and use case-insensitive substring match.
- const clientHdr = ctx.requestHeader.get('user-agent')?.toLowerCase() ?? ''; - const expectedOrigin: ProposalOrigin = clientHdr.includes('cosmo-hub') ? 'HUB' : 'COSMO'; + const clientHdr = + ctx.requestHeader.get('x-cosmo-client') ?? + ctx.requestHeader.get('user-agent') ?? + ''; + const hdr = clientHdr.toLowerCase(); + const expectedOrigin: ProposalOrigin = hdr.includes('cosmo-hub') ? 'HUB' : 'COSMO';
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
11-11: Useimport typefor type-only symbols.Prevents emitting a runtime import for TS-only types (safer with .js paths). If you later drop
ProposalOrigin(see next comment), adjust accordingly.-import { ProposalOrigin, ProposalState } from '../../../db/models.js'; +import type { ProposalOrigin, ProposalState } from '../../../db/models.js';
📜 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 (5)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(5 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(2 hunks)controlplane/src/types/index.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(3 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx
- controlplane/src/types/index.ts
- proto/wg/cosmo/platform/v1/platform.proto
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#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/updateProposal.ts
🧬 Code graph analysis (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
⏰ 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)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
…-from-the-hub-cosmo-should-not
…-originating-from-the-hub-cosmo-should-not' into wilson/eng-7878-if-a-proposal-is-originating-from-the-hub-cosmo-should-not
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (4)
230-231: Guard for undefined origin and derive a readable flagOlder/legacy proposals might lack
origin. DeriveisCosmoOriginonce to simplify conditions and make the intent explicit.Apply:
const { name, createdAt, createdByEmail, state, subgraphs, latestCheckSuccess, - origin, + origin, } = proposal; + + const isCosmoOrigin = origin === ProposalOrigin.COSMO;
308-334: Don’t nest Tooltip content inside a disabled DropdownMenuTrigger; conditionally render the whole menuWith
DropdownMenuTriggerdisabled, the nestedTooltipmay be flaky across browsers/ATs, and the menu content is still in the DOM though unusable. Render the dropdown only when allowed; otherwise render a standalone tooltip + non-interactive control. This improves a11y and reduces markup.Apply:
- {state === "DRAFT" && ( - <DropdownMenu> - <DropdownMenuTrigger asChild disabled={origin !== ProposalOrigin.COSMO}> - {origin === ProposalOrigin.COSMO ? ( - <Button - className="ml-4" - disabled={isApproving || isClosing} - > - Review Changes - <ChevronDownIcon className="ml-2 h-4 w-4" /> - </Button> - ) : ( - <Tooltip> - <TooltipTrigger asChild> - <span - className={buttonVariants({ - className: "ml-4 cursor-not-allowed opacity-60 hover:!bg-primary", - })} - > - Review Changes - <ChevronDownIcon className="ml-2 h-4 w-4" /> - </span> - </TooltipTrigger> - <TooltipContent> - This proposal was created outside of Cosmo and cannot be updated within Cosmo. - </TooltipContent> - </Tooltip> - )} - </DropdownMenuTrigger> - <DropdownMenuContent align="end" className="w-[500px] p-3"> + {state === "DRAFT" && ( + isCosmoOrigin ? ( + <DropdownMenu> + <DropdownMenuTrigger asChild> + <Button + className="ml-4" + disabled={isApproving || isClosing} + > + Review Changes + <ChevronDownIcon className="ml-2 h-4 w-4" /> + </Button> + </DropdownMenuTrigger> + <DropdownMenuContent align="end" className="w-[500px] p-3"> ... - </DropdownMenuContent> - </DropdownMenu> + </DropdownMenuContent> + </DropdownMenu> + ) : ( + <Tooltip> + <TooltipTrigger asChild> + <span + className={buttonVariants({ + // keep visual consistency with a primary button, but non-interactive + className: "ml-4 cursor-not-allowed opacity-60", + })} + role="button" + aria-disabled="true" + tabIndex={-1} + > + Review Changes + <ChevronDownIcon className="ml-2 h-4 w-4" /> + </span> + </TooltipTrigger> + <TooltipContent> + This proposal was created outside of Cosmo and cannot be updated within Cosmo. + </TooltipContent> + </Tooltip> + ) )}Also applies to: 335-424
321-323: Use buttonVariants with explicit variant/size for consistencyPassing only
classNamemay miss variants (theme drift). Specify variant/size to match the enabled button.Example:
- className={buttonVariants({ - className: "ml-4 cursor-not-allowed opacity-60", - })} + className={buttonVariants({ + variant: "default", + size: "default", + className: "ml-4 cursor-not-allowed opacity-60", + })}
810-841: Consider adding an e2e test to assert the gatingAdd a Studio e2e test that:
- Loads a HUB-origin proposal and asserts the “Review Changes” control is non-interactive and shows the tooltip.
- Loads a COSMO-origin proposal and asserts the dropdown opens and actions are enabled/disabled based on
isApproving/isClosing.I can draft a Playwright spec targeting these states if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
⏰ 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)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (3)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (3)
75-76: Enum import aligns with the PR goalImporting
ProposalOriginfrom the generated connect types is correct and matches the feature intent.
308-334: Overall: matches PR intentGating “Review Changes” by
origin === COSMOfulfills the feature requirement.
308-309: Server-side origin check present — resolvedcontrolplane/src/core/bufservices/proposal/updateProposal.ts returns an error when proposal.proposal.origin !== expectedOrigin (expectedOrigin is derived from the request user-agent; see lines ~154–158). updateProposal handles state transitions (approve/close), so the backend blocks mismatched-origin update/approve requests.
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
approved on behalf of CLI, please get an approval for controlplane separately
…-from-the-hub-cosmo-should-not
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
controlplane/migrations/0132_slippery_payback.sql (1)
1-2: Qualify enum type and make default cast explicit; consider idempotency guard
- Minor: keep schema-qualification consistent and make the DEFAULT explicit to the enum to avoid search_path surprises.
- Optional: if your migration runner can re-run in certain envs, wrap CREATE TYPE in an idempotent guard to avoid failures.
Apply this diff:
-CREATE TYPE "public"."proposal_origin" AS ENUM('INTERNAL', 'EXTERNAL');--> statement-breakpoint -ALTER TABLE "proposals" ADD COLUMN "origin" "proposal_origin" DEFAULT 'INTERNAL' NOT NULL; +CREATE TYPE "public"."proposal_origin" AS ENUM('INTERNAL', 'EXTERNAL');--> statement-breakpoint +ALTER TABLE "proposals" + ADD COLUMN "origin" "public"."proposal_origin" + NOT NULL + DEFAULT 'INTERNAL'::"public"."proposal_origin";If you want an idempotent type creation, consider:
DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_type t JOIN pg_namespace n ON n.oid = t.typnamespace WHERE t.typname = 'proposal_origin' AND n.nspname = 'public') THEN CREATE TYPE "public"."proposal_origin" AS ENUM('INTERNAL', 'EXTERNAL'); END IF; END$$;studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
309-333: Make disabled “Review Changes” fully inert and accessibleGood gating on origin. Minor UX nits:
- Remove the hover color on the disabled-looking element.
- Add aria-disabled for assistive tech.
Apply this diff:
- <Tooltip> + <Tooltip> <TooltipTrigger asChild> - <span - className={buttonVariants({ - className: "ml-4 cursor-not-allowed opacity-60 hover:!bg-primary", - })} - > + <span + aria-disabled="true" + className={buttonVariants({ + className: "ml-4 cursor-not-allowed opacity-60", + })} + > Review Changes <ChevronDownIcon className="ml-2 h-4 w-4" /> </span> </TooltipTrigger>controlplane/test/proposal/update-proposal.test.ts (3)
52-62: Default origin handling: simplify and tighten typesGood call passing origin through and defaulting to INTERNAL. Minor cleanups:
- Set the default at destructuring to avoid the nullish coalescing at usage.
- Optional: type the client param to PromiseClient for better IDE/type safety.
- Optional: subgraphSchemaSDL is not used in this helper; consider removing it from the signature to avoid confusion.
Apply this minimal change:
- const { federatedGraphName, proposalName, subgraphName, subgraphSchemaSDL, updatedSubgraphSDL, origin } = options; + const { + federatedGraphName, + proposalName, + subgraphName, + subgraphSchemaSDL, + updatedSubgraphSDL, + origin = ProposalOrigin.INTERNAL, + } = options; ... - origin: origin ?? ProposalOrigin.INTERNAL, + origin,Type annotations (outside this range) you may consider:
import type { PromiseClient } from '@connectrpc/connect'; import { PlatformService } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_connectquery'; async function createTestProposal( client: PromiseClient<typeof PlatformService>, options: { /* ... */ } ) { /* ... */ }
513-518: DRY up origin: INTERNAL in direct createProposal callsYou’re explicitly setting origin: ProposalOrigin.INTERNAL in multiple direct createProposal calls. Since your helper already defaults to INTERNAL, consider:
- Using the helper where feasible to reduce duplication, or
- Extracting a small factory/local helper for direct multi-subgraph cases to keep origin handling consistent in one place.
Also applies to: 681-686, 1504-1511, 1780-1786
2217-2237: Add coverage for state updates on EXTERNAL-origin proposalsYou only validate updatedSubgraphs is blocked. Add a similar check for updateAction: 'state' to ensure all review/update paths are gated by origin.
Apply after the first update attempt:
const updateProposalResponse = await client.updateProposal({ proposalName: createProposalResponse.proposalName, federatedGraphName: fedGraphName, namespace: DEFAULT_NAMESPACE, updateAction: { case: 'updatedSubgraphs', value: { subgraphs: [ { name: newSubgraphName, schemaSDL: newSubgraphSchemaSDL, isDeleted: false, isNew: true, labels: [label], }, ], }, }, }); + // Attempt a state transition should also be blocked + const stateUpdateResp = await client.updateProposal({ + proposalName: createProposalResponse.proposalName, + federatedGraphName: fedGraphName, + namespace: DEFAULT_NAMESPACE, + updateAction: { case: 'state', value: 'APPROVED' }, + }); + expect(stateUpdateResp.response?.code).toBe(EnumStatusCode.ERR_NOT_FOUND);
📜 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 (10)
cli/src/commands/proposal/commands/create.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(5 hunks)controlplane/migrations/0132_slippery_payback.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/util.ts(2 hunks)controlplane/src/db/schema.ts(2 hunks)controlplane/test/proposal/create-proposal.test.ts(26 hunks)controlplane/test/proposal/update-proposal.test.ts(8 hunks)proto/wg/cosmo/platform/v1/platform.proto(3 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- controlplane/src/db/schema.ts
- controlplane/test/proposal/create-proposal.test.ts
- controlplane/src/core/util.ts
- controlplane/migrations/meta/_journal.json
🧰 Additional context used
🧬 Code graph analysis (4)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
controlplane/test/proposal/update-proposal.test.ts (5)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)controlplane/test/test-util.ts (6)
DEFAULT_NAMESPACE(52-52)SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_SUBGRAPH_URL_ONE(49-49)createFederatedGraph(623-638)DEFAULT_ROUTER_URL(48-48)controlplane/src/core/test-util.ts (2)
genID(53-55)genUniqueLabel(57-59)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
createFederatedGraph(346-355)enableProposalsForNamespace(2512-2521)controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts (1)
enableProposalsForNamespace(17-96)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
cli/src/commands/proposal/commands/create.ts (2)
controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)
⏰ 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). (7)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
proto/wg/cosmo/platform/v1/platform.proto (1)
2717-2718: Field numbers and defaults for ProposalOrigin are sound; ensure codegen is updated
- origin added to Proposal as field 11 and CreateProposalRequest as field 6; enum defaults to INTERNAL (0) when omitted, matching desired behavior.
- No numbering collisions observed.
Confirm regenerated artifacts are committed across languages (TS, Go, etc.) to avoid CI breakage. Typical steps:
- pnpm generate && pnpm build for @wundergraph/cosmo-connect
- Ensure connect-go/gen and any other generated targets are updated
Also applies to: 2728-2731, 2744-2745
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
224-231: Origin plumbed through the UI state correctlyDestructuring origin and using it to gate actions aligns with the backend contract.
cli/src/commands/proposal/commands/create.ts (1)
4-4: All CreateProposal callers use INTERNAL origin by default
CLI explicitly sets ProposalOrigin.INTERNAL; generated CreateProposalRequest defaults to INTERNAL when origin is omitted.connect/src/wg/cosmo/platform/v1/platform_pb.ts (3)
367-381: Enum addition aligns with proto changeThe new
ProposalOriginenum and metadata registration match the proto updates and keep the generated TS artifacts in sync. ✅
21378-21400: Proposal.origin default keeps backward compatibilityAdding the
originfield with an INTERNAL default preserves behavior for existing clients while allowing the new gating logic to discriminate external requests. Looks good.
21511-21528: CreateProposalRequest origin plumbing is correctPropagating the enum to
CreateProposalRequestensures callers can explicitly pass the origin, and the default still favors current clients. Implementation checks out.controlplane/test/proposal/update-proposal.test.ts (1)
4-4: Import looks correct and necessaryBringing in ProposalOrigin aligns with the new origin field usage throughout the tests.
…-from-the-hub-cosmo-should-not
…-from-the-hub-cosmo-should-not
…-from-the-hub-cosmo-should-not
…-from-the-hub-cosmo-should-not # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
154-175: Policy mismatch — enforce INTERNAL-only for proposal updatesThis issue was already flagged in a previous review. The current logic derives
expectedOriginfrom the caller's user-agent and permits EXTERNAL-origin proposals when the caller sends "cosmo-hub". The PR objective states to "disallow reviewing proposals not created by Cosmo," which means only proposals with origin 'INTERNAL' should be allowed for updates, regardless of the caller.Apply this diff to enforce INTERNAL-only updates:
- const clientHdr = ctx.requestHeader.get('user-agent')?.toLowerCase() ?? ''; - const expectedOrigin: ProposalOrigin = clientHdr.includes('cosmo-hub') ? 'EXTERNAL' : 'INTERNAL'; - if (proposal.proposal.origin !== expectedOrigin) { + // Only proposals created by Cosmo may be reviewed/updated + if (proposal.proposal.origin !== 'INTERNAL') { return { response: { code: EnumStatusCode.ERR_NOT_FOUND, details: `Proposal ${req.proposalName} not found`, }, breakingChanges: [], nonBreakingChanges: [], compositionErrors: [], checkId: '', lintWarnings: [], lintErrors: [], graphPruneWarnings: [], graphPruneErrors: [], compositionWarnings: [], lintingSkipped: false, graphPruningSkipped: false, checkUrl: '', }; }
📜 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 (4)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(5 hunks)controlplane/src/core/bufservices/proposal/createProposal.ts(2 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
controlplane/src/core/bufservices/proposal/updateProposal.ts (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)
controlplane/src/core/bufservices/proposal/createProposal.ts (1)
controlplane/src/core/util.ts (1)
toProposalOriginEnum(658-667)
⏰ 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)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (10)
controlplane/src/core/bufservices/proposal/updateProposal.ts (1)
11-11: LGTM!The import of
ProposalOriginis correctly added alongsideProposalStateand is necessary for the new origin verification logic.controlplane/src/core/bufservices/proposal/createProposal.ts (1)
25-25: LGTM!The import of
toProposalOriginEnumis correctly added to support the origin conversion at line 352.proto/wg/cosmo/platform/v1/platform.proto (3)
2728-2731: LGTM!The
ProposalOriginenum is well-defined:
- Follows proto3 conventions (first value is 0)
- Uses appropriate UPPER_SNAKE_CASE naming
INTERNALas the default (0) value provides a secure default- Values are clear and descriptive
2717-2717: LGTM!The
originfield in theProposalmessage is correctly defined:
- Field number 11 is sequential and doesn't conflict with existing fields
- Not marked optional, defaulting to
INTERNAL(0) is appropriate for backwards compatibility- Placement is logical within the message structure
2744-2744: Ensure origin validation is enforced server-side.The
originfield inCreateProposalRequestis correctly defined with field number 6. However, since this is a client-provided field, ensure that:
- Server-side validation enforces that the provided origin matches the actual request source
- Clients cannot arbitrarily set the origin to bypass access controls
The default value of
INTERNAL(0) is appropriate for backwards compatibility.This concern is related to the validation request in
createProposal.ts(line 352). The verification script provided there will help confirm if proper validation exists.connect/src/wg/cosmo/platform/v1/platform_pb.ts (5)
363-381: Enum wiring looks solid.
ProposalOriginmirrors the proto definition and is correctly registered withproto3.util.setEnumType.
21377-21380: Default aligns with expectations.Adding
originwith a default ofProposalOrigin.INTERNALkeeps existing behavior stable while exposing the new metadata.
21397-21401: Descriptor update matches enum type.Including
originin the field metadata withproto3.getEnumType(ProposalOrigin)keeps serialization consistent.
21510-21513: Sensible default on request payload.Create requests now carry
origin, defaulting toINTERNAL, which preserves existing clients while enabling overrides.
21525-21528: Field metadata stays in sync.The descriptor for
origincorrectly points to the sharedProposalOriginenum, ensuring consistent encode/decode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/proposal/update-proposal.test.ts (1)
2149-2243: LGTM with minor suggestions.The test correctly validates that external proposals cannot be updated. The regex pattern at line 2240 follows the previous review's recommendation and is safe in this test context despite the static analysis warning (the
proposalNameis controlled bygenID(), not user input, eliminating ReDoS risk).Consider adding a companion test that attempts to update the state (e.g., to APPROVED or CLOSED) of an external proposal, to ensure state transitions are also blocked. Currently, this test only verifies blocking subgraph updates.
Example test structure:
test('should fail to update state of a proposal created from hub', async () => { // ... setup similar to current test ... const createProposalResponse = await createTestProposal(client, { // ... same setup ... origin: ProposalOrigin.EXTERNAL, }); const updateProposalResponse = await client.updateProposal({ proposalName: createProposalResponse.proposalName, federatedGraphName: fedGraphName, namespace: DEFAULT_NAMESPACE, updateAction: { case: 'state', value: 'APPROVED', }, }); expect(updateProposalResponse.response?.code).toBe(EnumStatusCode.ERR_NOT_FOUND); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/proposal/update-proposal.test.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/test/proposal/update-proposal.test.ts (4)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
ProposalOrigin(562-562)ProposalOrigin(591-593)ProposalOrigin(595-597)ProposalOrigin(604-606)controlplane/src/db/models.ts (1)
ProposalOrigin(39-39)controlplane/test/test-util.ts (6)
DEFAULT_NAMESPACE(52-52)SetupTest(66-417)createThenPublishSubgraph(550-573)DEFAULT_SUBGRAPH_URL_ONE(49-49)createFederatedGraph(623-638)DEFAULT_ROUTER_URL(48-48)controlplane/src/core/bufservices/proposal/enableProposalsForNamespace.ts (1)
enableProposalsForNamespace(17-96)
🪛 ast-grep (0.39.6)
controlplane/test/proposal/update-proposal.test.ts
[warning] 2239-2239: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Proposal .*${proposalName} not found)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
🔇 Additional comments (3)
controlplane/test/proposal/update-proposal.test.ts (3)
4-4: LGTM!The import of
ProposalOriginaligns with the feature requirements and follows the existing import pattern.
44-75: LGTM!The helper function enhancement maintains backward compatibility while enabling origin-specific testing. The default to
ProposalOrigin.INTERNALensures existing test cases continue to work as expected.
517-517: LGTM!Explicitly specifying
origin: ProposalOrigin.INTERNALimproves test clarity and ensures the origin assumption is documented rather than implicit. This consistency across test cases is a good practice.Also applies to: 685-685, 1509-1509, 1657-1657, 1785-1785
wilsonrivera
deleted the
wilson/eng-7878-if-a-proposal-is-originating-from-the-hub-cosmo-should-not
branch
4 participants
Summary by CodeRabbit
New Features
Behavior Changes
Chores
Tests
Checklist