Fix Validate readiness indicator file path#1
Open
odaysec wants to merge 2 commits intoRoblox:masterfrom
Open
Conversation
Added validation to ensure readiness indicator file is a single filename without path separators or parent references. the problem is that `ReadinessIndicatorExistsNow` and `GetReadinessIndicatorFile` accept an arbitrary string, normalize it, convert it to an absolute path, and pass it to `os.Stat` with no validation. To fix this, we should constrain what paths are acceptable. The most robust approach, given the limited context, is to restrict `ReadinessIndicatorFile` to a single filename (no directory separators, no `..`), or at least to a relative path inside a specific safe directory. Since we do not see a configured safe directory in the snippets and must avoid changing broader behavior, the least intrusive and safest change is to require `ReadinessIndicatorFile` to be a single path component (filename) with no `/`, `\`, or `..`. That preserves existing functionality for the common case where a simple file name is used, and prevents traversal or absolute-path abuse. Concretely, in `pkg/types/conf.go` we will modify both `GetReadinessIndicatorFile` and `ReadinessIndicatorExistsNow` so that they: 1. First validate `readinessIndicatorFileRaw`: - Reject if it is empty. - Reject if it contains `/`, `\`, or `".."` 2. If invalid, return an error (or false + error) clearly indicating the problem. 3. If valid, proceed as before: `filepath.Clean`, `filepath.Abs`, and `os.Stat`. This confines the path to a single component, eliminating directory traversal and uncontrolled file access, without changing any call sites or other behavior. No new imports or external dependencies are needed; we already import `path/filepath` and `os` in this file.
odaysec
commented
Jan 19, 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
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.
Added validation to ensure readiness indicator file is a single filename without path separators or parent references.
the problem is that
ReadinessIndicatorExistsNowandGetReadinessIndicatorFileaccept an arbitrary string, normalize it, convert it to an absolute path, and pass it toos.Statwith no validation. To fix this, we should constrain what paths are acceptable. The most robust approach, given the limited context, is to restrictReadinessIndicatorFileto a single filename (no directory separators, no..), or at least to a relative path inside a specific safe directory. Since we do not see a configured safe directory in the snippets and must avoid changing broader behavior, the least intrusive and safest change is to requireReadinessIndicatorFileto be a single path component (filename) with no/,\, or... That preserves existing functionality for the common case where a simple file name is used, and prevents traversal or absolute-path abuse.Concretely, in
pkg/types/conf.gowe will modify bothGetReadinessIndicatorFileandReadinessIndicatorExistsNowso that they:readinessIndicatorFileRaw:/,\, or".."filepath.Clean,filepath.Abs, andos.Stat.This confines the path to a single component, eliminating directory traversal and uncontrolled file access, without changing any call sites or other behavior. No new imports or external dependencies are needed; we already import
path/filepathandosin this file.