fix: reject unsafe JFrog credential targets#1490
Conversation
|
@codex review |
Performance BenchmarksCompared
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a476aafcca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 340a4f9677
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parsed_ip = None | ||
|
|
||
| return parsed_ip is not None and parsed_ip.is_loopback | ||
| return parsed_ip is not None and (parsed_ip.is_loopback or parsed_ip.is_unspecified) |
There was a problem hiding this comment.
When MODELAUDIT_JFROG_ALLOWED_HOSTS contains an IPv4-mapped IPv6 literal like [::ffff:127.0.0.1], this check still returns false on Python 3.12 because IPv6Address.is_loopback does not consider the mapped IPv4 address, so the host remains in _get_trusted_jfrog_hosts() and download_artifact() sends explicit or env JFrog credentials to loopback. Fresh evidence beyond the earlier numeric-alias thread: the new regression added in this commit fails under uv run pytest tests/integrations/test_jfrog.py -q for [::ffff:127.0.0.1].
Useful? React with 👍 / 👎.
| parsed = urlparse(url) | ||
| hostname = _normalize_hostname(parsed.hostname or "") | ||
| if parsed.scheme != "https" and not (parsed.scheme == "http" and _is_local_jfrog_host(hostname)): | ||
| if parsed.scheme != "https": |
There was a problem hiding this comment.
After this change, plaintext or local JFrog-looking URLs return false here, so the CLI no longer enters the JFrog redaction path and falls through to the generic Path does not exist: {path} error in modelaudit/cli.py; for inputs such as https://user:pass@localhost/artifactory/repo/model.bin?token=... or http://localhost/..., the rejected URL is echoed with embedded credentials/query tokens instead of using redact_jfrog_url_for_display(). This is a regression for local JFrog URLs that were previously recognized as JFrog and redacted before failing the download path.
Useful? React with 👍 / 👎.
|
|
||
| def _raise_for_jfrog_redirect(response: requests.Response, source_url: str) -> None: | ||
| """Reject redirects so JFrog credentials never cross origin boundaries.""" | ||
| if response.status_code in {301, 302, 303, 307, 308}: |
There was a problem hiding this comment.
This redirect rejection is unconditional, so even public/anonymous JFrog artifacts with no explicit or environment credentials now fail as Refusing redirect... instead of following normal Artifactory CDN/object-storage redirects and scanning the file. The stated guard is to keep JFrog credentials from crossing origins, so the redirect should only be blocked when credentials are actually being sent; otherwise this breaks previously working public repositories that rely on redirects.
Useful? React with 👍 / 👎.
ianw-oai
left a comment
ianw-oai
left a comment
There was a problem hiding this comment.
Reviewed; this looks acceptable.
2 participants
Summary
Validation