Skip to content
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.

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

doc: clarify path.isAbsolute doesn’t resolve paths #57073

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ericfortis
Copy link

@ericfortis ericfortis commented Feb 15, 2025

Since path.isAbsolute() it’s likely used for mitigating path traversals, I’m documentating that it doesn’t resolve paths.

I didn’t test its usages but for example it looks that it’s used for that here: https://github.com/nodejs/node/blob/47ae886e43537365ccc54beff0b1ac08a5a5aa19/lib/fs.js#L1803C3-L1815C4

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Feb 15, 2025
path.isAbsolute('.'); // false
path.isAbsolute('/foo/bar'); // true
path.isAbsolute('/baz/..'); // true
path.isAbsolute('/baz/../..'); // true (doesn’t resolve)
Copy link
Member

Choose a reason for hiding this comment

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

this path would still be absolute even if resolved, so i'm not sure what it adds to include it here? (you can't go higher than /, so /baz/../.. would resolve to /, which is indeed absolute)

Copy link
Author

@ericfortis ericfortis Feb 15, 2025

Choose a reason for hiding this comment

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

isAbsolute is likely to be guarding traversals that could go higher, e.g.

  if (path.isAbsolute(myDir))
   path.join(MOUNT, myDir)

Copy link
Author

Choose a reason for hiding this comment

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

perhaps using another word for that comment such as doesn’t simplify. Not sure what’s the correct term for that

Copy link
Member

Choose a reason for hiding this comment

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

That seems a very confusing use case to me - an absolute path should never be prefixed by anything - but certainly if people are doing that then some kind of guidance seems beneficial.

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, even if there was a resolve, it would still be an absolute path

Suggested change
path.isAbsolute('/baz/../..'); // true (doesn’t resolve)
path.isAbsolute('/baz/../..'); // true

@ericfortis ericfortis requested a review from ljharb February 15, 2025 19:40
@ericfortis ericfortis requested a review from aduh95 February 16, 2025 16:21
path.isAbsolute('.'); // false
path.isAbsolute('/foo/bar'); // true
path.isAbsolute('/baz/..'); // true
path.isAbsolute('/baz/../..'); // true (doesn’t resolve)
Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, even if there was a resolve, it would still be an absolute path

Suggested change
path.isAbsolute('/baz/../..'); // true (doesn’t resolve)
path.isAbsolute('/baz/../..'); // true

Comment on lines +320 to +329
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)
}
Copy link
Contributor

@aduh95 aduh95 Feb 16, 2025

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

Copy link
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.

Copy link
Author

@ericfortis ericfortis Feb 16, 2025

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
  const parts = dpath.split('/')
  let sum = 0
  for (const part of parts) {
    if (part === '')
      continue
    if (part === '..')
      sum--
    else
      sum++
    if (sum < 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)
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants