fix: HTTP query string params array parsing #38222
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces direct qs usage with a new parseQueryParams utility (arrayLimit: 500, throwOnLimitExceeded: true), adds tests, and updates Router to call the utility. Router now catches parse errors, logs a warning, and returns HTTP 400 on invalid query parameters. Adds a shared logger for the Meteor API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router (HTTP)
participant Parser as parseQueryParams
participant Logger as logger
participant Server as Response
Client->>Router: HTTP request with query string
Router->>Parser: parseQueryParams(req.url or req.query)
alt parse succeeds
Parser-->>Router: parsed query object
Router->>Server: continue normal handling (next middleware/handler)
else parse fails (throws)
Parser-->>Router: throws error
Router->>Logger: log warning (path + error)
Router->>Server: respond 400 {"error":"Invalid query parameters"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38222 +/- ##
===========================================
+ Coverage 70.61% 70.62% +0.01%
===========================================
Files 3135 3136 +1
Lines 108614 108626 +12
Branches 19561 19537 -24
===========================================
+ Hits 76699 76720 +21
+ Misses 29909 29897 -12
- Partials 2006 2009 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/16 22:10 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 16 Jan 2026 22:10:01 GMT |
sampaiodiego
force-pushed
the
fix-http-array-parse
branch
from
fd802a9 to
10c7697
Compare
![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
🤖 Fix all issues with AI agents
In `@packages/http-router/src/Router.ts`:
- Around line 188-190: The parseQueryParams call in Router.parseQueryParams can
throw when parseQueryParams(...) exceeds array limits; wrap the call in a
try-catch inside the Router.parseQueryParams method (or catch where
Router.method invokes it) and map that specific error to a controlled 400
response instead of letting it propagate. Detect the
array-limit/throwOnLimitExceeded error from parseQueryParams, return an empty
params or a sentinel error from parseQueryParams, and in Router.method catch
that sentinel/error and return a 400 Bad Request with a clear message; refer to
the parseQueryParams function and the Router.method entry point to locate and
implement the try-catch and the 400 response handling.
🧹 Nitpick comments (1)
packages/http-router/src/parseQueryParams.test.ts (1)
39-45: Consider asserting the specific error type or message.The test verifies that an error is thrown, but doesn't verify the error type or message. This could help ensure the correct exception is being caught versus other potential errors.
Suggested improvement
it('should throw error when array limit is exceeded', () => { const largeArray = Array(501) .fill(0) .map((_, i) => `ids[]=${i}`) .join('&'); - expect(() => parseQueryParams(largeArray)).toThrow(); + expect(() => parseQueryParams(largeArray)).toThrow(/array limit/i); });
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/api/server/router.ts">
<violation number="1" location="apps/meteor/app/api/server/router.ts:48">
P2: Return the JSON response from the catch block; otherwise the handler resolves to undefined instead of a ResponseSchema.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/router.ts`:
- Around line 42-50: The error path in the handler currently logs the
parseQueryParams failure and calls c.json(...) but then uses a bare return
(return;) which yields undefined and breaks the declared
Promise<ResponseSchema<TypedOptions>> contract; replace the bare return with
returning the JSON response (i.e. return c.json({ status: 'error', message:
'Invalid query parameters' }, 400)) so the Hono response is actually returned to
the caller and the function signature is satisfied, keeping the existing
logger.warn and parseQueryParams usage intact.
3e4470c to
616525b
Compare
sampaiodiego
force-pushed
the
fix-http-array-parse
branch
from
616525b to
0f668c9
Compare
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
I noticed that the endpoint to get user presence was failing sometimes and I was able to figure out it was because the
idsparam that is supposed to be an array was being converted to an object if more than 20 ids was provided.we use
qsto parse query string params and it has a limit of 20 items by default. this is way lower than it used to be when we used Restivus, in fact I was to able to send a request with 1600 items with no problems.on this PR I'm changing the limit to 500 and adding a flag to throw in case we receive more than that, imo it is better to throw and break because of this instead of having it parsed as object and breaking weirdly because it is not an array as it is supposed (and typed) to be.
Issue(s)
ARCH-1922
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.