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.

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

fix: resolve relative bundle file path correctly [ROAD-329] #112

Merged
merged 2 commits into from
Oct 15, 2021

Conversation

michelkaporin
Copy link
Contributor

  • Ready for review
  • Linked to Jira ticket

What does this PR do?

This PR resolves the issue of determining relative bundle file paths when many folders are passed for the analysis.

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

When getBundleFilePath() is called for multiple folders analysis (e.g. VS Code workspace with many folders), it is passed with baseDir = '' argument that makes relative path resolution fail on Linux within VS Code extension currently.

The following behaviour is observed at the moment:
args: baseDir='', filePath='/home/user/Downloads/goof-master/routes/index.js'
returns: 'Downloads/goof-master/routes/index.js'

path.relative() relies on process.cwd() that can be different based on where Node.js process was started. For VS Code, If this code is called on Mac OS, process.cwd() defaults to /, however Linux defaults to the user directory e.g. /home/user. Thus wrong analysis results file paths for bundles will be returned.

Relevant Jira ticket: https://snyksec.atlassian.net/browse/ROAD-329

Screenshots

Additional questions

@michelkaporin michelkaporin requested a review from a team as a code owner October 15, 2021 08:16
const linuxPath = '/home/user/Git/goof/routes/index.js';
expect(getBundleFilePath(linuxPath, baseDir)).toEqual(linuxPath);

const windowsPath = 'C:\\Users\\user\\Git\\goof\\index.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm afraid our engine will fail, if it receives this path.
can you please also add an explicit test for it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, stop, this is a reverse process. all good

@michelkaporin michelkaporin merged commit 5f8c641 into master Oct 15, 2021
@michelkaporin michelkaporin deleted the fix/bundlePath-resolve branch October 15, 2021 10:11
@snyksec
Copy link

snyksec commented Oct 15, 2021

🎉 This PR is included in version 4.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants