Repository navigation
fix: validate hostnames and paths built from scenario metadata - #154
Open
adamsrnmsu wants to merge 1 commit into
Open
adamsrnmsu wants to merge 1 commit into
adamsrnmsu wants to merge 1 commit into
Conversation
This was referenced Oct 1, 2026
adamsrnmsu
force-pushed
the
fix-metadata-path-validation
branch
from
October 2, 2026 01:51
4470825 to
4d927f7
Compare
7 tasks done
adamsrnmsu
force-pushed
the
fix-metadata-path-validation
branch
from
October 2, 2026 02:03
4d927f7 to
1d30bbb
Compare
GhostofGoes
suggested changes
Oct 3, 2026
GhostofGoes
left a comment
GhostofGoes
left a comment
Contributor
There was a problem hiding this comment.
Nice work, a good security and sanity pass. A few nits. Also make sure you test :)
| - **SCEPTRE App**: A `fep` without a mgmt interface raised `UnboundLocalError`, or reused the previous fep's endpoints. | ||
| - **SCEPTRE App**: A historian on a subnet with no OPC server was configured with an unrelated OPC's tag list and no address to collect from. It now gets no tags and a warning naming the subnet. | ||
|
|
||
| ### Security |
| @field_validator("name") | ||
| @classmethod | ||
| def _validate_device_name(cls, v: str | None) -> str | None: | ||
| if v is not None and not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9_ -]{0,62}", v): |
Contributor
There was a problem hiding this comment.
This should also verify that they are a minimum of 2 characters long and aren't a reserved name like 'all' or 'phenix', following the state of phenix validation as of phenix 2026.10.02
Author
There was a problem hiding this comment.
validate_hostname() now follows phenix v2026.10.02: 2 to 63 letters, digits and interior hyphens (no _), not all digits, and not all or phenix. Ignition device names get the same 2-character minimum and reserved names, but they keep spaces because Ignition uses them as display names. Note that rejecting phenix everywhere is stricter than core, which only errors on it for Windows nodes and warns otherwise. I can relax that if you prefer.
| from phenix_apps.common.logger import logger | ||
|
|
||
|
|
||
| def _check_fetch_path(path: str) -> str: |
Author
There was a problem hiding this comment.
Went with PurePosixPath(path).parts instead. os.pathsep is the PATH-list separator (:), and os.sep is the host separator, while this is a path inside the guest.
adamsrnmsu
force-pushed
the
fix-metadata-path-validation
branch
from
October 4, 2026 23:53
1d30bbb to
661f52e
Compare
Apps and SCORCH components build host-side file paths from scenario metadata (hostnames, device names, filenames from VMs) without checking them, so a value containing '/' or '..' could write outside the experiment directory. Add two shared helpers in common/utils.py: - validate_hostname(): phenix core's node-name rules as of v2026.10.02 (2 to 63 letters, digits and interior hyphens, not all digits, not 'all' or 'phenix'). external_node hosts in particular only exist in scenario metadata and bypass core's check. - safe_join(): joins parts onto a base and rejects a result that resolves outside it, including absolute parts that would replace the base. Use them in AppBase.add_node, caldera, helics, ignition, otsim, protonuke, scale and its plugins, sceptre, wireguard, and the cc, ssh and providerdata SCORCH components. mm_send/mm_recv keep VM-side paths inside the mount and mm_recv requires a normalized absolute host destination. Generated configs also stop accepting embedded newlines (protonuke args, wireguard fields), wireguard configs holding the private key are written 0600, and the sceptre Windows startup scripts drop from 0777 to 0755. Files touched here, including all of common/utils.py, also move from os.path to pathlib, since safe_join returns a Path. utils.abs_path() now always returns a Path. test_path_helpers.py pins the helpers' behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo
adamsrnmsu
force-pushed
the
fix-metadata-path-validation
branch
from
October 5, 2026 00:00
661f52e to
86f26a9
Compare
GhostofGoes
approved these changes
Oct 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Apps and SCORCH components build host-side file paths from scenario metadata (hostnames, device names, filenames reported by VMs) without checking those values. A value containing
/or..can write outside the experiment directory. This PR validates those values before they reach the filesystem.Two new helpers in
common/utils.py:validate_hostname()applies phenix core's node-name rules as of v2026.10.02: 2 to 63 letters, digits and interior hyphens, not all digits, and notallorphenix.external_nodehosts exist only in scenario metadata and never pass through core's check.safe_join()joins parts onto a base and rejects any result that resolves outside it, including an absolute part that would replace the base.Where they are used:
AppBase.add_node, caldera, helics, ignition, otsim, protonuke, scale (plus thebuiltinandwind_turbineplugins), sceptre and wireguardccrecvdestination,sshSFTP downloads andproviderdataconfig fetchesmm_sendandmm_recv, which keep VM-side paths inside the mount;mm_recvalso requires a normalized absolute host destinationOther changes to generated files:
argsand wireguard fields may not contain newlines.0600.0777to0755.User-visible constraints: the Scale
hostname_prefix, the wind turbinenameand the Ignition devicenameare limited to hostname-safe characters. The READMEs and CHANGELOG (### Security) are updated.Related Issues/PRs
One of four independent PRs: #152, #153, #155. Each is a single commit on
mainand they can merge in any order. #155 also adds a### Securitysection toCHANGELOG.md, so whichever of the two merges second needs a small rebase. No other files conflict. This PR leavessunspec/__init__.pyalone so it doesn't conflict with #156, which deletes it.Type of Change
fix)Checklist
Testing
make checkandmake testinsrc/pythonon Python 3.12: 741 passed. The newcommon/tests/test_path_helpers.pycovers hostname acceptance and rejection,safe_joincontainment (including absolute parts and..that stays inside the base), and themm_recvdestination guard. The scale and wind_turbine tests now check the files written to disk instead of mockingopen.Additional Notes
Files touched here, including all of
common/utils.py, also move fromos.pathtopathlib, sincesafe_join()returns aPath.utils.abs_path()now always returns aPath; with a relative path it used to return astr, which matters to any out-of-tree caller that concatenates strings onto it.🤖 Generated with Claude Code
https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo