Skip to content

Validate the file name derived from origin in get_file - #23330

Open
SABITHSAHEB wants to merge 2 commits into
keras-team:masterfrom
SABITHSAHEB:get-file-origin-fname
Open

Validate the file name derived from origin in get_file#23330
SABITHSAHEB wants to merge 2 commits into
keras-team:masterfrom
SABITHSAHEB:get-file-origin-fname

Conversation

@SABITHSAHEB

@SABITHSAHEB SABITHSAHEB commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Description

  1. get_file rejects separators in the fname argument, but when fname is omitted the name comes from basename(urlsplit(origin).path), which never reaches that check.
  2. basename drops separators and still returns .. for a URL path ending in /.., so the download and extraction targets resolve one level above <cache_dir>/<cache_subdir>: with extract=True the archive members are written into cache_dir itself, over files such as keras.json, and without it get_file returns a directory and never downloads at all.
  3. Windows behaves the same way for a path ending in a drive letter, because basename keeps C: and the later join then drops datadir.

Treating those names as unparsable reuses the error the function already raises for a URL that carries no file name. The test covers the /, /. and /.. forms; like #23287 the Windows case is not tested since there is no Windows runner.

Contributor Agreement

Please review our AI-Assisted Contribution Policy and check all boxes below before submitting your PR for review:

  • I am a human, and not a bot.
  • I will be responsible for responding to review comments in a timely manner.
  • I will work with the maintainers to push this PR forward until submission.

Note: Failing to adhere to this agreement may result in your future PRs no longer being reviewed.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the security and robustness of the get_file utility in keras/src/utils/file_utils.py by validating the parsed file name from the origin URL. It prevents directory traversal and escaping of the target directory by rejecting invalid file names such as ., .., or those containing drive letters, raising a ValueError in these cases. Corresponding unit tests have been added to verify this behavior. There are no review comments, so no further feedback is provided.

@codecov-commenter

codecov-commenter commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.17%. Comparing base (abd068b) to head (103bd50).
⚠️ Report is 46 commits behind head on master.

Files with missing lines Patch % Lines
keras/src/utils/file_utils.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #23330      +/-   ##
==========================================
+ Coverage   80.46%   84.17%   +3.71%     
==========================================
  Files         466      468       +2     
  Lines       70017    70501     +484     
  Branches    11582    11663      +81     
==========================================
+ Hits        56339    59345    +3006     
+ Misses      10824     8177    -2647     
- Partials     2854     2979     +125     
Flag Coverage Δ
keras 84.00% <0.00%> (+3.72%) ⬆️
keras-cpu 84.00% <0.00%> (+4.45%) ⬆️
keras-gpu ?
keras-jax 58.11% <0.00%> (-0.08%) ⬇️
keras-numpy 53.91% <0.00%> (?)
keras-openvino 59.69% <0.00%> (+0.10%) ⬆️
keras-tensorflow 59.78% <0.00%> (-0.04%) ⬇️
keras-torch 59.10% <0.00%> (-0.04%) ⬇️
keras-tpu ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JyotinderSingh

Copy link
Copy Markdown
Collaborator

Please fill out the contributor agreement in the PR description.

@JyotinderSingh JyotinderSingh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@google-ml-butler google-ml-butler Bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Aug 1, 2026
Comment thread keras/src/utils/file_utils.py Outdated
Comment on lines 327 to 328

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. There is no gap after "'{origin}'." and before "Please specific ..." in the string builder. Would be nice to fix it as part of the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that was missing before this PR too. Added the space and pushed, the message now reads "... provided: ''. Please specify the fname argument."

@google-ml-butler google-ml-butler Bot removed the ready to pull Ready to be merged into the codebase label Aug 3, 2026
@SABITHSAHEB

Copy link
Copy Markdown
Contributor Author

Both of the review-commitment items are checked in the description, and I'll keep this one moving until it lands. The missing space you flagged is fixed in 103bd50.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants