RockteMQ-AI commented on code in PR #2940:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2940#discussion_r3909124690
##########
web/src/services/consumerService.test.ts:
##########
@@ -44,7 +44,29 @@ vi.mock('../config', () => ({
}));
vi.mock('../api/metadata', () => metadataApi);
-describe('consumer service mock data', () => {
+describe('consumer service mock data', () => { it('clamps zero or negative
pagination to the first page in mock mode', () => {
Review Comment:
The `describe` opening brace and the first `it(...)` call are on the same
line (`describe('consumer service mock data', () => { it('clamps ...`). Should
be on separate lines for readability and consistency with the other test files.
##########
web/src/services/consumerService.ts:
##########
@@ -112,8 +112,10 @@ export async function listConsumerGroupPage(
params: ConsumerGroupPageQuery = {},
): Promise<PageResult<ConsumerGroup>> {
if (isMockMode()) {
- const page = params.page ?? 1;
- const pageSize = params.pageSize ?? 20;
+ // Clamp like listTopicsPage so a zero or negative page/pageSize cannot
make the
Review Comment:
The clamping is applied only in mock mode. In the real-API path, the
original `params.page` and `params.pageSize` are forwarded to the backend
without any client-side validation. While server-side validation is the
ultimate defense, consider applying the same clamping before the API call for
defense in depth — or at minimum document that callers must validate inputs.
The same concern applies to `topicService.getTopicConsumerPage` (line ~237) and
`messageService.listDLQGroups` (line ~142), which pass raw `page`/`pageSize` to
the API.
##########
web/src/services/consumerService.ts:
##########
@@ -112,8 +112,10 @@ export async function listConsumerGroupPage(
params: ConsumerGroupPageQuery = {},
Review Comment:
The PR body claims that `aclService.pageAclUsers`,
`messageService.queryMessagePage`, and `opsService.listAlertRulesPage` already
clamp pagination inputs. Investigation shows that only `listTopicsPage`
actually uses `Math.max`/`Math.min` clamping; `pageAclUsers` computes `from =
(params.page - 1) * params.pageSize` with no guard, and `queryMessagePage` uses
only `?? 1` / `?? 50` defaults (no `Math.max`/`Math.min`). `listAlertRulesPage`
does not exist. This means `pageAclUsers` and `queryMessagePage` remain
vulnerable to the same negative-slice bug this PR fixes. Consider either fixing
them in this PR or filing a follow-up issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
