Shared: Add library for filepath normalization #14500
Conversation
shared/util/codeql/util/FilePath.qll
Outdated
|
|
||
| private int getNumComponents() { result = strictcount(int i | exists(this.getComponent(i))) } | ||
|
|
||
| /** count .. segments in prefix in normalization from index i */ |
There was a problem hiding this comment.
I see you simply copied my algorithm suggestion verbatim, but these qldocs could be a bit nicer now that the code is moving from a slack thread and into actual main.
| /** | ||
| * Gets the normalized filepath for this string. | ||
| * | ||
| * Normalizes `..` paths, `.` paths, and multiple `/`s as much as possible, but does not normalize case, resolve symlinks, or make relative paths absolute. |
There was a problem hiding this comment.
Should this also note only local paths are handled (correctly)? E.g.: the UNC path //server/public/../private should resolve to //server/public/private (you can't .. out of a share for security reasons), but AFAICT this would resolve it to /server/private.
| * | ||
| * The normalized path will not have a trailing `/`. | ||
| * | ||
| * Only `/` is treated as a path separator. |
There was a problem hiding this comment.
Does this mean something e.g. the absolute path C:\foo\bar\baz\../quux would get normalised to quux?
There was a problem hiding this comment.
Or maybe this warrants a stronger caution that DOS/Windows paths in general are unsupported as AFAICT C:/.. and even C:foo/.. incorrectly resolve to . despite using / separators.
There was a problem hiding this comment.
Seems reasonable to address these cases in the documentation. However as this work was semi-blocking for #14343, and these cases are not relevant for this use case, I have decided to merge anyway. I will address these doc improvements in a follow-up PR.
| | a/b/../c | a/c | | ||
| | a/b//////c/./d/../e//d// | a/b/c/e/d | | ||
| | a/b/c | a/b/c | | ||
| | a/b/c/../../../../d/e/../f | ../d/f | |
There was a problem hiding this comment.
Might be good to add a similar test but with an absolute path (e.g. /a/b/c/../../../../d/e/../f should resolve to /d/f.
|
Seems like @sashabu had some reasonable review questions. Shouldn't they at least have been answered before merging? |
Adds a library
FilePathexposing an abstract classNormalizableFilepath, to be extended to provide strings that should be normalized as filepaths (i.e. transforminga/b/../ctoa/c) through thegetNormalizedPathmember.