Exclude the temporary directory from scanning. #236
Conversation
chrisgavin
force-pushed
the
ignore-temp-dir
branch
5 times, most recently
from
b7a2046 to
6f152ce
Compare
chrisgavin
changed the title
chrisgavin
force-pushed
the
ignore-temp-dir
branch
from
6f152ce to
1a4d2e3
Compare
chrisgavin
force-pushed
the
ignore-temp-dir
branch
from
1a4d2e3 to
206e34c
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Looks generally good to me with one comment. I want to test it to confirm my understanding of the behaviour. Should get that testing done today or tomorrow.
src/analysis-paths.ts
Outdated
| logger.warning( | ||
| "Storing the CodeQL Runner in the directory being analyzed is not recommended." | ||
| ); | ||
| pathsIgnore = pathsIgnore.concat(config.tempDir); |
There was a problem hiding this comment.
I think you want to add tempRelativeToWorking here. These paths are relative to the cwd.
There was a problem hiding this comment.
Ah okay. At least with the JavaScript extractor I had no trouble with an absolute path, but if we're using relative elsewhere that does seem safer in case other extractors are less forgiving.
There was a problem hiding this comment.
I think the python extractor is more picky and only works with relative paths. That's what I remember. There's an issue on the CodeQL side to make that behaviour more consistent.
chrisgavin
force-pushed
the
ignore-temp-dir
branch
from
9773a65 to
dbecf76
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
I've reproduced the bug with the codeql-runner-linux executable not in the cwd or the directory of the repository. As noted earlier the issue is with the temporary directory being in the current directory. By default the temp directory is ./codeql-runner relative to the cwd, i.e. where the runner executable was run from but not necessarily where it is located.
Something else I realised when testing this is this bug won't happen every time. The files being detected are those from the downloaded and unpacked bundle. This only happens if we don't find the bundle in the toolcache, and hence we download a new version to the temp dir and copy it to the toolcache. So on the next run the temp dir is wiped, but the bundle is not re-downloaded because we already have one in the toolcache that is no wiped.
You could get a similar problem if the toolcache is located in the cwd. The toolcache by default is created in the home directory, so this could happen if for some reason analysis is being run directly from the home directory.
On the plus side, I've tested using a runner built from this PR and it wokred. I tested it for javascript and python.
chrisgavin
force-pushed
the
ignore-temp-dir
branch
from
b958d86 to
08bfd49
Compare
chrisgavin
force-pushed
the
ignore-temp-dir
branch
from
08bfd49 to
62f25fd
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Sorry for the delay. I've done another quick test of this to make sure. Everything looks good and it's working perfectly.
|
No worries! I'm glad it all works. |
This fixes the case where the CodeQL Runner is downloaded on-the-fly to the source directory of the repository being analyzed. It gets copied to the toolcache which is outside of the working directory, but still remains in the repository directory too. We could delete it from the repository directory after copying it to the toolcache but a more robust solution seems like it would just be to ignore the temporary directory completely.
It's not ideal that people download the runner in this way because even compiled languages could take issue with unexpected files in the repository but it seems like people do so we should handle it as well as we can.
Merge / deployment checklist