fix(backend): use empty separator when joining HTTP body chunks (ACM-38039)#6528
Conversation
…38039) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
📝 WalkthroughWalkthroughRequest handlers now join streamed request-body chunks with an empty separator before JSON parsing or forwarding. Test utilities add two-chunk HTTP/2 requests, and the operator-check suite verifies the resulting behavior. ChangesRequest body chunking
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
davidaznaur
left a comment
davidaznaur
left a comment
There was a problem hiding this comment.
LGTM, thank you for updating wizard ones as well
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidaznaur, KevinFCormier, vishsanghishetty The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (1)
backend/test/routes/operatorCheck.test.ts (1)
64-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-ASCII chunk-boundary regression.
This test validates comma-free assembly, but its ASCII-only operator payload cannot detect UTF-8 corruption when a character spans chunks. Add a valid Unicode-bearing request case for one updated handler, or cover the shared body parser directly.
As per coding guidelines, tests should meaningfully cover behavior, not just achieve coverage metrics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/routes/operatorCheck.test.ts` around lines 64 - 79, Add a regression case near the existing requestMultiChunk test that sends a valid operator payload containing a non-ASCII character split across UTF-8 byte chunks, then assert the handler preserves the decoded value and returns the expected response. Reuse requestMultiChunk and the existing operator-check assertions, or test the shared body parser directly if that is the applicable boundary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/pagination.ts`:
- Line 67: Update each request-body collector to decode UTF-8 safely before JSON
parsing, avoiding string concatenation of raw chunks that can corrupt split
multibyte characters. Apply the same change at backend/src/lib/pagination.ts:67,
backend/src/routes/aggregators/appSetData.ts:36,
backend/src/routes/aggregators/statuses.ts:36,
backend/src/routes/ansibletower.ts:39, backend/src/routes/operatorCheck.ts:44,
backend/src/routes/rosaWizardApi.ts:40, backend/src/routes/rosaWizardApi.ts:76,
backend/src/routes/upgrade-risks-prediction.ts:49, and
backend/src/routes/userpreference.ts:83 by setting UTF-8 encoding on the request
stream or buffering chunks and decoding once with UTF-8 before
concatenation/JSON.parse.
---
Nitpick comments:
In `@backend/test/routes/operatorCheck.test.ts`:
- Around line 64-79: Add a regression case near the existing requestMultiChunk
test that sends a valid operator payload containing a non-ASCII character split
across UTF-8 byte chunks, then assert the handler preserves the decoded value
and returns the expected response. Reuse requestMultiChunk and the existing
operator-check assertions, or test the shared body parser directly if that is
the applicable boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e23fecec-27d2-43cb-a66c-2a541eb10dbe
📒 Files selected for processing (10)
backend/src/lib/pagination.tsbackend/src/routes/aggregators/appSetData.tsbackend/src/routes/aggregators/statuses.tsbackend/src/routes/ansibletower.tsbackend/src/routes/operatorCheck.tsbackend/src/routes/rosaWizardApi.tsbackend/src/routes/upgrade-risks-prediction.tsbackend/src/routes/userpreference.tsbackend/test/mock-request.tsbackend/test/routes/operatorCheck.test.ts
| }) | ||
| req.on('end', async () => { | ||
| const body = chucks.join() | ||
| const body = chucks.join('') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
files=(
backend/src/lib/pagination.ts
backend/src/routes/aggregators/appSetData.ts
backend/src/routes/aggregators/statuses.ts
backend/src/routes/ansibletower.ts
backend/src/routes/operatorCheck.ts
backend/src/routes/rosaWizardApi.ts
backend/src/routes/upgrade-risks-prediction.ts
backend/src/routes/userpreference.ts
)
for file in "${files[@]}"; do
echo "== $file =="
rg -n "setEncoding|join\\(['\"][\"']\\)" "$file" || true
doneRepository: stolostron/console
Length of output: 843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
backend/src/lib/pagination.ts
backend/src/routes/aggregators/appSetData.ts
backend/src/routes/aggregators/statuses.ts
backend/src/routes/ansibletower.ts
backend/src/routes/operatorCheck.ts
backend/src/routes/rosaWizardApi.ts
backend/src/routes/upgrade-risks-prediction.ts
backend/src/routes/userpreference.ts
)
for file in "${files[@]}"; do
echo "===== $file ====="
wc -l "$file"
sed -n '1,140p' "$file" | nl -ba | sed -n '1,140p'
echo
doneRepository: stolostron/console
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
backend/src/lib/pagination.ts
backend/src/routes/aggregators/appSetData.ts
backend/src/routes/aggregators/statuses.ts
backend/src/routes/ansibletower.ts
backend/src/routes/operatorCheck.ts
backend/src/routes/rosaWizardApi.ts
backend/src/routes/upgrade-risks-prediction.ts
backend/src/routes/userpreference.ts
)
for file in "${files[@]}"; do
echo "===== $file ====="
sed -n '1,160p' "$file" | cat -n
echo
doneRepository: stolostron/console
Length of output: 39148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
backend/src/lib/pagination.ts
backend/src/routes/aggregators/appSetData.ts
backend/src/routes/aggregators/statuses.ts
backend/src/routes/ansibletower.ts
backend/src/routes/operatorCheck.ts
backend/src/routes/rosaWizardApi.ts
backend/src/routes/upgrade-risks-prediction.ts
backend/src/routes/userpreference.ts
)
for file in "${files[@]}"; do
echo "===== $file ====="
sed -n '1,180p' "$file" | cat -n
echo
doneRepository: stolostron/console
Length of output: 39148
Decode request bodies before concatenation.
These handlers still collect req.on('data') chunks as strings without setEncoding('utf8') or a single Buffer.concat(...).toString('utf8') pass. Split multibyte characters can be corrupted before JSON.parse, so valid Unicode payloads may fail or round-trip incorrectly. Apply the same fix to the other body collectors in this change.
📍 Affects 8 files
backend/src/lib/pagination.ts#L67-L67(this comment)backend/src/routes/aggregators/appSetData.ts#L36-L36backend/src/routes/aggregators/statuses.ts#L36-L36backend/src/routes/ansibletower.ts#L39-L39backend/src/routes/operatorCheck.ts#L44-L44backend/src/routes/rosaWizardApi.ts#L40-L40backend/src/routes/rosaWizardApi.ts#L76-L76backend/src/routes/upgrade-risks-prediction.ts#L49-L49backend/src/routes/userpreference.ts#L83-L83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/lib/pagination.ts` at line 67, Update each request-body collector
to decode UTF-8 safely before JSON parsing, avoiding string concatenation of raw
chunks that can corrupt split multibyte characters. Apply the same change at
backend/src/lib/pagination.ts:67,
backend/src/routes/aggregators/appSetData.ts:36,
backend/src/routes/aggregators/statuses.ts:36,
backend/src/routes/ansibletower.ts:39, backend/src/routes/operatorCheck.ts:44,
backend/src/routes/rosaWizardApi.ts:40, backend/src/routes/rosaWizardApi.ts:76,
backend/src/routes/upgrade-risks-prediction.ts:49, and
backend/src/routes/userpreference.ts:83 by setting UTF-8 encoding on the request
stream or buffering chunks and decoding once with UTF-8 before
concatenation/JSON.parse.
There was a problem hiding this comment.
Will be addressed in a follow-up ticket.
There was a problem hiding this comment.
@KevinFCormier, acknowledged. The UTF-8-safe decoding improvement remains deferred from this PR. Would you like me to open and assign a GitHub follow-up issue to you with the affected request-body collectors and acceptance criteria?
🐇💭
|
3 participants
📝 Summary
Ticket Summary (Title):
Backend: HTTP request body chunks joined without empty separator can corrupt JSON payloads
Ticket Link:
https://issues.redhat.com/browse/ACM-38039
Type of Change:
Root Cause
Several backend route handlers collect HTTP request body data using
req.on('data')listeners that push chunks into an array, then join them beforeJSON.parse(). However,Array.join()was called without an explicit empty-string separator, which means JavaScript's default comma (,) separator would be used. If a request body arrives in multiple TCP chunks, the commas inserted at chunk boundaries corrupt the JSON before parsing.One location (
virtualMachineProxy.ts) already used the correct.join('')form — this PR aligns all other locations.Changes
Changed
.join()to.join('')in 9 locations across 8 backend files:backend/src/routes/rosaWizardApi.tsgetAwsAccountIds,getAwsBillingAccountIdsbackend/src/routes/userpreference.tsuserpreferencebackend/src/routes/operatorCheck.tsoperatorCheckbackend/src/routes/aggregators/statuses.tsrequestAggregatedStatusesbackend/src/routes/aggregators/appSetData.tsrequestAggregatedAppSetDatabackend/src/routes/ansibletower.tsansibleTowerbackend/src/lib/pagination.tsrequestListViewbackend/src/routes/upgrade-risks-prediction.tsupgradeRisksPredictionAdded a
requestMultiChunk()test helper and a regression test that sends a request body split across two chunks to verify correct reassembly.Context
Discovered during review of PR #6490. See this comment thread.
✅ Checklist
General
If Bugfix
🗒️ Notes for Reviewers
.join()→.join('')across all HTTP body chunk reassembly sites.utils.tsandapplicationsOCP.tsthat use.join()for cache key construction are intentionally comma-separated and were not changed.Summary by CodeRabbit
Bug Fixes
Tests