Fix / Image URL fetch validates the first FQDN then follows redirects to internal targets#2501
Open
grzegorz-roboflow wants to merge 16 commits into
Open
Fix / Image URL fetch validates the first FQDN then follows redirects to internal targets#2501grzegorz-roboflow wants to merge 16 commits into
grzegorz-roboflow wants to merge 16 commits into
Conversation
…returning 302 Location: http://169.254.169.254/latest/meta-data, then expects load_image_from_url() to reject it before fetching the redirected URL. current code follows the redirect and decodes the mocked image bytes (resulting in test failure)
grzegorz-roboflow
requested review from
PawelPeczek-Roboflow,
dkosowski87,
hansent,
probicheaux,
rafel-roboflow and
yeldarby
as code owners
PawelPeczek-Roboflow
requested changes
Jun 19, 2026
PawelPeczek-Roboflow
left a comment
PawelPeczek-Roboflow
left a comment
Collaborator
There was a problem hiding this comment.
requests a bit more validation
Collaborator
Author
|
Upon reflection we can simplify this change by disabling redirects and guarding access to non-public networks behind config flag |
Disable automatic redirects for URL image fetches, reject hosts that resolve to non-public addresses by default, and add an opt-in env flag for deployments that intentionally load images from private networks. Add unit coverage for redirect blocking, metadata-address DNS resolution, and the private-network override.
Collaborator
Author
|
Pivot - disable automatic redirects for URL image fetches, reject hosts that resolve to non-public addresses by default, and add an opt-in env flag for deployments that intentionally load images from private networks. Add unit coverage for redirect blocking, metadata-address DNS resolution, and the private-network override. |
PawelPeczek-Roboflow
requested changes
Jul 3, 2026
PawelPeczek-Roboflow
left a comment
PawelPeczek-Roboflow
left a comment
Collaborator
There was a problem hiding this comment.
I would do the following:
- it's likely people use
load_image_from_url(...)so do not make BC and havemax_redirectswith default value MAX_IMAGE_URL_REDIRECTSshould align withrequests.Sessiondefault (30 from what I checked)- we have 2 other places where equivalent function exists - both in
inference_sdk- onerequestsbased, the otherasynciobased - in those we do not whitelist domains etc - we should do what we did in
inference.coreand have the same env variable to configure behaviour load_image_from_urlis strange whenMAX_IMAGE_URL_REDIRECTSis set - looks like maybe we lack some control flag?
when VALIDATE_IMAGE_URL_REDIRECTS is set False - we should have warning and information when it's going to change
Collaborator
|
let's discuss the change |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fix GHSA-hjmm-hr52-vrp2
Type of Change
Testing
Checklist
Additional Context
N/A