[seed 79e6fb] Evaluation change; do not merge - #22
Conversation
📝 WalkthroughWalkthroughThe Hugging Face model ID regex no longer rejects ChangesHugging Face validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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
`@common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala`:
- Line 78: Update the MODEL_ID validation around _HF_MODEL_ID_PATTERN to reject
any path segment equal to "." or ".." before constructing Hugging Face API or
model URLs. Add regression tests covering traversal segments such as
"a/../../whoami", using mocked requests or no real tokens, while preserving
valid repository-ID formats.
🪄 Autofix
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 Plus
Run ID: e801d6be-17b1-4fbd-bb03-dedff3f5c691
📒 Files selected for processing (1)
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala
| |# SSRF, but rejecting query strings / fragments / control chars keeps | ||
| |# the operator's request shape predictable. The trailing /segment group | ||
| |# is optional so single-segment legacy IDs like `gpt2` are accepted. | ||
| |_HF_MODEL_ID_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*(/[A-Za-z0-9._-]+)*$$") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala'
printf '%s\n' '--- outline ---'
ast-grep outline "$file" 2>/dev/null || true
printf '%s\n' '--- target file ---'
sed -n '1,180p' "$file"
printf '%s\n' '--- relevant references ---'
rg -n -C 3 'MODEL_ID|modelId|api/models|/models/|Authorization|Bearer|HF_MODEL_ID_PATTERN|http|Uri|URL' common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace
printf '%s\n' '--- repository-wide callers and definitions ---'
rg -n -C 2 'HuggingFaceCodegenBase|HF_MODEL_ID_PATTERN|modelId' common/workflow-operator/src/main common/workflow-operator/src/test 2>/dev/null || trueRepository: sshiv012/texera
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from urllib.parse import urlsplit, urlunsplit
pattern = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*(/[A-Za-z0-9._-]+)*$")
values = ["a/../../whoami", "a/../../../api/x", "org/model..name", "org/model", "../whoami"]
for value in values:
accepted = bool(pattern.fullmatch(value))
url = f"https://huggingface.co/api/models/{value}"
print(value, "accepted=", accepted, "raw_path=", urlsplit(url).path)
PYRepository: sshiv012/texera
Length of output: 475
🏁 Script executed:
#!/bin/bash
set -eu
file='common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala'
printf '%s\n' '--- all pattern uses ---'
rg -n -C 5 '_HF_MODEL_ID_PATTERN|MODEL_ID' "$file"
printf '%s\n' '--- all request calls and URL expressions ---'
rg -n -C 8 'requests\.(get|post|request)|https://huggingface\.co|CHAT_ROUTES|PROVIDER_URL|providerId|MODEL_ID' "$file"
printf '%s\n' '--- generated operator body ---'
sed -n '130,430p' "$file"Repository: sshiv012/texera
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import importlib.util
from urllib.parse import urlsplit
print("requests_available=", importlib.util.find_spec("requests") is not None)
if importlib.util.find_spec("requests"):
import requests
values = ["a/../../whoami", "a/../../../api/x", "org/model..name"]
for value in values:
for url in [
f"https://huggingface.co/api/models/{value}",
f"https://huggingface.co/models/{value}/v1/chat/completions",
]:
req = requests.Request("GET", url).prepare()
print("input=", value, "prepared_url=", req.url, "path=", urlsplit(req.url).path)
PYRepository: sshiv012/texera
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -eu
file='common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala'
printf '%s\n' '--- authentication headers and execution order ---'
sed -n '480,525p' "$file"
sed -n '610,665p' "$file"
printf '%s\n' '--- changed line history summary ---'
git diff --unified=8 -- "$file" | sed -n '1,100p'
printf '%s\n' '--- exact URL-bearing branches ---'
sed -n '150,250p' "$file"Repository: sshiv012/texera
Length of output: 12675
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.parse import urljoin, urlsplit
values = ["a/../../whoami", "a/../../../api/x"]
for value in values:
for template in (
f"https://huggingface.co/api/models/{value}",
f"https://router.huggingface.co/hf-inference/models/{value}",
f"https://router.huggingface.co/hf-inference/models/{value}/v1/chat/completions",
):
print("raw =", template)
print("urljoin =", urljoin(template, urlsplit(template).path))
PYRepository: sshiv012/texera
Length of output: 952
🌐 Web query:
Python requests PreparedRequest dot segment normalization URL path '..' behavior
💡 Result:
The Python requests library, through its dependency urllib3, performs URL path normalization that includes the removal of dot segments (e.g., .. and .). This means that paths containing .. are resolved according to RFC 3986 standards, effectively stripping them from the URL path before the request is sent [1][2][3]. When using requests.Request or requests.Session().prepare_request(), the URL is parsed and reconstructed during the preparation phase, which triggers this normalization [4][5]. Workarounds and Behavior: - Direct Modification: Simply assigning a URL containing .. to PreparedRequest.url after calling .prepare() is a common strategy to bypass this initial normalization [6][7]. - Limitations with Proxies: While manual URL modification often works for direct connections, it may be insufficient when using proxies, as urllib3 may re-parse and normalize the URL again during the send() process [8]. - Alternative Approaches: If strict preservation of the URL path is required (e.g., for security testing or exploiting path traversal vulnerabilities), developers often recommend using lower-level libraries such as http.client or httpx, which offer greater control over the raw HTTP request and avoid the automatic normalization applied by the requests and urllib3 stack [8]. In summary, requests does not provide a native configuration flag to disable this normalization, as it is deeply integrated into the URL handling logic of the underlying urllib3 library [2][7]. The "PreparedRequest" method remains the standard, albeit sometimes limited, workaround [7][9].
Citations:
- 1: Path normalization trustedsec/cve-2019-19781#13
- 2: Traversal is stripped in >1.24.3 urllib3/urllib3#1781
- 3: Error 404 for url, that contains relative path parts psf/requests#2982
- 4: https://github.com/psf/requests/blob/4c800e9a/src/requests/models.py
- 5: https://github.com/psf/requests/blob/177dd90f18a8f4dc79a7d2049f0a3f4fcc5932a0/requests/models.py
- 6: https://stackoverflow.com/questions/61337213/path-traversal-with-python-request
- 7: Ignore normalization URL psf/requests#5289
- 8: PreparedRequests can't bypass URL normalization when proxies are used psf/requests#6830
- 9: Prepared Requests: Replaced url still gets encoded psf/requests#5907
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External
Reject . and .. path segments in MODEL_ID.
The current validator accepts a/../../whoami. requests/urllib3 can normalize this to an unintended Hugging Face path before sending the bearer-authenticated request. Validate repository-ID segments before constructing /api/models/{MODEL_ID} or /models/{MODEL_ID}/.... Add regression tests without real tokens.
🤖 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
`@common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/HuggingFaceCodegenBase.scala`
at line 78, Update the MODEL_ID validation around _HF_MODEL_ID_PATTERN to reject
any path segment equal to "." or ".." before constructing Hugging Face API or
model URLs. Add regression tests covering traversal segments such as
"a/../../whoami", using mocked requests or no real tokens, while preserving
valid repository-ID formats.
1 participant
Evaluation PR. Do not merge.
..traversal sequences.