OCPBUGS-74140: Prevent Chinese characters from rendering as unicode escape sequences in alert messages#16156
Conversation
Assisted by Claude
openshift-ci-robot
added
jira/severity-critical
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-74140, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
📝 WalkthroughWalkthroughThis pull request introduces Unicode escape handling for error messages in the console fetch utilities. A new ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![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)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts (1)
5-23: Add a malformed-escape regression test for parser safety.Nice happy-path coverage. Please add one case for invalid/out-of-range escapes (e.g.
\\UFFFFFFFF) to lock in fail-open behavior and prevent future regressions.✅ Suggested test case
describe('unescapeGoUnicode', () => { + it('should not throw on malformed or out-of-range Go unicode escapes', () => { + expect(() => unescapeGoUnicode('\\UFFFFFFFF')).not.toThrow(); + expect(unescapeGoUnicode('\\UFFFFFFFF')).toBe('\\UFFFFFFFF'); + }); + it('should unescape 4-digit Go unicode escapes', () => { expect(unescapeGoUnicode('\\ue00f')).toBe('\ue00f'); expect(unescapeGoUnicode('\\ue4c8')).toBe('\ue4c8'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts` around lines 5 - 23, Add a regression test in the unescapeGoUnicode suite to assert malformed/out-of-range escapes are preserved (fail-open). In the test file console-fetch.spec.ts near the other cases for unescapeGoUnicode, add an it block that calls unescapeGoUnicode with an invalid escape like '\\UFFFFFFFF' (and optionally other malformed examples) and expects the original input string to be returned unchanged; reference the unescapeGoUnicode function to locate where to add this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts`:
- Line 229: The 403 JSON error branch still passes raw json.message into the
HttpError, so escaped Unicode can leak; update the 403 handling to call
unescapeGoUnicode(json.message) before constructing the HttpError (same as the
other branch that throws new HttpError(unescapeGoUnicode(reason), ...)). Locate
the 403 response branch in console-fetch-utils.ts and replace the raw
json.message usage with unescapeGoUnicode(json.message) when creating the
HttpError so both error paths behave consistently.
- Around line 153-156: The unescapeGoUnicode function can throw RangeError when
String.fromCodePoint is given out-of-range values; update the replacer callbacks
in unescapeGoUnicode to parse the hex, check that the code point is a finite
integer within 0..0x10FFFF (and for \uXXXX also within 0..0xFFFF), and only call
String.fromCodePoint for valid values; if validation fails (NaN or
out-of-range), return the original matched escape sequence (the full match) so
the function fails gracefully instead of throwing.
---
Nitpick comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts`:
- Around line 5-23: Add a regression test in the unescapeGoUnicode suite to
assert malformed/out-of-range escapes are preserved (fail-open). In the test
file console-fetch.spec.ts near the other cases for unescapeGoUnicode, add an it
block that calls unescapeGoUnicode with an invalid escape like '\\UFFFFFFFF'
(and optionally other malformed examples) and expects the original input string
to be returned unchanged; reference the unescapeGoUnicode function to locate
where to add this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f0190f24-f1fd-4a05-ab5b-6299ab48b1bb
📒 Files selected for processing (2)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts
🧬 Code graph analysis (1)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts (1)
unescapeGoUnicode(153-156)
| export const unescapeGoUnicode = (str: string): string => | ||
| str | ||
| .replace(/\\U([0-9a-fA-F]{8})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16))) | ||
| .replace(/\\u([0-9a-fA-F]{4})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd 'console-fetch-utils\.ts$' | head -n1)"
echo "Inspecting $FILE"
sed -n '149,170p' "$FILE"
echo
echo "Searching for guard patterns near unescapeGoUnicode:"
rg -n -C2 'unescapeGoUnicode|fromCodePoint|0x10ffff|try\s*\{|catch\s*\(' "$FILE"Repository: openshift/console
Length of output: 1618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts"
echo "Inspecting 403 error branch (lines 190-210):"
sed -n '190,210p' "$FILE"Repository: openshift/console
Length of output: 696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts"
echo "=== All uses of unescapeGoUnicode in file ==="
rg -n 'unescapeGoUnicode' "$FILE"
echo -e "\n=== Full error handling flow (lines 160-240) to understand all paths ==="
sed -n '160,240p' "$FILE" | head -80Repository: openshift/console
Length of output: 2412
Harden unescapeGoUnicode to prevent throwing on invalid Unicode code points.
String.fromCodePoint throws RangeError for out-of-range values (> 0x10FFFF). If a malformed \UXXXXXXXX escape sequence appears in an API error message, this will propagate as an uncaught exception instead of being caught by normal error handling, resulting in a degraded user experience. Validate the code point range and fail gracefully by returning the original escape sequence.
Proposed fix
+const decodeGoUnicodeEscape = (raw: string, hex: string): string => {
+ const codePoint = Number.parseInt(hex, 16);
+ if (!Number.isFinite(codePoint) || codePoint < 0 || codePoint > 0x10ffff) {
+ return raw;
+ }
+ try {
+ return String.fromCodePoint(codePoint);
+ } catch {
+ return raw;
+ }
+};
+
export const unescapeGoUnicode = (str: string): string =>
str
- .replace(/\\U([0-9a-fA-F]{8})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16)))
- .replace(/\\u([0-9a-fA-F]{4})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16)));
+ .replace(/\\U([0-9a-fA-F]{8})/g, (raw, hex) => decodeGoUnicodeEscape(raw, hex))
+ .replace(/\\u([0-9a-fA-F]{4})/g, (raw, hex) => decodeGoUnicodeEscape(raw, hex));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const unescapeGoUnicode = (str: string): string => | |
| str | |
| .replace(/\\U([0-9a-fA-F]{8})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16))) | |
| .replace(/\\u([0-9a-fA-F]{4})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16))); | |
| const decodeGoUnicodeEscape = (raw: string, hex: string): string => { | |
| const codePoint = Number.parseInt(hex, 16); | |
| if (!Number.isFinite(codePoint) || codePoint < 0 || codePoint > 0x10ffff) { | |
| return raw; | |
| } | |
| try { | |
| return String.fromCodePoint(codePoint); | |
| } catch { | |
| return raw; | |
| } | |
| }; | |
| export const unescapeGoUnicode = (str: string): string => | |
| str | |
| .replace(/\\U([0-9a-fA-F]{8})/g, (raw, hex) => decodeGoUnicodeEscape(raw, hex)) | |
| .replace(/\\u([0-9a-fA-F]{4})/g, (raw, hex) => decodeGoUnicodeEscape(raw, hex)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts`
around lines 153 - 156, The unescapeGoUnicode function can throw RangeError when
String.fromCodePoint is given out-of-range values; update the replacer callbacks
in unescapeGoUnicode to parse the hex, check that the code point is a finite
integer within 0..0x10FFFF (and for \uXXXX also within 0..0xFFFF), and only call
String.fromCodePoint for valid values; if validation fails (NaN or
out-of-range), return the original matched escape sequence (the full match) so
the function fails gracefully instead of throwing.
| } | ||
|
|
||
| throw new HttpError(reason, response.status, response, json); | ||
| throw new HttpError(unescapeGoUnicode(reason), response.status, response, json); |
There was a problem hiding this comment.
Apply unescape consistently in the 403 JSON error path as well.
Right now unescaping is only applied in this branch; the 403 branch still uses raw json.message, so escaped Unicode can still leak to users there.
💡 Proposed fix
if (response.status === 403) {
return response.json().then((json) => {
throw new HttpError(
- json.message || 'Access denied due to cluster policy.',
+ unescapeGoUnicode(json.message || 'Access denied due to cluster policy.'),
response.status,
response,
json,
);
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts`
at line 229, The 403 JSON error branch still passes raw json.message into the
HttpError, so escaped Unicode can leak; update the 403 handling to call
unescapeGoUnicode(json.message) before constructing the HttpError (same as the
other branch that throws new HttpError(unescapeGoUnicode(reason), ...)). Locate
the 403 response branch in console-fetch-utils.ts and replace the raw
json.message usage with unescapeGoUnicode(json.message) when creating the
HttpError so both error paths behave consistently.
| } | ||
|
|
||
| throw new HttpError(reason, response.status, response, json); | ||
| throw new HttpError(unescapeGoUnicode(reason), response.status, response, json); |
There was a problem hiding this comment.
The fix was applied here but not to the existing 403 handler above (line 194). Could Unicode escapes also appear in 403 json.message responses? If so, should we apply the same treatment there for consistency?
| */ | ||
| export const unescapeGoUnicode = (str: string): string => | ||
| str | ||
| .replace(/\\U([0-9a-fA-F]{8})/g, (_, hex) => String.fromCodePoint(parseInt(hex, 16))) |
There was a problem hiding this comment.
What happens if the API returns a malformed escape like \UFFFFFFFF? String.fromCodePoint() throws a RangeError for code points above 0x10FFFF. Should we add bounds checking to fail gracefully?
| 0x2ebf0, | ||
| )}${String.fromCodePoint(0x2ebf1)}`; | ||
| expect(unescapeGoUnicode(input)).toBe(expected); | ||
| }); |
There was a problem hiding this comment.
Should we add a test case for invalid/out-of-range escape sequences to ensure the function doesn't throw unexpectedly?
|
@cajieh Thank you for the review. I pushed updates addressing the comments. |
|
@sg00dwin: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, sg00dwin 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 |
openshift-ci
bot
added
the
approved
3 participants
The K8s API server escapes certain Unicode characters (PUA, CJK Extension I) using Go-style sequences (\uXXXX, \UXXXXXXXX) in validation error messages. These were displayed as literal text instead of actual characters.
\ue00f,\U0002ebf0) as literaltext instead of actual characters
unescapeGoUnicodeutility to convert escape sequences back to Unicode characters invalidateStatusAssisted by: Claude Code
After
Before
Summary by CodeRabbit
Bug Fixes
Tests