[ART-14541] attempt to make security check pass#2471
[ART-14541] attempt to make security check pass#2471pruan-rht wants to merge 3 commits intoopenshift-eng:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughReplaces simple finalUrl assembly with decoded-and-sanitized remaining-path handling: rejects Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Proxy as Proxy Handler
participant Validator as URL Validator
participant Allowlist as Allowlist Check
participant Target as Target Origin
Client->>Proxy: Request with remainingPath
Proxy->>Allowlist: lookup targetBaseUrl
alt not allowlisted
Allowlist-->>Proxy: not allowed
Proxy-->>Client: 403 Forbidden
else allowlisted
Proxy->>Validator: decode & sanitize remainingPath
alt invalid path or disallowed chars
Validator-->>Proxy: invalid
Proxy-->>Client: 400 Invalid path/target
else valid
Validator-->>Proxy: finalUrl (parsed)
Proxy->>Target: forward request to finalUrl
Target-->>Proxy: response
Proxy-->>Client: response
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@art-cluster/cloudflare/src/index.ts`:
- Around line 30-41: The current sanitization of remainingPath fails on
mixed-encoded dot segments and doesn't check control characters; update the
logic around remainingPath/secureEndpoint to first percent-decode the path
(safely catching decode errors), reject if any control characters are present,
split the decoded path into segments and validate/normalize dot-segments (reject
if any '..' remain or if normalization would escape the base path), then
construct finalUrl and enforce containment by comparing parsedFinal.pathname to
parsedBase.pathname using a normalized prefix check (e.g., ensure
parsedFinal.pathname startsWith parsedBase.pathname after normalization of
both); return 400 on any decode errors, control chars, dot-segment escapes, or
if parsedFinal.hostname/protocol/pathname fail the containment check.
|
@pruan-rht: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@art-cluster/cloudflare/src/index.ts`:
- Around line 30-39: The control-character range in the regex used to validate
decodedPath (currently /[@\\\u0000-\u001F\u007F]/) triggers the Biome lint rule;
replace that regex check with an explicit char-code scan: after obtaining
decodedPath (from remainingPath) iterate over its characters (e.g., with a for
loop or Array.from) and if any character code is <= 0x1F or === 0x7F mark
invalid, and also separately test for '@' and '\' (backslash) as before; then
use that boolean in the same conditional that currently uses segments.some(...)
so segments and decodedPath validation logic (the check that returns new
Response('Invalid path', { status: 400 })) remains functionally equivalent but
lint-compliant.
| // Sanitize remainingPath to prevent SSRF via path traversal or authority injection | ||
| let decodedPath: string; | ||
| try { | ||
| decodedPath = decodeURIComponent(remainingPath); | ||
| } catch { | ||
| return new Response('Invalid path', { status: 400 }); | ||
| } | ||
| const segments = decodedPath.split('/'); | ||
| if (segments.some((s) => s === '.' || s === '..') || /[@\\\u0000-\u001F\u007F]/.test(decodedPath)) { | ||
| return new Response('Invalid path', { status: 400 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file mentioned in the review
find . -name "index.ts" -path "*/cloudflare/src/*" | head -5Repository: openshift-eng/art-tools
Length of output: 104
🏁 Script executed:
# Check repository structure
git ls-files | grep -E "(cloudflare|biome)" | head -20Repository: openshift-eng/art-tools
Length of output: 554
🏁 Script executed:
# Look for Biome configuration
fd -e "biome.json" -e "biome.jsonc" -e ".biomeignore"Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Read the index.ts file to see the actual code
cat -n art-cluster/cloudflare/src/index.ts | head -50Repository: openshift-eng/art-tools
Length of output: 3301
🏁 Script executed:
# Check package.json for Biome and lint config
cat art-cluster/cloudflare/package.jsonRepository: openshift-eng/art-tools
Length of output: 505
🏁 Script executed:
# Check if there's a root biome config
cat biome.json 2>/dev/null || cat biome.jsonc 2>/dev/null || echo "No biome config found in root"Repository: openshift-eng/art-tools
Length of output: 1842
🏁 Script executed:
# Check if biome is available and try to run it on the cloudflare directory
which biome || npm list -g biome 2>/dev/null || echo "Biome not found globally"Repository: openshift-eng/art-tools
Length of output: 180
🏁 Script executed:
# Check if we can find biome in node_modules at root or in cloudflare
find . -name "biome" -o -name "@biomejs" 2>/dev/null | head -10Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check Biome documentation about regex and control character rules
# First, let's search for any issues or comments in the codebase about this
rg "control.*char|@.*regex|biome.*lint" -i art-cluster/cloudflare/Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Let's check what Biome rules might be triggered by searching Biome's known rules
# The regex /[@\\\u0000-\u001F\u007F]/ might trigger the 'noControlCharactersInRegex' rule
# Let's search for any documentation or comments about this in the repo
rg "noControlCharactersInRegex|control.*character" -iRepository: openshift-eng/art-tools
Length of output: 49
🌐 Web query:
Biome lint noControlCharactersInRegex rule regex control characters
💡 Result:
Biome’s lint/suspicious/noControlCharactersInRegex rule flags regex patterns that match ASCII control characters (code points 0–31, U+0000–U+001F), because they’re usually accidental and invisible. It’s recommended/enabled by default and has no autofix. [1]
What it disallows (examples)
- Hex escapes:
\x00…\x1F(e.g./\x0C/) [1] - Unicode escapes:
\u0000…\u001F(e.g./\u000C/) [1] - Code point escapes:
\u{0}…\u{1F}(e.g./\u{C}/u) [1] - Raw unescaped control characters embedded in the regex source (U+0000–U+001F) [1]
What it allows
“Control escapes” like \t and \n are allowed. [1]
Typical fixes
- If you meant whitespace/newlines: use
\t,\n,\r, or a normal space, instead of\x0C,\u000C, etc. [1] - If you truly need those control characters, you’ll need to disable the rule for that line/file or adjust your pattern design (Biome intentionally treats these as suspicious). [1]
Config
{
"linter": {
"rules": {
"suspicious": {
"noControlCharactersInRegex": "error"
}
}
}
}[1]
Sources: Biome rule docs. [1]
🏁 Script executed:
# Search for any existing issues or discussions about this in the repo
rg "u0000|u007F|control.*char" -i art-cluster/Repository: openshift-eng/art-tools
Length of output: 207
🏁 Script executed:
# Check Biome version in biome.json schema
cat biome.json | grep -i schemaRepository: openshift-eng/art-tools
Length of output: 129
Fix Biome lint failure from control‑character regex.
The regex /[@\\\u0000-\u001F\u007F]/ on line 38 contains Unicode escape sequences for control characters (U+0000–U+001F), which triggers Biome's lint/suspicious/noControlCharactersInRegex rule. This rule is enabled by default in the biome.json configuration (v2.3.14) as part of the recommended ruleset.
Replace the control‑char test with a char‑code scan to maintain the same security validation while remaining lint-compliant:
Proposed fix
- if (segments.some((s) => s === '.' || s === '..') || /[@\\\u0000-\u001F\u007F]/.test(decodedPath)) {
+ const hasControlChars = (() => {
+ for (let i = 0; i < decodedPath.length; i += 1) {
+ const code = decodedPath.charCodeAt(i);
+ if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) {
+ return true;
+ }
+ }
+ return false;
+ })();
+ if (segments.some((s) => s === '.' || s === '..') || /[@\\]/.test(decodedPath) || hasControlChars) {
return new Response('Invalid path', { status: 400 });
}🧰 Tools
🪛 Biome (2.3.14)
[error] 38-38: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 38-38: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In `@art-cluster/cloudflare/src/index.ts` around lines 30 - 39, The
control-character range in the regex used to validate decodedPath (currently
/[@\\\u0000-\u001F\u007F]/) triggers the Biome lint rule; replace that regex
check with an explicit char-code scan: after obtaining decodedPath (from
remainingPath) iterate over its characters (e.g., with a for loop or Array.from)
and if any character code is <= 0x1F or === 0x7F mark invalid, and also
separately test for '@' and '\' (backslash) as before; then use that boolean in
the same conditional that currently uses segments.some(...) so segments and
decodedPath validation logic (the check that returns new Response('Invalid
path', { status: 400 })) remains functionally equivalent but lint-compliant.
1 participant
No description provided.