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.

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

Jump to bottom

Add normalizePath and tests #6587

Merged
merged 1 commit into from
Jul 7, 2018
Merged

Add normalizePath and tests #6587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 43 additions & 2 deletions lib/pure/os.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ proc setCurrentDir*(newDir: string) {.inline, tags: [].} =

proc expandFilename*(filename: string): string {.rtl, extern: "nos$1",
tags: [ReadDirEffect].} =
## Returns the full (`absolute`:idx:) path of the file `filename`,
## raises OSError in case of an error.
## Returns the full (`absolute`:idx:) path of an existing file `filename`,
## raises OSError in case of an error. Follows symlinks.
when defined(windows):
var bufsize = MAX_PATH.int32
when useWinUnicode:
Expand Down Expand Up @@ -338,6 +338,47 @@ proc expandFilename*(filename: string): string {.rtl, extern: "nos$1",
result = $r
c_free(cast[pointer](r))

proc normalizePath*(path: var string) {.rtl, extern: "nos$1", tags: [].} =
Copy link
Member

Choose a reason for hiding this comment

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

this should be in ospaths, no? nothing in normalizePath accesses file system

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, I forgot about this. But @FedericoCeratto has waited long enough with this PR so we can fix this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

of course; I much prefer faster PR turnaround than waiting forever for PR's to be so-called "perfect" (this slow turnaround really hurts D ecosystem IMO, Nim seems better in that regard)

Copy link
Member

Choose a reason for hiding this comment

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

=> kept track of that in #8177 meta-issue so we don't forget

## Normalize a path.
##
## Consecutive directory separators are collapsed, including an initial double slash.
##
## On relative paths, double dot (..) sequences are collapsed if possible.
## On absolute paths they are always collapsed.
##
## Warning: URL-encoded and Unicode attempts at directory traversal are not detected.
## Triple dot is not handled.
let isAbs = isAbsolute(path)
var stack: seq[string] = @[]
for p in split(path, {DirSep}):
case p
of "", ".":
continue
of "..":
if stack.len == 0:
if isAbs:
discard # collapse all double dots on absoluta paths
else:
stack.add(p)
elif stack[^1] == "..":
stack.add(p)
else:
discard stack.pop()
else:
stack.add(p)

if isAbs:
path = DirSep & join(stack, $DirSep)
elif stack.len > 0:
path = join(stack, $DirSep)
else:
path = "."

proc normalizedPath*(path: string): string {.rtl, extern: "nos$1", tags: [].} =
## Returns a normalized path for the current OS. See `<#normalizePath>`_
result = path
normalizePath(result)

when defined(Windows):
proc openHandle(path: string, followSymlink=true, writeAccess=false): Handle =
var flags = FILE_FLAG_BACKUP_SEMANTICS or FILE_ATTRIBUTE_NORMAL
Expand Down
62 changes: 62 additions & 0 deletions tests/stdlib/tos.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ true
true
true
true

'''
"""
# test os path creation, iteration, and deletion
Expand Down Expand Up @@ -137,8 +138,69 @@ import times
let tm = fromUnix(0) + 100.microseconds
writeFile("a", "")
setLastModificationTime("a", tm)

when defined(macosx):
echo "true"
else:
echo getLastModificationTime("a") == tm
removeFile("a")

when defined(Linux) or defined(osx):

block normalizedPath:
block relative:
doAssert normalizedPath(".") == "."
doAssert normalizedPath("..") == ".."
doAssert normalizedPath("../") == ".."
doAssert normalizedPath("../..") == "../.."
doAssert normalizedPath("../a/..") == ".."
doAssert normalizedPath("../a/../") == ".."
doAssert normalizedPath("./") == "."

block absolute:
doAssert normalizedPath("/") == "/"
doAssert normalizedPath("/.") == "/"
doAssert normalizedPath("/..") == "/"
doAssert normalizedPath("/../") == "/"
doAssert normalizedPath("/../..") == "/"
doAssert normalizedPath("/../../") == "/"
doAssert normalizedPath("/../../../") == "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would make more sense to raise an error for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the point of normalization. If you are looking for a way to detect and prevent directory traversal I think that should go into joinPath

Copy link
Member

@timotheecour timotheecour Jul 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @dom96 , throwing seems much saner behavior.
Anecdotically, D's version also doesn't throw but that behavior isn't documented (just filed https://issues.dlang.org/show_bug.cgi?id=19069 to see if good arguments come up)

EDIT likewise with python:
os.path.normpath('/../..')
Out[6]: '/'

even though i don't see that being documented...

what's a use case where silently accepting is preferable to throwing?

doAssert normalizedPath("/a/b/../../foo") == "/foo"
doAssert normalizedPath("/a/b/../../../foo") == "/foo"
doAssert normalizedPath("/./") == "/"
doAssert normalizedPath("//") == "/"
doAssert normalizedPath("///") == "/"
doAssert normalizedPath("/a//b") == "/a/b"
doAssert normalizedPath("/a///b") == "/a/b"
doAssert normalizedPath("/a/b/c/..") == "/a/b"
doAssert normalizedPath("/a/b/c/../") == "/a/b"

else:

block normalizedPath:
block relative:
doAssert normalizedPath(".") == "."
doAssert normalizedPath("..") == ".."
doAssert normalizedPath("..\\") == ".."
doAssert normalizedPath("..\\..") == "..\\.."
doAssert normalizedPath("..\\a\\..") == ".."
doAssert normalizedPath("..\\a\\..\\") == ".."
doAssert normalizedPath(".\\") == "."

block absolute:
doAssert normalizedPath("\\") == "\\"
doAssert normalizedPath("\\.") == "\\"
doAssert normalizedPath("\\..") == "\\"
doAssert normalizedPath("\\..\\") == "\\"
doAssert normalizedPath("\\..\\..") == "\\"
doAssert normalizedPath("\\..\\..\\") == "\\"
doAssert normalizedPath("\\..\\..\\..\\") == "\\"
doAssert normalizedPath("\\a\\b\\..\\..\\foo") == "\\foo"
doAssert normalizedPath("\\a\\b\\..\\..\\..\\foo") == "\\foo"
doAssert normalizedPath("\\.\\") == "\\"
doAssert normalizedPath("\\\\") == "\\"
doAssert normalizedPath("\\\\\\") == "\\"
doAssert normalizedPath("\\a\\\\b") == "\\a\\b"
doAssert normalizedPath("\\a\\\\\\b") == "\\a\\b"
doAssert normalizedPath("\\a\\b\\c\\..") == "\\a\\b"
doAssert normalizedPath("\\a\\b\\c\\..\\") == "\\a\\b"