Skip to content

Conversation

@pheus
Copy link
Contributor

@pheus pheus commented Sep 10, 2025

Fixes: #20236

Summary
Silently normalize the on‑disk filename derived from image attachments to prevent nested directories and path traversal, while preserving the user‑entered name in the database/UI unchanged.

What’s changed

  • In upload_to:
    • Normalize browser paths (e.g. C:\fakepath\photo.jpg) and extract the real uploaded extension.
    • Use the user‑entered name when present; sanitize via Django’s filename utility.
    • Prefix with {object_type.model}_{object_id} and validate the final relative path with validate_file_name(..., allow_relative_path=True).
    • Preserve only known image extensions (bmp, gif, jpeg, jpg, png, webp); otherwise save without an extension.
  • (Hardening) Guard filename parsing to avoid IndexError if older files don’t follow the expected underscore pattern.

Behavior before

  • Supplying slashes in the name could create subdirectories and trigger IndexError in code that assumes a fixed filename layout.

Behavior after

  • Files are always stored under image-attachments/ with a flat, safe basename.
  • The value shown/stored in the name field is not altered; only the on‑disk filename is normalized.

Notes for reviewers

  • This follows the maintainer guidance to silently normalize rather than reject slashes.
  • No user‑visible changes to the name field or forms.

@jeremystretch jeremystretch requested review from a team and jeremystretch and removed request for a team September 11, 2025 12:12
@jnovinger jnovinger requested review from jnovinger and removed request for jeremystretch September 12, 2025 16:32
Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

Hey, @pheus , this looks great and achieves exactly what the original issue and discussion required. I do have one ask, though, which is to include some minimal testing of ImageAttachment.filename and image_upload to establish future expectations.

Having said that, I think it's important that we ship this in the upcoming v4.4.1 release coming next Tuesday. So, if there's not enough time to add the tests before then, we can merge this as is and add tests as a follow up. Thoughts?

@pheus
Copy link
Contributor Author

pheus commented Sep 12, 2025

Thanks for the quick review and for aiming this at v4.4.1. This is much appreciated!

I’ll add a minimal test and aim to push these by Monday to leave time for review. If timing gets tight, I’m fine merging as-is and following up with a tests-only PR.

@pheus pheus force-pushed the 20236-sanitize-image-attachment-name branch from 62f9ae9 to ea2d339 Compare September 12, 2025 19:28
Refines logic for generating filenames and paths for file uploads. Uses
`Path` for cross-platform compatibility, adds sanitization, and
validates file paths to prevent security risks. Enhances support for
custom instance names and preserves specified file extensions.
Add minimal tests for `image_upload` and `ImageAttachment.filename`.

Fixes netbox-community#20236
@pheus pheus force-pushed the 20236-sanitize-image-attachment-name branch from ea2d339 to 615cad3 Compare September 12, 2025 19:31
@pheus
Copy link
Contributor Author

pheus commented Sep 12, 2025

I’ve added minimal tests covering:

  • image_upload normalization (slashes/backslashes; no nested paths) and extension handling
  • ImageAttachment.filename prefix handling and legacy names (no IndexError)
  • Windows-style inputs (C:\fake_path\...)

I hope this aligns with what you had in mind. Thanks for the review and guidance. And for maintaining NetBox.

@pheus pheus requested a review from jnovinger September 12, 2025 19:47
Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

Thanks, @pheus . Those are exactly the sorts of tests I had in mind.

@jnovinger jnovinger merged commit 2d6b3d1 into netbox-community:main Sep 12, 2025
7 checks passed
@pheus pheus deleted the 20236-sanitize-image-attachment-name branch September 12, 2025 22:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New image attachment index breaks if image name contains a slash

2 participants