Walkthrough

This PR hardens the Poly Haven asset download flow by adding path traversal prevention. The download_polyhaven_asset function now validates and normalizes each include file path before writing, rejecting unsafe paths that are absolute, contain .. references, or would escape the temporary directory.

Changes

Path Traversal Prevention in Asset Downloads

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Possibly related issues

• #257: Directly addresses the path traversal vulnerability in download_polyhaven_asset by implementing the include_path validation and normalization defense.

Poem

🐰 A path through the tunnel, so twisted and deep,
But now we check bounds before the big leap,
No .. shall escape, no absolute way,
The temp dir stays safe—security's at play! ✨

❌ Failed checks (1 warning)

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• X
• Mastodon
• Reddit
• LinkedIn

• Prevents arbitrary file write via path traversal in Poly Haven asset downloads
• Validates include paths from API responses against directory escape attempts
• Rejects absolute paths and paths containing ".." components
• Ensures resolved paths remain within temporary directory boundary

Add path traversal protection to include downloads

• Added path validation logic before downloading included files from Poly Haven API
• Normalizes include paths and checks for absolute paths and ".." components
• Verifies resolved absolute path stays within temp_dir boundary using startswith check
• Logs and skips unsafe includes instead of aborting, maintaining existing error handling convention

addon.py

Description

download_polyhaven_asset rejects any include_path containing the substring ".." anywhere, which will
also skip legitimate filenames like "albedo..png" even though they are not traversal segments. This
can break model imports by silently omitting valid dependencies while the later abspath containment
check already prevents escaping temp_dir.

Code

addon.py[R762-766]

Evidence

The new condition uses a raw substring match (".." in include_path), which will evaluate true for
any filename containing two dots, regardless of whether .. is a directory component; this is
directly shown in the updated code around the new validation block.

addon.py[748-766]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`download_polyhaven_asset` currently treats any occurrence of the substring `".."` in `include_path` as unsafe. This is overly broad: it flags valid filenames containing `..` (e.g., `albedo..png`) even when there is no directory traversal.

### Issue Context
The code already computes `abs_target_path` and checks containment under `abs_temp_dir` via a prefix check. The additional `".." in include_path` check should be narrowed to detect traversal *segments* (path components equal to `..`), not arbitrary substrings.

### Fix Focus Areas
- addon.py[754-766]

### Implementation notes
- Normalize first: `normalized = os.path.normpath(include_path)`.
- Reject absolute paths using the normalized value.
- Detect traversal by checking path parts, e.g. via `pathlib.PurePath(normalized).parts` and `any(p == ".." for p in parts)`.
- Keep (or strengthen) containment using `os.path.commonpath([abs_temp_dir, abs_target_path]) == abs_temp_dir` (optionally use `realpath` instead of `abspath` for canonicalization).
- Ensure behavior remains "skip + log" for unsafe entries, matching existing convention.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

download_polyhaven_asset recomputes abs_temp_dir inside the include loop even though temp_dir never
changes. This adds unnecessary repeated work and obscures which values are loop-invariant.

Code

addon.py[R759-761]

Evidence

The code assigns abs_temp_dir = os.path.abspath(temp_dir) inside the for include_path ... loop,
even though temp_dir is created once and not modified within the loop.

addon.py[748-766]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`abs_temp_dir = os.path.abspath(temp_dir)` is computed on every iteration of the `include` loop, despite being constant for the lifetime of `temp_dir`.

### Issue Context
This is a minor inefficiency and readability issue; hoisting the computation clarifies intent and avoids repeated calls when there are many includes.

### Fix Focus Areas
- addon.py[748-766]

### Implementation notes
- Move `abs_temp_dir = os.path.abspath(temp_dir)` (or `realpath`) to just before the `for include_path, include_info ...` loop.
- Optionally precompute `abs_temp_prefix = abs_temp_dir + os.sep` if keeping a prefix-based containment check.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools