-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 shasum path #1793
base: master
Are you sure you want to change the base?
Fix shasum path #1793
Conversation
Signed-off-by: Dmitrii Kovalkov <kovalkov.dmitriy@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1793 +/- ##
======================================
Coverage 32% 32%
======================================
Files 45 45
Lines 2609 2609
Branches 337 337
======================================
Hits 859 859
Misses 1750 1750
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/electron/routing_service.ts
Outdated
@@ -323,7 +323,7 @@ async function installLinuxRoutingServices(): Promise<void> { | |||
command += | |||
' && ' + | |||
installationFileDescriptors | |||
.map(({filename, sha256}) => `/usr/bin/echo "${sha256} ${path.join(tmp, filename)}" | /usr/bin/shasum -a 256 -c`) | |||
.map(({filename, sha256}) => `/usr/bin/echo "${sha256} ${path.join(tmp, filename)}" | $(which shasum) -a 256 -c`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the path because it opens up a security vulnerability, that's why we use absolute paths to read-only files.
One alternative would be to check a few absolute paths instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @jyyi1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative path is not secure, it allows a normal user running arbitrary code as root.
Signed-off-by: Dmitrii Kovalkov <kovalkov.dmitriy@gmail.com>
Moved to absolute paths, check it, please |
@@ -323,7 +323,7 @@ async function installLinuxRoutingServices(): Promise<void> { | |||
command += | |||
' && ' + | |||
installationFileDescriptors | |||
.map(({filename, sha256}) => `/usr/bin/echo "${sha256} ${path.join(tmp, filename)}" | /usr/bin/shasum -a 256 -c`) | |||
.map(({filename, sha256}) => `/usr/bin/echo "${sha256} ${path.join(tmp, filename)}" | (/usr/bin/shasum -a 256 -c || /usr/bin/core_perl/shasum -a 256 -c)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really the logic we want. If the first command fails for whatever reason (e.g. fingerpring fails) we don't want to run the second command. And we don't want to try to run the first command if it doesn't exist. It's a lot cleaner and easier to reason about if you check the existence of the files before running them.
This also needs to be tested to make sure it works on Ubuntu/Debian, which is our only supported Linux.
/cc @jyyi1 to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'd rather test which shasum
(either /usr/bin/shasum
or /usr/bin/core_perl/shasum
) exists before running the command, and only append the correct full shasum
path to the command
:
shasumCmd = findShasum() // this function returns `/usr/bin/shasum` or `/usr/bin/core_perl/shasum` depending on which one exists
command +=
' && ' +
installationFileDescriptors
.map(({filename, sha256}) => `/usr/bin/echo "${sha256} ${path.join(tmp, filename)}" | ${shasumCmd} -a 256 -c`)
On different systems
shasum
have different path, like this/usr/bin/shasum
or/usr/bin/core_perl/shasum
and etc