EPMRPP-114318 || Add client-side validations and truncation#261
EPMRPP-114318 || Add client-side validations and truncation#261maria-hambardzumian wants to merge 2 commits intodevelopfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 15 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request adds comprehensive sanitization and truncation functionality to the ReportPortal client. New helper functions process launch and test item metadata—removing binary control characters and enforcing length limits on names, descriptions, and attributes. Configuration options control these behaviors, with sensible defaults and explicit length limits exported as constants. Changes
Sequence DiagramsequenceDiagram
actor Client
participant RPClient as ReportPortal Client
participant Sanitizer as Sanitizer/Helpers
participant ReportPortal as ReportPortal API
Client->>RPClient: startLaunch(launchData)
RPClient->>Sanitizer: sanitizeData(launchData, limits)
Sanitizer->>Sanitizer: cleanBinaryCharacters(name)
Sanitizer->>Sanitizer: truncateString(name, 256)
Sanitizer->>Sanitizer: truncateAttributes(attributes)
Sanitizer-->>RPClient: sanitizedData
RPClient->>ReportPortal: POST /api/v2/launch
ReportPortal-->>RPClient: launchResponse
Client->>RPClient: startTestItem(itemData)
RPClient->>Sanitizer: sanitizeData(itemData, limits)
Sanitizer->>Sanitizer: cleanBinaryCharacters(name)
Sanitizer->>Sanitizer: truncateString(name, 1024)
Sanitizer->>Sanitizer: truncateAttributes(attributes)
Sanitizer-->>RPClient: sanitizedData
RPClient->>ReportPortal: POST /api/v2/item
ReportPortal-->>RPClient: itemResponse
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/report-portal-client.js (1)
714-714:⚠️ Potential issue | 🟡 MinorDebug log shows pre-sanitization payload.
This
logDebugemitsfinishTestItemRQ(the caller's raw input) even though the actual request is built fromfinishTestItemData. Users debugging truncation/sanitization issues will see misleading output. Swap in the sanitized object:- this.logDebug(`Finish test item with tempId ${itemTempId}`, finishTestItemRQ); + this.logDebug(`Finish test item with tempId ${itemTempId}`, finishTestItemData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/report-portal-client.js` at line 714, The debug log is printing the caller's raw payload (finishTestItemRQ) instead of the sanitized request actually sent; update the this.logDebug call in the finish-test-item flow to log the sanitized object (finishTestItemData) instead of finishTestItemRQ so debugging reflects the real outgoing payload—i.e., replace the second argument to this.logDebug(`Finish test item with tempId ${itemTempId}`, ...) with finishTestItemData (keeping the same message and itemTempId variable).
🧹 Nitpick comments (5)
lib/helpers.js (2)
56-61:truncateAttributeFieldsunconditionally coercesvalueeven whenkey-only is present.
result.value = truncateString(String(result.value), ...)turns anundefined/missing value into the literal string"undefined". In practicetruncateAttributesfilters out entries without avaluebefore calling this (line 82), so it's safe today — but if that filter is ever loosened, this becomes a data-corruption bug. Consider mirroring theif (result.key)guard for symmetry and defensiveness:Proposed fix
- if (result.key) result.key = truncateString(String(result.key), ATTRIBUTE_LENGTH_LIMIT); - result.value = truncateString(String(result.value), ATTRIBUTE_LENGTH_LIMIT); + if (result.key != null) result.key = truncateString(String(result.key), ATTRIBUTE_LENGTH_LIMIT); + if (result.value != null) result.value = truncateString(String(result.value), ATTRIBUTE_LENGTH_LIMIT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/helpers.js` around lines 56 - 61, The function truncateAttributeFields currently coerces result.value unconditionally which turns undefined into the string "undefined"; update truncateAttributeFields to only call truncateString on result.value when it is present (mirror the existing if (result.key) guard), i.e., check for result.value (or result.hasOwnProperty('value') / result.value != null) before assigning result.value = truncateString(String(result.value), ATTRIBUTE_LENGTH_LIMIT), using the same ATTRIBUTE_LENGTH_LIMIT and truncateString helper.
34-42:sanitizeFieldassumes a string value; non-string inputs will throw.If a caller passes a
number/booleanasnameordescription(not impossible given the JS public API),cleanBinaryCharacterscalls.replaceon it and throws. Either coerce withString(value)at the top or document the contract:- if (!value) return value; - - let result = value; + if (value == null || value === '') return value; + let result = String(value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/helpers.js` around lines 34 - 42, sanitizeField assumes its input is a string and will throw if given non-strings because cleanBinaryCharacters and truncateString call string methods; update sanitizeField to coerce non-null/undefined inputs to strings (e.g., call String(value) at start) before calling cleanBinaryCharacters/truncateString, or alternatively validate and early-return for non-string types; reference sanitizeField, cleanBinaryCharacters and truncateString when making the change so callers supplying numbers/booleans won’t crash.lib/commons/config.js (1)
128-133: Explicit0limit silently becomes the default.
options.launchNameLengthLimit || LAUNCH_NAME_LENGTH_LIMIT(and the three siblings) treats0as "unset" and falls back to the default. If a user deliberately passes0(or any other falsy numeric), they'll be surprised. Prefer an explicitundefinedcheck:Proposed fix
- launchNameLengthLimit: options.launchNameLengthLimit || LAUNCH_NAME_LENGTH_LIMIT, - itemNameLengthLimit: options.itemNameLengthLimit || ITEM_NAME_LENGTH_LIMIT, - launchDescriptionLengthLimit: - options.launchDescriptionLengthLimit || LAUNCH_DESCRIPTION_LENGTH_LIMIT, - itemDescriptionLengthLimit: - options.itemDescriptionLengthLimit || ITEM_DESCRIPTION_LENGTH_LIMIT, + launchNameLengthLimit: + options.launchNameLengthLimit ?? LAUNCH_NAME_LENGTH_LIMIT, + itemNameLengthLimit: options.itemNameLengthLimit ?? ITEM_NAME_LENGTH_LIMIT, + launchDescriptionLengthLimit: + options.launchDescriptionLengthLimit ?? LAUNCH_DESCRIPTION_LENGTH_LIMIT, + itemDescriptionLengthLimit: + options.itemDescriptionLengthLimit ?? ITEM_DESCRIPTION_LENGTH_LIMIT,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commons/config.js` around lines 128 - 133, The current assignments for launchNameLengthLimit, itemNameLengthLimit, launchDescriptionLengthLimit, and itemDescriptionLengthLimit use the || operator so falsy numeric values like 0 fall back to defaults; change each to use an explicit undefined check on the corresponding options property (e.g., options.launchNameLengthLimit !== undefined ? options.launchNameLengthLimit : LAUNCH_NAME_LENGTH_LIMIT) so an explicit 0 is preserved.lib/report-portal-client.js (1)
81-97:sanitizeDatamutates its input.All current call sites already pass a freshly spread object, so this is safe today. Given the helper is named generically and may be reused, consider making it non-mutating to avoid future foot-guns:
Proposed fix
- sanitizeData(data, { nameLimit, descriptionLimit } = {}) { + sanitizeData(data, { nameLimit, descriptionLimit } = {}) { + const result = { ...data }; const opts = { replaceBinaryChars: this.config.replaceBinaryChars, truncateFields: this.config.truncateFields, truncateAttributes: this.config.truncateAttributes, }; - if (nameLimit && data.name != null) { - data.name = helpers.sanitizeField(data.name, nameLimit, opts); + if (nameLimit && result.name != null) { + result.name = helpers.sanitizeField(result.name, nameLimit, opts); } - if (descriptionLimit && data.description != null) { - data.description = helpers.sanitizeField(data.description, descriptionLimit, opts); + if (descriptionLimit && result.description != null) { + result.description = helpers.sanitizeField(result.description, descriptionLimit, opts); } - if (data.attributes) { - data.attributes = helpers.truncateAttributes(data.attributes, opts); + if (result.attributes) { + result.attributes = helpers.truncateAttributes(result.attributes, opts); } - return data; + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/report-portal-client.js` around lines 81 - 97, sanitizeData currently mutates its input object; make it non-mutating by creating and operating on a shallow copy at the start (e.g., const sanitized = { ...data }) and return that copy instead of mutating `data`; when adjusting nested fields ensure you replace properties on the copy (sanitized.name, sanitized.description) and assign sanitized.attributes = helpers.truncateAttributes(...) (or a cloned/processed attributes array) so helpers.sanitizeField and helpers.truncateAttributes are applied to the copy while still using this.config.replaceBinaryChars / truncateFields / truncateAttributes, then return the sanitized copy.__tests__/helpers.spec.js (1)
160-168: Consider extending coverage to the full C0+DEL control range.The "all purely binary control characters" test only probes
0x00-0x07and0x11-0x1B, which is exactly what the current regex matches — so regex gaps (e.g.\x08,\x0B,\x0C,\x0E-\x10,\x1C-\x1F,\x7F) go undetected. See the related comment onBINARY_CHARSinlib/helpers.js. Expanding this fixture would guard against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/helpers.spec.js` around lines 160 - 168, Update the test for helpers.cleanBinaryCharacters to cover the full C0 control range plus DEL so regex regressions are caught: construct binaryCodes to include 0x00–0x1F and 0x7F but exclude the allowed whitespace controls (0x09, 0x0A, 0x0D), then build input via String.fromCharCode and assert the result is '\uFFFD' repeated for that full set; reference helpers.cleanBinaryCharacters and the BINARY_CHARS behavior when updating the fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/helpers.js`:
- Around line 63-83: truncateAttributes currently sorts the entire attributes
array and slices to ATTRIBUTE_NUMBER_LIMIT which can drop system attributes
appended later; change truncateAttributes to first partition attributes (use
isAttributeObject and attribute.system === true) into systemEntries and
userEntries, sort and apply compareAttributesByKey and the
ATTRIBUTE_NUMBER_LIMIT cap only to userEntries, then recombine systemEntries
(keeping them first or always included) with the capped userEntries before
applying replaceBinaryChars (cleanAttributeBinaryChars) and
truncateAttributeFields; ensure non-object attributes handling and the
truncateEnabled flag behavior remain unchanged.
- Around line 21-26: The BINARY_CHARS regex in lib/helpers.js is missing many
control characters (e.g. \x08, \x0B-\x0F, \x10, \x1C-\x1F, \x7F), so update the
BINARY_CHARS pattern used by cleanBinaryCharacters to match all C0 control bytes
except the common whitespace controls (TAB \x09, LF \x0A, CR \x0D) — i.e.
replace the current ranges with a single pattern that targets
\x00-\x08\x0B\x0C\x0E-\x1F\x7F (or equivalently whitelist only \t\n\r and
replace everything else in the 0x00-0x1F/0x7F set) and keep
cleanBinaryCharacters behavior of replacing matches with '\uFFFD'; also extend
the tests in __tests__/helpers.spec.js to assert removal/replacement for the
previously uncovered bytes (\x08, \x0B, \x0C, \x0E-\x1F, \x7F) so the gap is
covered.
---
Outside diff comments:
In `@lib/report-portal-client.js`:
- Line 714: The debug log is printing the caller's raw payload
(finishTestItemRQ) instead of the sanitized request actually sent; update the
this.logDebug call in the finish-test-item flow to log the sanitized object
(finishTestItemData) instead of finishTestItemRQ so debugging reflects the real
outgoing payload—i.e., replace the second argument to this.logDebug(`Finish test
item with tempId ${itemTempId}`, ...) with finishTestItemData (keeping the same
message and itemTempId variable).
---
Nitpick comments:
In `@__tests__/helpers.spec.js`:
- Around line 160-168: Update the test for helpers.cleanBinaryCharacters to
cover the full C0 control range plus DEL so regex regressions are caught:
construct binaryCodes to include 0x00–0x1F and 0x7F but exclude the allowed
whitespace controls (0x09, 0x0A, 0x0D), then build input via String.fromCharCode
and assert the result is '\uFFFD' repeated for that full set; reference
helpers.cleanBinaryCharacters and the BINARY_CHARS behavior when updating the
fixture.
In `@lib/commons/config.js`:
- Around line 128-133: The current assignments for launchNameLengthLimit,
itemNameLengthLimit, launchDescriptionLengthLimit, and
itemDescriptionLengthLimit use the || operator so falsy numeric values like 0
fall back to defaults; change each to use an explicit undefined check on the
corresponding options property (e.g., options.launchNameLengthLimit !==
undefined ? options.launchNameLengthLimit : LAUNCH_NAME_LENGTH_LIMIT) so an
explicit 0 is preserved.
In `@lib/helpers.js`:
- Around line 56-61: The function truncateAttributeFields currently coerces
result.value unconditionally which turns undefined into the string "undefined";
update truncateAttributeFields to only call truncateString on result.value when
it is present (mirror the existing if (result.key) guard), i.e., check for
result.value (or result.hasOwnProperty('value') / result.value != null) before
assigning result.value = truncateString(String(result.value),
ATTRIBUTE_LENGTH_LIMIT), using the same ATTRIBUTE_LENGTH_LIMIT and
truncateString helper.
- Around line 34-42: sanitizeField assumes its input is a string and will throw
if given non-strings because cleanBinaryCharacters and truncateString call
string methods; update sanitizeField to coerce non-null/undefined inputs to
strings (e.g., call String(value) at start) before calling
cleanBinaryCharacters/truncateString, or alternatively validate and early-return
for non-string types; reference sanitizeField, cleanBinaryCharacters and
truncateString when making the change so callers supplying numbers/booleans
won’t crash.
In `@lib/report-portal-client.js`:
- Around line 81-97: sanitizeData currently mutates its input object; make it
non-mutating by creating and operating on a shallow copy at the start (e.g.,
const sanitized = { ...data }) and return that copy instead of mutating `data`;
when adjusting nested fields ensure you replace properties on the copy
(sanitized.name, sanitized.description) and assign sanitized.attributes =
helpers.truncateAttributes(...) (or a cloned/processed attributes array) so
helpers.sanitizeField and helpers.truncateAttributes are applied to the copy
while still using this.config.replaceBinaryChars / truncateFields /
truncateAttributes, then return the sanitized copy.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7422e7f1-c78a-4528-9328-09774f64cb23
📒 Files selected for processing (7)
__tests__/helpers.spec.js__tests__/report-portal-client.spec.jsindex.d.tslib/commons/config.jslib/constants/limits.jslib/helpers.jslib/report-portal-client.js
| const truncateAttributes = (attributes, options = {}) => { | ||
| const { replaceBinaryChars = true, truncateAttributes: truncateEnabled = true } = options; | ||
|
|
||
| if (!Array.isArray(attributes)) return attributes; | ||
|
|
||
| let result = attributes.filter(isAttributeObject).map((attr) => ({ ...attr })); | ||
| if (result.length === 0) return []; | ||
|
|
||
| if (result.length > ATTRIBUTE_NUMBER_LIMIT) { | ||
| result.sort(compareAttributesByKey); | ||
| result = result.slice(0, ATTRIBUTE_NUMBER_LIMIT); | ||
| } | ||
|
|
||
| if (replaceBinaryChars) { | ||
| result = result.map(cleanAttributeBinaryChars); | ||
| } | ||
|
|
||
| if (!truncateEnabled) return result; | ||
|
|
||
| return result.filter((attr) => attr.value != null).map(truncateAttributeFields); | ||
| }; |
There was a problem hiding this comment.
Sort-then-cap can drop system attributes.
When a caller supplies >256 attributes, the list is sorted by key and sliced to 256. Since startLaunch appends system attributes (client, os, skippedIssue, etc.) after user attributes, those system attributes — which downstream consumers rely on for client/OS/version reporting — may be silently discarded depending on alphabetic ordering of user keys. Consider partitioning: always keep system: true entries (or keep them first) and apply the 256-cap only to non-system entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/helpers.js` around lines 63 - 83, truncateAttributes currently sorts the
entire attributes array and slices to ATTRIBUTE_NUMBER_LIMIT which can drop
system attributes appended later; change truncateAttributes to first partition
attributes (use isAttributeObject and attribute.system === true) into
systemEntries and userEntries, sort and apply compareAttributesByKey and the
ATTRIBUTE_NUMBER_LIMIT cap only to userEntries, then recombine systemEntries
(keeping them first or always included) with the capped userEntries before
applying replaceBinaryChars (cleanAttributeBinaryChars) and
truncateAttributeFields; ensure non-object attributes handling and the
truncateEnabled flag behavior remain unchanged.
1 participant
https://jiraeu.epam.com/browse/EPMRPP-114318
Summary by CodeRabbit
New Features
Tests