NEW: @W-15884203@: Implement the retire-js engine #23
Conversation
stephen-carter-at-sf
force-pushed
the
d/W-15884203
branch
from
0f6845b to
10d1df5
Compare
stephen-carter-at-sf
force-pushed
the
d/W-15884203
branch
from
10d1df5 to
5e2f6fd
Compare
| "@types/tmp": "^0.2.6", | ||
| "isbinaryfile": "^5.0.2", | ||
| "node-stream-zip": "^1.15.0", | ||
| "retire": "^5.0.0", | ||
| "tmp": "^0.2.3" |
There was a problem hiding this comment.
I confirmed that all these third party libraries (which we used before) have all already been approved.
stephen-carter-at-sf
force-pushed
the
d/W-15884203
branch
from
e2f5c75 to
33ec286
Compare
| this.retireJsExecutor = retireJsExecutor ? retireJsExecutor : new AdvancedRetireJsExecutor(emitLogEventFcn); | ||
| } | ||
|
|
||
| getName(): string { |
There was a problem hiding this comment.
Some places you call this method and in other places, you directly use the RetireJsEngine.NAME. Is that intentional?
There was a problem hiding this comment.
getName is the only way the core module gets the engine name. The only other place I call getName here in this project is when testing that getName returns the expected name. Otherwise, I only use the constant. This indeed is intentional. In those other instances where I use the constant, I don't have an instance of the engine in order to call getName anyway.
| /** | ||
| * Expands a list of files and/or folders to be a list of all contained files, including the files found in subfolders | ||
| */ | ||
| export function expandToListAllFiles(absoluteFileOrFolderPaths: string[]): string[] { |
There was a problem hiding this comment.
Question: What does this method do that using globby and glob('**/**') wouldn't do?
There was a problem hiding this comment.
This function receives the list of files and folders that it gets from core. For example:
if the user passes to the cli ['some/relativeFile.js', 'some/relative/folder', 'some/relative/folder/someFileInTheFolder.js'] then core automatically removes dudundant entries, and makes the paths absolute:
['/full/path/some/relativeFile.js', '/full/path/some/relative/folder']
and then the engine gets this list.
Now retire js needs to walk through the list of all files... so this expandToListAllFiles will then convert things to
['/full/path/some/relativeFile.js', '/full/path/some/relative/folder/subFolder/someFile.js', '/full/path/some/relative/folder/anotherFile.html', ...., '/full/path/some/relative/folder/yetAnotherFile.zip']
for example.
globby just expands globs. Also, i'd like to not bring in unnecessary tools to do operations that are simple. Using a hammer all the time can get messy.
The algorithm I'm using here is a very common path algorithm for finding all files under a directory.
| */ | ||
| export function isZipFile(file: string) { | ||
| const buf: Buffer = fs.readFileSync(file); | ||
| /* istanbul ignore next */ |
There was a problem hiding this comment.
Why is this being excluded?
There was a problem hiding this comment.
Any time we have literally no way to hit a branch of code because of some crazy edge case, it's nice to make this known to the code coverage utility.
/* istanbul ignore next */
is how you basically say, ignore the next line for code coverage checking here.
It seems there are many edge cases that is-zip covers... but our zip files only hit a few of these branches. But it leaving code as is since our users might have some random file that hits these other branches.
| }); | ||
|
|
||
| it('When validate is called, then nothing happens since it currently is a no-op', () => { | ||
| engine.validate(); |
There was a problem hiding this comment.
Is this test actually verifying that nothing is happening? Or is it just for coverage?
There was a problem hiding this comment.
Our validate method here is a no-op. Making sure that at least the core module can call it without things blowing up. So it is both for coverage, but also as a sanity check that when the core module calls it - it won't break anything.
| /** | ||
| * AdvancedRetireJsExecutor - An advanced strategy of calling retire that does a lot more work to catch more vulnerabilities | ||
| * | ||
| * This executor examines all specifies files and makes a copy of all text based files into a temporary folder, renaming |
There was a problem hiding this comment.
Is this something we introduced in 5.x or is this the same strategy currently followed by sfdx-scanner?
There was a problem hiding this comment.
No this is exactly what our current strategy with 4.x is. This is what makes our retire-js engine more complicated... because we have to do all this advanced stuff.
stephen-carter-at-sf
force-pushed
the
d/W-15884203
branch
from
40222eb to
5a495eb
Compare
No description provided.