Skip to content

doc: clarify path.isAbsolute is not enough for mitigating path traversals #57073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduh95 merged 4 commits into from
Mar 2, 2025
Merged

doc: clarify path.isAbsolute is not enough for mitigating path traversals #57073

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6399f9e
doc: clarify path.isAbsolute doesn’t resolve paths
ericfortis Feb 15, 2025
7cbdcbb
doc: clarify path.isAbsolute doesn’t resolve paths
ericfortis Feb 16, 2025
f79fbba
doc: clarify path.isAbsolute doesn’t resolve paths
ericfortis Mar 2, 2025
b68f49b
doc: clarify path.isAbsolute doesn’t resolve paths
ericfortis Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
doc: clarify path.isAbsolute doesn’t resolve paths
  • Loading branch information
@ericfortis
ericfortis committed Feb 16, 2025
commit 7cbdcbb9d4673122dd25e5d7856bb614aa8a352b
15 changes: 12 additions & 3 deletions doc/api/path.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,18 @@ added: v0.11.2
* `path` {string}
* Returns: {boolean}

The `path.isAbsolute()` method determines if the literal (non-resolved) `path`
is an absolute path. Therefore, it’s likely that you want to run it through
`path.normalize()` before using it in `path.join()` or `path.isAbsolute()`.
The `path.isAbsolute()` method determines if the literal (non-resolved) `path` is an
absolute path. Therefore, it’s likely that you want to run it through `path.normalize()`
if your plan is to use it as a subpath in order to mitigate path traversals. For example:

```js
// Normalizing the subpath mitigates traversing above the mount directory.
const mySubpath = path.normalize('/foo/../..');
if (path.isAbsolute(mySubpath)) {
const myPath = path.join(MOUNT_DIR, mySubpath)
}

@aduh95 aduh95 Feb 16, 2025 •
edited
Loading

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.

I don't really understand why you would call path.isAbsolute at all.

Suggested change
The `path.isAbsolute()` method determines if the literal (non-resolved) `path` is an
absolute path. Therefore, it’s likely that you want to run it through `path.normalize()`
if your plan is to use it as a subpath in order to mitigate path traversals. For example:
```js
// Normalizing the subpath mitigates traversing above the mount directory.
const mySubpath = path.normalize('/foo/../..');
if (path.isAbsolute(mySubpath)) {
const myPath = path.join(MOUNT_DIR, mySubpath)
}
The `path.isAbsolute()` method determines if `path` is an absolute path. Being an
absolute path doesn't not guarentee it would be safe to treat as a subpath:
```js
const MOUNT_DIR = '/home/example/';
const userProvidedPath = '/foo/../../../etc/passwd';
console.log(path.isAbsolute(userProvidedPath)); // true
console.log(path.join(MOUNT_DIR, userProvidedPath)); // /etc/passwd
// Instead, call `path.normalize`:
console.log(path.join(MOUNT_DIR, path.normalize(`/${userProvidedPath}`))); // /home/example/etc/passwd

@ericfortis ericfortis Feb 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Flow control, e.g.

const mySubpath = path.normalize('/foo/../..')
if (!path.isAbsolute(mySubpath))
  throw 'FORBIDDEN'
const myPath = path.join(MOUNT_DIR, mySubpath)

I agree it‘s not needed.

@ericfortis ericfortis Feb 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, instead of documenting the behavior, a better approach could be adding path.isAbsoluteSubpath(). That way, when autocompleting, users can tell there's something not obvious about path.isAbsolute().

Something like,

function isAbsoluteSubpath(path) {
  const dpath = decodeURIComponent(path)
  if (!dpath.startsWith('/'))
    return false
  let nOutside = 0
  for (const part of dpath.split('/')) {
    if (part === '')
      continue
    if (part === '..')
      nOutside++
    else
      nOutside--
    if (nOutside > 0)
      return false
  }
  return true
}

function is(path, expected) {
  console.assert(expected === isAbsoluteSubpath(path), path)
}

;[
  '.',
  './',
  '..',
  '/..',
  '../',
  '/home//../../..',
  '/home/user/../../..'
].forEach(p => {
  is(p, false)
  is(encodeURIComponent(p), false)
})

;[
  '/',
  '/home/user/docs',
  '/home/user/./docs',
  '/home/user//docs'
].forEach(p => {
  is(p, true)
  is(encodeURIComponent(p), true)
})

@aduh95 aduh95 Mar 1, 2025

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.

If the current behavior is confusing, let's update the doc to make them less confusing. Having clearer docs doesn't mean we can't add another API, that'd be a false dichotomy to think we can do one and not the other.

Would you like to take my suggestion so this PR can move forward?

```


If the given `path` is a zero-length string, `false` will be returned.

Expand Down