GeezerVison is showing github.com — Clarify the documentation of pathlib.Path.is_relative_to() · Issue #99334 · python/cpython
Clarify the documentation of pathlib.Path.is_relative_to() #99334DescriptionHi,
Currently (python 3.10.6 & 3.11.0):
from pathlib import Path
p = Path('/var/log/../../opt')
p.is_relative_to('/var/log')
>>> True
p = p.resolve()
p.is_relative_to('/var/log')
>>> FalseOnce you know is_relative_to uses relative_to, this makes more sense but it's not obvious from the documentation and the examples given. Also it can easily lead to code that looks secure but isn't. Case in point, I was tasked with reviewing this code today (simplified for illustration purposes):
path = Path(ROOT_PATH, user_input_rel_path)
if path.is_relative_to(ROOT_PATH):
path.unlink()
else:
raise PermissionError('Nope!')I was unsure if I should open a bug or not because one could easily argue it isn't a bug. I do believe however that a warning in the documentation could save a few devs from making a mistake.
Linked PRs
Activitybarneygale commented on Nov 13, 2022 Pathlib never elides ../ segments unless you call resolve()
I'm not a big fan of is_relative_to(). And I agree that users may expect a slightly more rigorous check that automatically handles ../. Once #99031 lands, the method will be equivalent to b == a or b in a.parents, which IMO is too trivial to justify its existence. We could look at deprecating it? brettcannon commented on Nov 18, 2022 It's probably too late to bother deprecating the function when the implementation is going to become trivial, but the docs could be clarified to effectively state the one-liner to clarify what the function represents. thomas-mckay commented on Feb 17, 2023 I would say, if we're going as far as thinking about deprecation, I'd rather keep the function and have it do more:
class PurePath:
def is_relative_to(self, other, strict=False):
# I've adapted my exemple to the 3.14 implementation,
# not the current one (i.e.: `other` vs `*other`)
a, b = self, other
if strict:
# I've read about security concerns around `normpath`.
# I think limiting its usage to `PurePath` is the way to
# avoid them here (and I think `PurePath` is exactly were
# one would want this behavior), but I'm not an expert
# on the matter.
a = Path(os.path.normpath(a))
b = Path(os.path.normpath(b))
return b == a or b in a.parents
class Path(PurePath):
def is_relative_to(self, other, strict=False):
a, b = self, other
if strict:
a = a.resolve(strict=True)
b = b.resolve(strict=True)
return b == a or b in a.parents
Path('/var/../../opt').is_relative_to(Path('/var'))
>>> True # we keep the current behavior as default
Path('/var/../../opt').is_relative_to(Path('/var'), strict=True)
>>> FalseAlthough, I have to admit to not being a huge fan of resolve in this case. Sometimes, what one wants is just to ensure that a path is a child of another path, regardless of any symlink along the way. These cases would not be directly covered by this proposed solution, but we could do
path = PurePath(ROOT_PATH, user_input_rel_path)
if path.is_relative_to(ROOT_PATH, strict=True):
Path(path).unlink()
else:
raise PermissionError('Nope!')Not ideal, I'll admit, but close enough, I think.
But, if I'm being honest, I don't understand when the current behavior is actually useful. In which case would one want Path('/var/log/../../opt').is_relative_to('/var/log') to return True? I'm assuming it's useful to some, otherwise it wouldn't have been done that way, but I don't see it, personally. So maybe, the actual solution is even simpler:
class PurePath:
def is_relative_to(self, other):
a = Path(os.path.normpath(self))
b = Path(os.path.normpath(other))
return b == a or b in a.parents
class Path(PurePath):
...
Path('/var/../../opt').is_relative_to(Path('/var'))
>>> False
path = Path(ROOT_PATH, user_input_rel_path) # use .resolve() if you need symlink resolution
if path.is_relative_to(ROOT_PATH):
Path.unlink()
else:
raise PermissionError('Nope!')This is the more elegant solution but it is a breaking change. added a commit that references this issue on Jan 23, 2024GH-99334: Explain that `PurePath.is_relative_to()` is purely lexical. (… barneygale commented on Jan 23, 2024 Tis done! Thanks @thomas-mckay for reporting, hopefully the new text makes sense to you.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|