Reject unsafe filesystem paths from the HACS manifest - #5380
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens HACS manifest handling by preventing repository-provided manifest fields (filename, persistent_directory) from being used to form unsafe filesystem paths that could escape intended directories.
Changes:
- Introduces
is_safe_relative_path()and applies it to manifest path-like fields. - Enforces safe-relative-path validation in the publish-time schema (
HACS_MANIFEST_JSON_SCHEMA). - Sanitizes unsafe values at runtime in
HacsManifest.from_dict()(dropping them with a warning) and adds/extends tests covering the behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
custom_components/hacs/utils/path.py |
Adds the shared safe-relative-path helper used to gate manifest-derived paths. |
custom_components/hacs/utils/validate.py |
Updates the HACS manifest JSON schema to validate filename/persistent_directory as safe relative paths. |
custom_components/hacs/repositories/base.py |
Applies runtime sanitization of filename/persistent_directory when constructing HacsManifest from a dict. |
tests/utils/test_validate.py |
Adds schema-level tests ensuring unsafe paths and non-strings are rejected. |
tests/utils/test_path.py |
Adds unit tests for is_safe_relative_path() behavior. |
tests/repositories/test_hacs_manifest.py |
Adds runtime tests verifying unsafe values are ignored (and safe ones retained) during from_dict(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dropping just the offending value left the rest of a hostile manifest in place, and with `zip_release` still set, a dropped `filename` turned into an AttributeError in `should_try_releases`. `from_dict` now raises, so the manifest is rejected as a whole. The storage restore catches it and falls back to a default manifest, stored data can predate this check and one bad entry must not take down the restore of every other repository.
| "https://api.github.com/repos/hacs/integration/git/trees/main": 1, | ||
| "https://api.github.com/repos/hacs/integration/releases": 1 | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
There was a problem hiding this comment.
These are generated files.
There was a problem hiding this comment.
Ok.
Side note: Can we adjust the generation so that a newline at end of file is included?
| "https://api.github.com/repos/hacs/integration/git/trees/main": 1, | ||
| "https://api.github.com/repos/hacs/integration/releases": 1 | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
There was a problem hiding this comment.
These are generated files.
Proposed change
The
filenameandpersistent_directoryvalues fromhacs.jsonend up in filesystem paths ({temp_dir}/{filename}in both zip download paths, and{content.path.local}/{persistent_directory}during install). They were only validated asstr, so a hostile manifest could point them outside the intended directory with values like../../custom_components/eviland place files outside its category lane.This adds a shared
is_safe_relative_path()helper that rejects absolute paths, Windows drive paths, and any..segment (both slash styles), and applies it in two places:HACS_MANIFEST_JSON_SCHEMA: used by the HACS action, so default repository submissions with such values are rejected loudly at publish time.HacsManifest.from_dict: the single construction point for all runtime manifests (fetchedhacs.json, custom repositories, storage restore). An unsafe value rejects the whole manifest, a manifest that tries to climb out of its own directory is not to be trusted in part.That lands where you would want it at every call site: adding or updating a repository fails with the reason and the repository is not registered, downloading a specific version with an unsafe
hacs.jsonis refused, and the recurring update queue logs it and keeps the previously stored manifest.The storage restore is the one place that catches it. Stored data can predate this check, and the restore loop wraps every repository in one
except BaseException, so a single bad entry would otherwise take down the restore of every other repository. It logs a warning and falls back to a default manifest, so the unsafe value never reaches the filesystem either way.Nested relative paths like
sub/dirremain allowed, existing legitimate repositories are unaffected.For completeness: the zip extraction paths themselves were checked and are not vulnerable to zip-slip, Python's
zipfile.extractallsanitizes..components and absolute member names. This change is about the manifest values, which bypass that sanitization because they are used to build paths directly.Type of change
Checklist