refactor(backend): extract shared readRequestBody utility with UTF-8 safety (ACM-38043)#6529
refactor(backend): extract shared readRequestBody utility with UTF-8 safety (ACM-38043)#6529KevinFCormier wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughIntroduces shared helpers for reading UTF-8 request streams and parsing JSON bodies, then replaces duplicated ChangesRequest body parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KevinFCormier 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: 2
🧹 Nitpick comments (1)
backend/src/lib/body-parser.ts (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive request-body parameter names.
backend/src/lib/body-parser.ts#L90-L90: renamereqtorequest.backend/src/routes/operatorCheck.ts#L39-L39: renamedatatobodyString.backend/src/routes/userpreference.ts#L78-L78: renamedatatobodyString.backend/src/routes/virtualMachineProxy.ts#L126-L126: renamebodyStrtobodyString.As per coding guidelines, “Functions and variables must use descriptive camelCase names and avoid abbreviations.”
🤖 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/body-parser.ts` at line 90, Rename the request-body variables to descriptive camelCase names and update all references: use request in readRequestBody, bodyString in the operatorCheck and userpreference route handlers, and bodyString instead of bodyStr in virtualMachineProxy. Apply these changes in backend/src/lib/body-parser.ts:90, backend/src/routes/operatorCheck.ts:39, backend/src/routes/userpreference.ts:78, and backend/src/routes/virtualMachineProxy.ts:126.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/body-parser.ts`:
- Around line 90-100: The readRequestBody function must enforce a default
request-body byte limit before accumulating chunks. Track UTF-8 byte length,
stop or destroy the stream when the limit is exceeded, reject with an
identifiable size-limit error, and ensure callers propagate that error as HTTP
413 through their existing error paths.
In `@backend/src/lib/pagination.ts`:
- Line 63: Handle rejected body-reader and JSON-parsing promises by adding
terminal catch handling to each detached parser chain:
backend/src/lib/pagination.ts:63-63,
backend/src/routes/aggregators/statuses.ts:32-32,
backend/src/routes/rosaWizardApi.ts:34-34 and 61-61,
backend/src/routes/upgrade-risks-prediction.ts:44-44,
backend/src/routes/aggregators/appSetData.ts:30-30,
backend/src/routes/ansibletower.ts:33-33,
backend/src/routes/operatorCheck.ts:39-39,
backend/src/routes/userpreference.ts:78-78, and
backend/src/routes/virtualMachineProxy.ts:126-126. In each route’s existing
request-processing flow, emit the appropriate error response from the catch
handler, including the inner chain in ansibletower and preserving route-specific
responses; do not rely on surrounding try/catch blocks to catch detached
promises.
---
Nitpick comments:
In `@backend/src/lib/body-parser.ts`:
- Line 90: Rename the request-body variables to descriptive camelCase names and
update all references: use request in readRequestBody, bodyString in the
operatorCheck and userpreference route handlers, and bodyString instead of
bodyStr in virtualMachineProxy. Apply these changes in
backend/src/lib/body-parser.ts:90, backend/src/routes/operatorCheck.ts:39,
backend/src/routes/userpreference.ts:78, and
backend/src/routes/virtualMachineProxy.ts:126.
🪄 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: 38037012-b1d1-4247-9e2b-4bebb3acecc0
📒 Files selected for processing (10)
backend/src/lib/body-parser.tsbackend/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/src/routes/virtualMachineProxy.ts
| const body = chucks.join('') | ||
|
|
||
| const request = JSON.parse(body) as IRequestListView | ||
| void parseRequestJsonBody<IRequestListView>(req).then(async (request) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Handle rejected body-reader promises.
Each void read/parseRequestBody(...).then(...) discards stream and JSON parsing rejections. Invalid JSON or a request-stream error can leave the request unanswered and produce an unhandled rejection. Add a terminal .catch() that emits the route-appropriate error response.
backend/src/lib/pagination.ts#L63-L63: handle parser rejection before pagination processing.backend/src/routes/aggregators/statuses.ts#L32-L32: handle parser rejection before status aggregation.backend/src/routes/rosaWizardApi.ts#L34-L34: attach rejection handling; the outertry/catchcannot catch this detached chain.backend/src/routes/rosaWizardApi.ts#L61-L61: attach rejection handling; the outertry/catchcannot catch this detached chain.backend/src/routes/upgrade-risks-prediction.ts#L44-L44: handle parser rejection before prediction requests.backend/src/routes/aggregators/appSetData.ts#L30-L30: handle body-read rejection.backend/src/routes/ansibletower.ts#L33-L33: handle invalid JSON/body-read rejection in the inner chain.backend/src/routes/operatorCheck.ts#L39-L39: handle body-read rejection.backend/src/routes/userpreference.ts#L78-L78: handle body-read and POST JSON parsing rejection.backend/src/routes/virtualMachineProxy.ts#L126-L126: handle body-read rejection.
📍 Affects 9 files
backend/src/lib/pagination.ts#L63-L63(this comment)backend/src/routes/aggregators/statuses.ts#L32-L32backend/src/routes/rosaWizardApi.ts#L34-L34backend/src/routes/rosaWizardApi.ts#L61-L61backend/src/routes/upgrade-risks-prediction.ts#L44-L44backend/src/routes/aggregators/appSetData.ts#L30-L30backend/src/routes/ansibletower.ts#L33-L33backend/src/routes/operatorCheck.ts#L39-L39backend/src/routes/userpreference.ts#L78-L78backend/src/routes/virtualMachineProxy.ts#L126-L126
🤖 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 63, Handle rejected body-reader and
JSON-parsing promises by adding terminal catch handling to each detached parser
chain: backend/src/lib/pagination.ts:63-63,
backend/src/routes/aggregators/statuses.ts:32-32,
backend/src/routes/rosaWizardApi.ts:34-34 and 61-61,
backend/src/routes/upgrade-risks-prediction.ts:44-44,
backend/src/routes/aggregators/appSetData.ts:30-30,
backend/src/routes/ansibletower.ts:33-33,
backend/src/routes/operatorCheck.ts:39-39,
backend/src/routes/userpreference.ts:78-78, and
backend/src/routes/virtualMachineProxy.ts:126-126. In each route’s existing
request-processing flow, emit the appropriate error response from the catch
handler, including the inner chain in ansibletower and preserving route-specific
responses; do not rely on surrounding try/catch blocks to catch detached
promises.
|
…safety (ACM-38043) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
0077b22 to
b3dbb65
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/src/lib/body-parser.ts (1)
108-112: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not destroy the HTTP/2 stream before returning 413.
Line 111 destroys the underlying
Http2Stream, so callers cannot reliably send a response after the overflow rejection. The suppliedpaginateconsumer also currently maps this error to its generic 500 path. Drain/stop buffering without destroying the request, then mapRequestBodyTooLargeErrorto HTTP 413 in every consumer. Node documents that destroying anHttp2ServerRequestdestroys its underlying stream. (nodejs.org)🤖 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/body-parser.ts` around lines 108 - 112, Update the request handling around the data listener and RequestBodyTooLargeError: stop buffering and reject on overflow without calling req.destroy(), preserving the stream so a 413 response can be sent. In every consumer, including paginate, explicitly map RequestBodyTooLargeError to HTTP 413 instead of the generic 500 path.
🧹 Nitpick comments (2)
backend/src/lib/body-parser.ts (1)
103-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive parameter names.
Rename
reqtorequestin both exported helpers. As per coding guidelines, “Name functions and variables using descriptive camelCase names and avoid abbreviations.”🤖 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/body-parser.ts` around lines 103 - 130, Rename the req parameter to request in both exported helpers, readRequestBody and parseRequestJsonBody, and update every reference within those functions to use the new descriptive name without changing behavior.Source: Coding guidelines
backend/src/routes/ansibletower.ts (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive request and error identifiers.
Replace
towerReqwithtowerRequest, omit unusederr, and renameetoerror.As per coding guidelines, functions and variables must use descriptive camelCase names and avoid abbreviations.
Also applies to: 59-72
🤖 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/routes/ansibletower.ts` around lines 37 - 40, Update the ansible tower route’s request and error identifiers: rename towerReq to towerRequest, remove the unused err binding in the URL parsing catch, and rename e to error in the related error-handling blocks around the tower request flow.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/routes/ansibletower.ts`:
- Line 56: Update the HTTPS request configuration in the Ansible Tower route to
remove the rejectUnauthorized: false override. Configure the AAP CA through the
appropriate trusted certificate setting and keep TLS certificate validation
enabled for the credential-bearing request.
- Around line 33-74: Update the route handler containing readRequestBody to use
the required (req: Http2ServerRequest, res: Http2ServerResponse): Promise<void>
signature, and return the readRequestBody(req).then(...).catch(...) chain
instead of prefixing it with void, so the framework can await all request
processing.
---
Duplicate comments:
In `@backend/src/lib/body-parser.ts`:
- Around line 108-112: Update the request handling around the data listener and
RequestBodyTooLargeError: stop buffering and reject on overflow without calling
req.destroy(), preserving the stream so a 413 response can be sent. In every
consumer, including paginate, explicitly map RequestBodyTooLargeError to HTTP
413 instead of the generic 500 path.
---
Nitpick comments:
In `@backend/src/lib/body-parser.ts`:
- Around line 103-130: Rename the req parameter to request in both exported
helpers, readRequestBody and parseRequestJsonBody, and update every reference
within those functions to use the new descriptive name without changing
behavior.
In `@backend/src/routes/ansibletower.ts`:
- Around line 37-40: Update the ansible tower route’s request and error
identifiers: rename towerReq to towerRequest, remove the unused err binding in
the URL parsing catch, and rename e to error in the related error-handling
blocks around the tower request flow.
🪄 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: b4fc0004-e325-4d63-b0b1-8b563c978cc8
📒 Files selected for processing (10)
backend/src/lib/body-parser.tsbackend/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/src/routes/virtualMachineProxy.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/src/routes/aggregators/appSetData.ts
- backend/src/routes/aggregators/statuses.ts
- backend/src/routes/userpreference.ts
- backend/src/routes/upgrade-risks-prediction.ts
- backend/src/routes/virtualMachineProxy.ts
- backend/src/lib/pagination.ts
- backend/src/routes/rosaWizardApi.ts
- backend/src/routes/operatorCheck.ts
|
@KevinFCormier: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
📝 Summary
Ticket Summary (Title):
Backend: refactor repeated HTTP body chunk collection into shared utility with proper UTF-8 handling
Ticket Link:
https://issues.redhat.com/browse/ACM-38043
Type of Change:
What
Extracts the repeated HTTP request body chunk-collection pattern into two shared utilities in
backend/src/lib/body-parser.ts:readRequestBody(req)— reads the full request body as a UTF-8 string, callingreq.setEncoding('utf8')so Node's internalStringDecodersafely handles multi-byte character boundaries across chunksparseRequestJsonBody<T>(req)— reads the body thenJSON.parses it, returning a typed resultWhy
10 backend route handlers duplicated the same ~6-line chunk-collection pattern with minor variable name variations (
chucksvschunks,datavsbody). This duplication was:.join()bug fixed in PR #6528 existed in 9 of 10 locations precisely because of copy-pastesetEncoding('utf8'),dataevents emitBufferobjects. If a multi-byte UTF-8 character is split across chunks, calling.toString()on each chunk individually corrupts it. The new utility sets UTF-8 encoding on the stream so Node'sStringDecoderhandles this correctly.Follow-up to ACM-38039 / PR #6528.
Changes
body-parser.tsreadRequestBody()andparseRequestJsonBody<T>()ansibletower.tsreadRequestBody→ manualJSON.parseoperatorCheck.tsreadRequestBody(preserves try/catch around parse)rosaWizardApi.ts(×2)parseRequestJsonBody<Payload>statuses.tsparseRequestJsonBody<IRequestStatuses>appSetData.tsreadRequestBody(preserves try/catch around parse)pagination.tsparseRequestJsonBody<IRequestListView>upgrade-risks-prediction.tsparseRequestJsonBody<UpgradeRiskBody>userpreference.tsreadRequestBody(raw string needed for PUT forwarding)virtualMachineProxy.tsreadRequestBody(preserves try/catch around parse)Net result: +53/−79 lines (26 lines removed).
How to test
npm run test:backend— all 337 tests passnpm run check:backend— prettier, eslint, tsc all clean✅ Checklist
General
🗒️ Notes for Reviewers
userpreference.tsfor PUT forwarding,operatorCheck.tswith try/catch around parse) usereadRequestBody. Handlers that always JSON.parse useparseRequestJsonBody<T>.voidoperator prefixes the.then()calls to satisfy@typescript-eslint/no-floating-promises.Summary by CodeRabbit