feat: add Hugging Face fetch timeout flag#33
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a Changes--timeout flag for remote Hugging Face fetches
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| req = urllib.request.Request(url, headers=headers) | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=10) as response: | ||
| with urllib.request.urlopen(req, timeout=timeout) as response: |
There was a problem hiding this comment.
Potential user input in HTTP request may allow SSRF attack - medium severity
If an attacker can control the URL input leading into this HTTP request, the attack might be able to perform an SSRF attack. This kind of attack is even more dangerous if the application returns the response of the request to the user. It could allow them to retrieve information from higher privileged services within the network (such as the metadata service, which is commonly available in cloud services, and could allow them to retrieve credentials).
Show fix
Remediation: If possible, only allow requests to allowlisting domains. If not, consult the article linked above to learn about other mitigating techniques such as disabling redirects, blocking private IPs and making sure private services have internal authentication. If you return data coming from the request to the user, validate the data before returning it to make sure you don't return random data.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
|
|
||
| def _positive_float(value: str) -> float: | ||
| fvalue = float(value) | ||
| if fvalue <= 0: |
There was a problem hiding this comment.
_positive_float allows NaN because fvalue <= 0 is false for NaN, so invalid --timeout nan passes parsing and can break request timeout handling.
Details
✨ AI Reasoning
The new timeout validator is meant to enforce a strictly positive value, but it only checks whether the parsed float is less than or equal to zero. A NaN value bypasses that condition because NaN is neither <= 0 nor > 0. That means an invalid timeout can pass argument parsing and propagate into request logic, where timeout handling may raise runtime errors. This is a control-flow validation bug in the new logic.
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
![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 `@src/modelinfo/cli.py`:
- Around line 44-48: The _positive_float function in src/modelinfo/cli.py
currently accepts non-finite values like nan and inf, which can cause issues
downstream. Add a check after converting the string to float using
math.isfinite() to validate that the value is finite, and raise
argparse.ArgumentTypeError with an appropriate message if it is not (for
example, "timeout must be a finite number"). This validation should occur
alongside the existing check for positive values to ensure all invalid timeout
values are rejected during argument parsing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2f3d51d-d292-418f-9b0a-bd62151f8f38
📒 Files selected for processing (3)
src/modelinfo/cli.pysrc/modelinfo/parsers/huggingface.pytests/test_cli.py
1 participant
Adds a
--timeoutflag for remote Hugging Face fetches. The default stays at 10 seconds, and the value is passed through the metadata, config, index, shard header, and HEAD requests.I also added CLI coverage for the default/custom timeout values, invalid values, and the remote analysis path passing the timeout into the Hugging Face parser.
I couldn't run the test suite from this environment, so please treat the tests as unverified locally.
Closes #27
Summary by CodeRabbit
Release Notes
New Features
--timeoutCLI option to control the timeout for remote Hugging Face model downloads.Tests
--timeout, including default behavior, valid float handling, and rejection of invalid values.