feat: add pagination and limit handling for API keys#2430
Conversation
JivusAyrus
requested review from
Aenimus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughAdds pagination fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
===========================================
- Coverage 62.77% 36.06% -26.72%
===========================================
Files 296 944 +648
Lines 41440 123477 +82037
Branches 4266 4951 +685
===========================================
+ Hits 26015 44530 +18515
- Misses 15404 77386 +61982
- Partials 21 1561 +1540
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
JivusAyrus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/bufservices/check/getChecksByFederatedGraphName.ts (1)
73-85: Add validation for lower bounds of limit and offset.The code validates that
limit <= 50but doesn't check iflimitis at least 1, nor does it validate thatoffset >= 0. Negative or zero limit values could bypass intended constraints; negative offset values could cause unexpected database behavior.
🧹 Nitpick comments (6)
controlplane/src/core/bufservices/organization/getAuditLogs.ts (1)
57-58: Consider using nullish coalescing operator.The logical OR (
||) operator treats all falsy values (including0) as missing. While unlikely to cause issues in this context, the nullish coalescing operator (??) is generally preferred for defaulting, as it only treatsundefinedandnullas missing values.🔎 Proposed refactor
-req.limit = req.limit || 50; +req.limit = req.limit ?? 50;controlplane/src/core/bufservices/check/getCheckOperations.ts (2)
76-77: Default limit handling looks good.The logic correctly sets a default limit of 200 when not provided, preventing the validation error on line 79 from triggering unexpectedly.
Optional: Consider using nullish coalescing and a local variable
The
||operator will treat explicit0as falsy and override it to 200. While this is unlikely to be a practical issue (0 doesn't make sense for pagination), the nullish coalescing operator (??) would be more precise. Additionally, using a local variable avoids mutating the request object:- req.limit = req.limit || 200; + const limit = req.limit ?? 200;Then update the validation and usage:
- if (req.limit > 200) { + if (limit > 200) {And at line 97:
- limit: req.limit, + limit: limit,
79-93: Consider improving the error message clarity.The validation logic is correct, but the error message "Invalid limit" could be more descriptive to help API consumers understand the constraint.
Optional: More descriptive error message
- details: 'Invalid limit', + details: 'Limit must not exceed 200',controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
22-32: Pagination validation implemented correctly.The defaulting and validation logic is sound. The function correctly defaults
limitto 50 and validates it doesn't exceed 50.Optional: Add validation for negative or zero limits
While the current defaulting prevents negative limits in practice, you could add an explicit check for completeness:
req.limit = req.limit || 50; -if (req.limit > 50) { +if (req.limit <= 0 || req.limit > 50) { return { response: { code: EnumStatusCode.ERR, details: 'Invalid limit', }, apiKeys: [], count: 0, }; }studio/src/pages/[organizationSlug]/apikeys.tsx (2)
714-725: Pagination query setup implemented correctly.The page number and limit extraction from router query is well-implemented, with appropriate defaults and client-side limit capping.
Optional: Specify radix in parseInt
For clarity and to avoid edge cases, consider explicitly specifying the radix:
-const pageNumber = router.query.page ? parseInt(router.query.page as string) : 1; +const pageNumber = router.query.page ? parseInt(router.query.page as string, 10) : 1;
769-769: Consider handling empty non-first pages.The condition
apiKeys.length === 0 && pageNumber === 1correctly shows the empty state only on the first page. However, if a user navigates to a page that becomes empty (e.g., after deletion), they'll see an empty table rather than being redirected to a valid page.This is acceptable current behavior, but you could enhance UX by redirecting to the last valid page when the current page is empty and
pageNumber > 1:useEffect(() => { if (apiKeys.length === 0 && pageNumber > 1 && noOfPages > 0) { router.push({ pathname: router.pathname, query: { ...router.query, page: noOfPages }, }); } }, [apiKeys.length, pageNumber, noOfPages, router]);
📜 Review details
Configuration used: Organization 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 (12)
connect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/api-key/createAPIKey.tscontrolplane/src/core/bufservices/api-key/getAPIKeys.tscontrolplane/src/core/bufservices/check/getCheckOperations.tscontrolplane/src/core/bufservices/check/getChecksByFederatedGraphName.tscontrolplane/src/core/bufservices/federated-graph/getCompositions.tscontrolplane/src/core/bufservices/organization/getAuditLogs.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.tscontrolplane/src/core/repositories/ApiKeyRepository.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/pages/[organizationSlug]/apikeys.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/getProposalChecks.ts
🧬 Code graph analysis (3)
controlplane/src/core/repositories/ApiKeyRepository.ts (4)
controlplane/src/types/index.ts (1)
APIKeyDTO(317-326)controlplane/src/db/schema.ts (1)
apiKeys(1175-1205)controlplane/src/core/repositories/SubgraphRepository.ts (1)
count(818-853)controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
count(557-572)
controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
controlplane/src/db/schema.ts (1)
apiKeys(1175-1205)
studio/src/pages/[organizationSlug]/apikeys.tsx (7)
studio/src/components/group-select.tsx (1)
GroupSelect(8-70)controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
getAPIKeys(9-52)controlplane/src/core/repositories/ApiKeyRepository.ts (1)
getAPIKeys(111-153)controlplane/src/core/util.ts (1)
checkUserAccess(284-291)controlplane/src/db/schema.ts (1)
apiKeys(1175-1205)connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
Pagination(5857-5895)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (3)
Pagination(7076-7083)Pagination(7098-7098)Pagination(7113-7115)
⏰ 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_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
controlplane/src/core/bufservices/organization/getAuditLogs.ts (1)
57-69: LGTM! Default limit and validation implemented correctly.The implementation properly defaults the limit to 50 when not provided and enforces the maximum bound, which aligns with the PR's DOS prevention objectives.
controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (2)
98-99: LGTM: Default limit handling.The default limit of 50 is correctly applied before validation, ensuring consistent pagination behavior when no limit is specified.
112-159: Include totalProposalsCount in the pagination response.The
getProposalCheckshandler returnstotalChecksCountfor pagination support, butgetProposalsByFederatedGraphomits a total count despite supporting paginated requests (limit/offset). TheProposalRepository.countByFederatedGraphIdmethod exists and is used elsewhere (createProposal), so it should be called here to match pagination patterns across handlers and support client-side pagination calculations.controlplane/src/core/bufservices/proposal/getProposalChecks.ts (2)
78-79: LGTM: Consistent default limit handling.The default limit of 50 is correctly applied before validation, maintaining consistency with the pagination pattern used in
getProposalsByFederatedGraph.
103-109: Good: Complete pagination response.The response correctly includes
totalChecksCountalongside the paginated results, enabling proper client-side pagination controls.controlplane/src/core/bufservices/federated-graph/getCompositions.ts (1)
74-75: LGTM! Pagination defaults and validation added.The defaulting of
limitto 50 and the upper-bound validation are correctly implemented and consistent with the pagination pattern used throughout this PR.proto/wg/cosmo/platform/v1/platform.proto (1)
1091-1099: LGTM! Pagination fields added to API Keys messages.The protobuf definitions correctly add
limitandoffsetto the request andcountto the response, following standard pagination patterns.controlplane/src/core/bufservices/api-key/createAPIKey.ts (1)
34-47: LGTM! API key limit enforcement added.The pre-check correctly enforces a maximum of 200 API keys per organization, addressing the DoS risk mentioned in the PR objectives. The
>= 200check allows exactly 200 keys to be created while preventing the 201st, which is the intended behavior.controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
34-42: LGTM! Pagination implementation complete.The function correctly fetches paginated API keys and the total count, returning both in the response as defined in the protobuf schema.
controlplane/src/core/repositories/ApiKeyRepository.ts (1)
155-165: LGTM! Total count method implemented correctly.The
getAPIKeysCountmethod is well-implemented, using the standard pattern of querying withcount()and providing a sensible fallback.studio/src/pages/[organizationSlug]/apikeys.tsx (2)
765-765: LGTM! Page count calculation is correct.The calculation properly uses
Math.ceilto handle partial pages and includes a fallback for undefined count.
931-935: LGTM! Pagination component integrated correctly.The Pagination component is properly configured with the necessary props for page navigation.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
8175-8183: Generated code looks correct for pagination support.This is auto-generated protobuf code, and the pagination fields (
limit,offseton request;counton response) are correctly defined. The default value of0forlimitmeans the service layer must handle this case by applying a sensible default limit to prevent unbounded queries.Ensure the corresponding service implementation validates that
limit > 0(or applies a default) and thatoffset >= 0before executing database queries.Also applies to: 8228-8231
wilsonrivera
left a comment
wilsonrivera
left a comment
There was a problem hiding this comment.
Can we clamp the limits instead of returning a failure? That way we can prevent negative values too
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
25-33: Consider concurrent execution of queries.The API keys fetch and count query execute sequentially. For better performance, consider running them concurrently using
Promise.all.🔎 Proposed optimization
- const apiKeys = await apiKeyRepo.getAPIKeys({ - organizationID: authContext.organizationId, - limit: req.limit, - offset: req.offset, - }); - - const count = await apiKeyRepo.getAPIKeysCount({ - organizationID: authContext.organizationId, - }); + const [apiKeys, count] = await Promise.all([ + apiKeyRepo.getAPIKeys({ + organizationID: authContext.organizationId, + limit: req.limit, + offset: req.offset, + }), + apiKeyRepo.getAPIKeysCount({ + organizationID: authContext.organizationId, + }), + ]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
controlplane/src/core/bufservices/api-key/getAPIKeys.tscontrolplane/src/core/bufservices/check/getCheckOperations.tscontrolplane/src/core/bufservices/check/getChecksByFederatedGraphName.tscontrolplane/src/core/bufservices/federated-graph/getCompositions.tscontrolplane/src/core/bufservices/organization/getAuditLogs.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- controlplane/src/core/bufservices/federated-graph/getCompositions.ts
- controlplane/src/core/bufservices/check/getChecksByFederatedGraphName.ts
- controlplane/src/core/bufservices/check/getCheckOperations.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.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/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
🧬 Code graph analysis (3)
controlplane/src/core/bufservices/proposal/getProposalChecks.ts (1)
controlplane/src/core/util.ts (1)
clamp(543-545)
controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
controlplane/src/db/schema.ts (1)
apiKeys(1175-1205)
controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (1)
controlplane/src/core/util.ts (1)
clamp(543-545)
⏰ 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_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
controlplane/src/core/bufservices/organization/getAuditLogs.ts (1)
12-12: LGTM! Unified limit handling.The addition of the
clamputility import aligns with the PR objective to standardize limit handling across services.controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (1)
12-12: LGTM! Import addition supports new pagination logic.The
clampimport is correctly added to support the limit handling introduced on line 99.controlplane/src/core/bufservices/proposal/getProposalChecks.ts (1)
11-11: LGTM! Import is necessary for the clamping logic.The addition of the
clampimport is correct and required for the limit handling introduced on line 79.controlplane/src/core/bufservices/api-key/getAPIKeys.ts (3)
7-7: LGTM: Import added for clamp utility.The
clampfunction import is correctly added to support the new limit validation logic.
23-23: LGTM: Limit clamping logic is correct.The clamp implementation correctly defaults to 10 when no limit is provided and constrains the value between 1 and 50, preventing both empty results and excessive queries.
35-41: LGTM: Response structure correctly includes count.The response properly returns both the paginated API keys and the total count, enabling proper pagination UI implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
controlplane/src/core/bufservices/organization/getAuditLogs.ts (1)
12-12: DoS prevention logic is sound; consider the mutation pattern.The clamp-based limit handling effectively prevents DoS by defaulting to 10 and constraining to [1, 50]. The typo mentioned in the past review has been corrected.
However, the pattern of mutating
req.limitdirectly (rather than using a local variable) persists. While this works in the current context, it could have unintended side effects if the request object is reused elsewhere. This pattern is consistent across the PR, so if you choose to refactor, consider applying the change uniformly.📝 Example refactor using a local variable
- // default to 10 if no limit is provided - req.limit = clamp(req.limit || 10, 1, 50); + // default to 10 if no limit is provided + const limit = clamp(req.limit || 10, 1, 50); const auditLogs = await auditLogRepo.getAuditLogs({ organizationId: authContext.organizationId, - limit: req.limit, + limit, offset: req.offset,Also applies to: 57-58
controlplane/src/core/bufservices/federated-graph/getCompositions.ts (1)
13-13: Consistent and effective limit handling.The clamp implementation correctly prevents DoS by defaulting to 10 and bounding to [1, 50]. The logic is sound and consistent with the pagination pattern across the PR.
As with
getAuditLogs.ts, consider using a local variable instead of mutatingreq.limitdirectly to avoid potential side effects if the request object is reused.📝 Example refactor using a local variable
- // default to 10 if no limit is provided - req.limit = clamp(req.limit || 10, 1, 50); + // default to 10 if no limit is provided + const limit = clamp(req.limit || 10, 1, 50); const compositions = await graphCompositionRepository.getGraphCompositions({ fedGraphTargetId: federatedGraph.targetId, organizationId: authContext.organizationId, - limit: req.limit, + limit, offset: req.offset,Also applies to: 74-75
controlplane/src/core/bufservices/check/getCheckOperations.ts (1)
13-13: Effective limit handling with appropriate bounds for operations.The clamp implementation correctly prevents DoS by defaulting to 10 and bounding to [1, 200]. The higher maximum limit (200 vs. 50 in audit logs and compositions) is appropriate given that check operations typically involve larger datasets.
As with the other pagination handlers, consider using a local variable instead of mutating
req.limitdirectly to maintain request object immutability.📝 Example refactor using a local variable
- // default to 10 if no limit is provided - req.limit = clamp(req.limit || 10, 1, 200); + // default to 10 if no limit is provided + const limit = clamp(req.limit || 10, 1, 200); const affectedOperations = await schemaCheckRepo.getAffectedOperationsByCheckId({ checkId: req.checkId, - limit: req.limit, + limit, offset: req.offset,Also applies to: 76-77
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
controlplane/src/core/bufservices/api-key/getAPIKeys.tscontrolplane/src/core/bufservices/check/getCheckOperations.tscontrolplane/src/core/bufservices/check/getChecksByFederatedGraphName.tscontrolplane/src/core/bufservices/federated-graph/getCompositions.tscontrolplane/src/core/bufservices/organization/getAuditLogs.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- controlplane/src/core/bufservices/proposal/getProposalChecks.ts
- controlplane/src/core/bufservices/api-key/getAPIKeys.ts
- controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
- controlplane/src/core/bufservices/check/getChecksByFederatedGraphName.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T11:26:26.657Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
Applied to files:
controlplane/src/core/bufservices/check/getCheckOperations.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/getCheckOperations.ts
🧬 Code graph analysis (1)
controlplane/src/core/bufservices/check/getCheckOperations.ts (1)
controlplane/src/core/util.ts (1)
clamp(543-545)
⏰ 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_test
- GitHub Check: build_push_image
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/fetch-checks-by-federated-graph.test.ts (1)
224-242: Consider publishing an initial schema before running checks.The test creates 51 checks without first publishing a baseline schema to the subgraph. Other tests in this file follow the pattern of publishing a schema first (e.g., lines 73-80, 176-182, 365-379), then running additional checks against that baseline. While
checkSubgraphSchemamay work without a prior publish, adding an initial publish would:
- Ensure consistency with the established test pattern
- Provide a clear baseline for the checks to validate against
- Reduce the risk of unexpected behavior in check recording
🔎 Suggested addition
expect(createSubgraphResp.response?.code).toBe(EnumStatusCode.OK); +// Publish an initial schema to establish a baseline +const initialPublishResp = await client.publishFederatedSubgraph({ + name: subgraphName, + namespace: DEFAULT_NAMESPACE, + schema: 'type Query { initial: String }', +}); + +expect(initialPublishResp.response?.code).toBe(EnumStatusCode.OK); + // Create 51 checks for (let i = 0; i < 51; i++) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/fetch-checks-by-federated-graph.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/test/fetch-checks-by-federated-graph.test.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/test/fetch-checks-by-federated-graph.test.ts
🧬 Code graph analysis (1)
controlplane/test/fetch-checks-by-federated-graph.test.ts (2)
controlplane/src/core/test-util.ts (1)
genID(53-55)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). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
controlplane/test/fetch-checks-by-federated-graph.test.ts (2)
234-242: The identical schemas are acceptable for limit testing.All 51 checks use the same schema (
type Query { a: String }). While this is sufficient for testing the limit clamping behavior, varying the schemas slightly (e.g., adding an index to field names) could make the test more representative of real-world usage. However, for the specific purpose of validating pagination limits, the current approach is adequate.
247-278: LGTM! Limit clamping logic is correctly validated.The test properly validates the new limit clamping behavior:
- Limits exceeding 50 are accepted and return a successful response (line 260)
- Results are correctly clamped to 50 items (line 262)
- Valid limit values work as expected (line 278)
This aligns with the PR objective to add pagination and limit handling with clamping instead of hard rejections.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/api-key/getAPIKeys.ts (1)
27-35: LGTM! Pagination implementation follows standard patterns.The paginated fetch and separate count retrieval are correctly implemented and scoped to the organization. This two-query approach is a common pagination pattern.
Optional consideration: If the count query becomes expensive at scale, consider caching the total count or using database-level optimizations (e.g., indexed count queries). Monitor query performance as the number of API keys grows.
controlplane/src/core/bufservices/federated-graph/getCompositions.ts (1)
74-76: Enhance the comment to document both pagination parameters and clamping bounds.The comment on line 74 only mentions the default limit but doesn't describe the offset default or the clamping ranges for both parameters. Updating it would improve clarity:
- // default to 10 if no limit is provided + // Clamp pagination: limit defaults to 10 [1-50], offset defaults to 0 [0-500000] req.limit = clamp(req.limit || 10, 1, 50); req.offset = clamp(req.offset || 0, 0, 500_000);This pattern is consistent across other pagination handlers in bufservices (getAuditLogs, getProposalChecks, getChecksByFederatedGraphName, etc.), with the offset bound intentionally set to prevent database performance degradation from excessively large offsets.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
controlplane/src/core/bufservices/api-key/getAPIKeys.tscontrolplane/src/core/bufservices/check/getCheckOperations.tscontrolplane/src/core/bufservices/check/getChecksByFederatedGraphName.tscontrolplane/src/core/bufservices/federated-graph/getCompositions.tscontrolplane/src/core/bufservices/organization/getAuditLogs.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts
- controlplane/src/core/bufservices/check/getChecksByFederatedGraphName.ts
- controlplane/src/core/bufservices/proposal/getProposalChecks.ts
- controlplane/src/core/bufservices/check/getCheckOperations.ts
⏰ 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: 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: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
controlplane/src/core/bufservices/organization/getAuditLogs.ts (1)
57-59: Good DoS prevention with pagination bounds.The clamping logic correctly prevents DoS attacks by limiting both
limitto [1, 50] andoffsetto [0, 500_000]. The addition of offset clamping (line 59) is a solid improvement.I notice the mutation approach for
req.limitandreq.offsetwas retained despite the previous suggestion to use local variables. Given the earlier feedback that "either approach works", this is fine—just wanted to confirm this was the intended resolution.controlplane/src/core/bufservices/api-key/getAPIKeys.ts (3)
7-7: LGTM! Clamp utility correctly imported.The
clamputility is properly imported and used for enforcing pagination bounds.
22-23: LGTM! Limit validation is correct.The default value of 10 and bounds [1, 50] are sensible for API key pagination.
37-43: LGTM! Response structure correctly includes pagination metadata.The response properly includes both the paginated
apiKeysand thecount, enabling client-side pagination controls.controlplane/src/core/bufservices/federated-graph/getCompositions.ts (1)
13-13: LGTM! Import addition supports the pagination clamping logic.The
clampimport is necessary for the new pagination bounds enforcement on lines 75-76.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @controlplane/src/core/bufservices/api-key/createAPIKey.ts:
- Around line 35-48: The current separate count check using
apiKeyRepo.getAPIKeysCount and the subsequent insert via apiKeyRepo.addAPIKey
causes a race condition allowing >200 keys; fix by enforcing the limit
atomically at the DB level (preferred) — add a CHECK constraint or trigger that
prevents inserting when an organization already has 200 keys; alternatively,
implement an application-level fix by acquiring a per-organization advisory/row
lock (based on authContext.organizationId) before calling getAPIKeysCount and
addAPIKey or move the count check into the same transaction inside addAPIKey so
the check-and-insert are atomic.
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/api-key/createAPIKey.ts (2)
35-48: Extract the hard-coded limit to a named constant.The value
200appears multiple times (comment, comparison, error message). Extract it to a constant for maintainability and to serve as a single source of truth.🔎 Proposed refactor
At the top of the file or in a constants module:
const MAX_API_KEYS_PER_ORGANIZATION = 200;Then update the code:
- // Check if the organization has reached the limit of 200 API keys + // Check if the organization has reached the maximum API key limit const apiKeysCount = await apiKeyRepo.getAPIKeysCount({ organizationID: authContext.organizationId, }); - if (apiKeysCount >= 200) { + if (apiKeysCount >= MAX_API_KEYS_PER_ORGANIZATION) { return { response: { code: EnumStatusCode.ERR, - details: 'Cannot create API key. Organization has reached the maximum limit of 200 API keys', + details: `Cannot create API key. Organization has reached the maximum limit of ${MAX_API_KEYS_PER_ORGANIZATION} API keys`, }, apiKey: '', }; }
35-48: Optional: Consider moving the count check after cheap validations.The count check requires a database query, while the name-length validation (line 64) is a cheap in-memory check. Reordering could avoid unnecessary DB calls when the name is invalid.
Trade-off: Current order provides faster feedback when the limit is reached, but performs a DB query even for trivially invalid inputs.
Suggested ordering
const keyName = req.name.trim(); + // Validate name length first (cheap check) + if (keyName.length < 3 || keyName.length > 50) { + return { + response: { + code: EnumStatusCode.ERR, + details: `An API key name ${req.name} does not follow the required naming rules`, + }, + apiKey: '', + }; + } + // Check if the organization has reached the limit of 200 API keys const apiKeysCount = await apiKeyRepo.getAPIKeysCount({ organizationID: authContext.organizationId, }); // ... rest of count check const apiKeyModel = await apiKeyRepo.getAPIKeyByName({ organizationID: authContext.organizationId, name: keyName, }); // ... rest of existing validations except name length check
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/core/bufservices/api-key/createAPIKey.tscontrolplane/src/core/repositories/ApiKeyRepository.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/repositories/ApiKeyRepository.ts
⏰ 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
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…08-wun-q425-14-dos-via-unrestricted-api-key-creation-and-lack
…ted-api-key-creation-and-lack
JivusAyrus
deleted the
suvij/eng-8608-wun-q425-14-dos-via-unrestricted-api-key-creation-and-lack
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist