Avoid storing Flickr secret-bearing URLs#46
Conversation
|
👋 Hello @glenn-jocher, thank you for submitting a
For more guidance, please refer to our Contributing Guide. Don't hesitate to leave a comment if you have any questions. Thank you for contributing to Ultralytics! 🚀 |
UltralyticsAssistant
left a comment
There was a problem hiding this comment.
🔍 PR Review
Made with ❤️ by Ultralytics Actions
Good privacy-focused cleanup overall: removing fallback URL construction and redacting CLI output are solid improvements, and the tests cover the intended behavior well. I found two correctness issues to address before merging: safe_filename_from_uri() can reintroduce path separators after decoding, and the new missing-URL guard should handle all falsey url_o values, not just None.
💬 Posted 2 inline comments
| .replace("%20", "_") | ||
| def safe_filename_from_uri(uri): | ||
| """Return a sanitized filename from a URI path without query parameters.""" | ||
| filename = unquote(Path(urlsplit(uri).path).name) or "download" |
There was a problem hiding this comment.
unquote() can reintroduce encoded path separators like %2F and %5C after you've already taken .name. That means a crafted URI such as .../..%2Fsecret.txt can turn into ../secret.txt here, and download_uri() will write outside the target directory or fail by creating nested paths. Decode first, then take the basename again from the decoded path.
Suggested change:
| filename = unquote(Path(urlsplit(uri).path).name) or "download" | |
| filename = Path(unquote(urlsplit(uri).path).replace("\\", "/")).name or "download" |
|
|
||
| for photo in photo_list: | ||
| url = build_photo_url(photo) | ||
| if url is None: |
There was a problem hiding this comment.
💡 MEDIUM: This only skips None, but build_photo_url() now returns whatever Flickr provides in url_o. If the API returns an empty string or another falsey value, we'll still append/log it and download_uri() may later try to fetch an invalid URL. Treat any falsey value as missing here.
Suggested change:
| if url is None: | |
| if not url: |
|
Merged! 🎉 Thanks @glenn-jocher for this thoughtful improvement to the Flickr scraper. As Benjamin Franklin said, “An ounce of prevention is worth a pound of cure.” This update reflects that perfectly by making scraping safer by default: relying only on Flickr-provided original URLs, skipping incomplete entries, and preventing sensitive URL details from leaking into logs or filenames. Really appreciate the care put into both the implementation and the test/documentation updates. This makes the tool more private, predictable, and trustworthy for everyone using it. |
Summary
url_oinstead of constructing fallback URLs withphoto["secret"]Security
Addresses CodeQL alert #2:
py/clear-text-storage-sensitive-dataatutils/general.py.Tests
python -m compileall -q .uv run --with-requirements requirements.txt --with pytest pytest -qgit diff --checkuvx ruff check --extend-select F,I,D,UP,RUF,FA --target-version py39 --ignore D100,D104,D203,D205,D212,D213,D401,D406,D407,D413,RUF001,RUF002,RUF012 flickr_scraper.py utils/general.py testsuv run --with-requirements requirements.txt python flickr_scraper.py --search 'honeybees on flowers' --n 1(expected missing credential path)🛠️ PR Summary
Made with ❤️ by Ultralytics Actions
🌟 Summary
This PR improves the Flickr scraper’s privacy and safety by only using Flickr-provided original image URLs, skipping incomplete entries, and preventing sensitive URL details from appearing in filenames or console output 🔒📷
📊 Key Changes
build_photo_url()now returns only Flickr’s directurl_ovalue instead of generating fallback image URLs from metadata.url_oare now skipped with a clear message:skipped (missing original URL).foundordownloaded🧹safe_filename_from_uri()helper to sanitize filenames and remove query parameters before saving files.download_uri()now uses the sanitized filename directly, reducing the chance of secrets or tokens being written to disk.🎯 Purpose & Impact