fix: CN-272 whiteout regex bug #690
Conversation
parker-snyk
force-pushed
the
CN-272-whiteout-regex-bug
branch
3 times, most recently
from
7f7dd67 to
f59fe4a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
parker-snyk
force-pushed
the
CN-272-whiteout-regex-bug
branch
from
ea113ba to
ef6a033
Compare
| export function isWhitedOutFile(filename: string) { | ||
| return filename.match(/.wh./gm); | ||
| const lastSlashIndex = filename.lastIndexOf("/"); |
There was a problem hiding this comment.
nit: would it be cleaner to just use the builtin path.basename. maybe not, just pointing out in case you didn't know about it
There was a problem hiding this comment.
Much cleaner, thank you! I did not know about this
| export function removeWhiteoutPrefix(filename: string): string { | ||
| // Replace .wh. that appears at the start or after the last slash, | ||
| // and ensure no slashes come after .wh. | ||
| return filename.replace(/^(.*\/)?\.wh\.([^\/]*)$/, "$1$2"); |
There was a problem hiding this comment.
do you think this should be "+" instead? I guess we expect 0 times to be a bug in the generator (i.e., impossible) but we should still remove it?
There was a problem hiding this comment.
(The comment applied to the * in [^\/]*)
| export function isWhitedOutFile(filename: string) { | ||
| return filename.match(/.wh./gm); | ||
| const lastSlashIndex = filename.lastIndexOf("/"); |
There was a problem hiding this comment.
this assumes unix path separator -- do we need to handle windows too?
There was a problem hiding this comment.
I just updated to use path.basename so it should account for this now - https://nodejs.org/api/path.html#windows-vs-posix
I didn't know about this function until you mentioned it, so let me know if I'm incorrect
| * prefixing the filename with a .wh. | ||
| * https://www.madebymikal.com/interpreting-whiteout-files-in-docker-image-layers | ||
| * https://github.com/opencontainers/image-spec/blob/main/layer.md#whiteouts | ||
| */ | ||
| export function isWhitedOutFile(filename: string) { |
There was a problem hiding this comment.
does this function also need to account for opaque whiteouts: https://github.com/opencontainers/image-spec/blob/main/layer.md#opaque-whiteout?
In other words, check that the filename is not .wh..wh..opq? Looking above, it's not clear our extractor logic even handles opaque whiteouts at all 🤔
There was a problem hiding this comment.
I think as long as it starts with .wh it should account for it, it looks like out logic checks if a file is or within any whited out file or directory, so if a directory starts with .wh, any files under it should be excluded. I can manually verify though
There was a problem hiding this comment.
Okay, I misunderstood how .wh..wh..opq worked, I'll fix this
We were using
.wh.to look for whited out files, but we didn't escape the., so it was a match-any characterWe also weren't following the OpenContainer spec: https://github.com/opencontainers/image-spec/blob/main/layer.md#whiteouts
.wh. needs to be the prefix of the filename. So it either needs to prefix everything (no slashes, just filename) or it needs to prefix the filename after the last slash (full path)
Our current implementation just looks for .wh. to appear anywhere, so we are incorrectly whiting out files. This includes libraries, so all of the node 'which' packages were being skipped, for example