Skip to content

fix(config): allow two dots in path segments#10836

Open
winklemad wants to merge 2 commits into
firebase:mainfrom
winklemad:fix/config-path-two-dot-segments-20260722
Open

fix(config): allow two dots in path segments#10836
winklemad wants to merge 2 commits into
firebase:mainfrom
winklemad:fix/config-path-two-dot-segments-20260722

Conversation

@winklemad

@winklemad winklemad commented Jul 22, 2026

Copy link
Copy Markdown

Description

Config.path() previously treated any normalized relative path containing the substring .. as outside the project directory. This falsely rejected valid in-project segments such as a Hosting public directory named foo..bar.

This change rejects an exact parent path, a path beginning with the parent-directory segment and platform separator, or an absolute result from path.relative() such as a cross-volume result on Windows. This preserves traversal protection while allowing valid two-dot path segments.

Scenarios Tested

  • A configured Hosting public path of foo..bar resolves inside the project.
  • .. and ../outside remain rejected.
  • An absolute result from path.relative() is rejected through a platform-independent regression.
  • path.win32.relative() cross-volume semantics reject D:\outside while allowing foo..bar.
  • npx mocha src/config.spec.ts — passed with 19 tests.
  • npm run lint:changed-files — passed with no errors.
  • npm run test:compile — passed.
  • npm ls --depth=0, npm run build, and npm run prepare — passed.
  • npm test -- -- --forbid-only — lint, formatting, and compilation passed; the broad unit phase encountered unrelated environment-sensitive failures locally, including restricted proxy and metadata access.

Sample Commands

Not applicable; this does not add or change commands or flags.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the path traversal check in Config.path() to allow path segments containing two dots (e.g., foo..bar) while still rejecting parent directory traversal, and adds corresponding unit tests. Feedback points out a potential security vulnerability on Windows where path.relative can return an absolute path if the directories are on different drives, thereby bypassing the check. A code suggestion was provided to handle this by checking if the relative path is absolute.

Comment thread src/config.ts Outdated
const outPath = path.normalize(path.join(this.projectDir, pathName));
if (path.relative(this.projectDir, outPath).includes("..")) {
const relativePath = path.relative(this.projectDir, outPath);
if (relativePath === ".." || relativePath.startsWith(`..${path.sep}`)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

On Windows, if this.projectDir and outPath are on different drives (e.g., C: and D:), path.relative returns an absolute path (e.g., D:\path). Since this absolute path does not start with .., the current check will fail to detect that the path is outside the project directory.\n\nTo prevent this directory traversal bypass on Windows, we should also check if relativePath is an absolute path using path.isAbsolute(relativePath).

Suggested change
if (relativePath === ".." || relativePath.startsWith(`..${path.sep}`)) {
if (path.isAbsolute(relativePath) || relativePath === ".." || relativePath.startsWith(".." + path.sep)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants