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

getFilePathIfBinarySync should not rely on filename and extname #6010

Closed
AliasT opened this issue Jun 30, 2021 · 4 comments · Fixed by #6021
Closed

getFilePathIfBinarySync should not rely on filename and extname #6010

AliasT opened this issue Jun 30, 2021 · 4 comments · Fixed by #6021

Comments

@AliasT
Copy link

AliasT commented Jun 30, 2021

const getFilePathIfBinarySync = module.exports.getFilePathIfBinarySync = function (filePath) {
const forceAllowedExts = [
'.dylib', // Dynamic library
'.node' // Native node addon
]
const name = path.basename(filePath)
const ext = path.extname(filePath)
// Sometimes .node is the base name, not as a file extension
// https://github.com/electron/electron-osx-sign/pull/169
if (forceAllowedExts.includes(ext) || forceAllowedExts.includes(name)) {
return filePath
}
// Still consider the file as binary if extension is empty or seems invalid
// https://github.com/electron-userland/electron-builder/issues/5465
if ((ext === '' || ext.indexOf(' ') >= 0) && name[0] !== '.') {
return isBinaryFile(filePath) ? filePath : undefined
}
return undefined
}

I was trying to embed a python into an electron app. getFilePathIfBinarySync simply checked the files with blank extname, but files like python/bin/python3.7 was ignored by the sign process. Also the .so and .a files.

These files should be included automatically.

@mmaietta
Copy link
Collaborator

Hi there!
Would you be willing to open a PR for adding python, .so, and .a files to forceAllowedExts? And also any other files ext you think are mandatory for signing?

For the interim, there's a binaries arg that accepts a list of paths of files you want to force-sign.

if (opts.binaries) {
// Accept absolute paths for external binaries, else resolve relative paths from the artifact's app Contents path.
const userDefinedBinaries = opts.binaries.map(function (destination) { return fs.existsSync(destination) ? destination : path.resolve(appContentsPath, destination)})
// Insert at front to prioritize signing. We still sort by depth next
childPaths = userDefinedBinaries.concat(childPaths)
debuglog('Signing addtional user-defined binaries: ' + JSON.stringify(userDefinedBinaries, null, 1))
}

@AliasT
Copy link
Author

AliasT commented Jul 1, 2021

I think isBinaryFile should do most of the work.

- if ((ext === '' || ext.indexOf(' ') >= 0) && name[0] !== '.') { 
    return isBinaryFile(filePath) ? filePath : undefined 
- } 

btw: opts.binaries does not support regex currently.

@mmaietta
Copy link
Collaborator

mmaietta commented Jul 1, 2021

Interesting, electron-osx-sign upstream has the code you suggest
https://github.com/electron/electron-osx-sign/blob/master/util.js#L157-L162

@develar was there a reason why we manually copied electron-osx-sign into the repo? Curious if we can return back to upstream

@AliasT
Copy link
Author

AliasT commented Jul 1, 2021

#4656 (comment)
#5190 (comment)
electron/osx-sign#169

I modified the code metioned above inside node_modules as workaround without extra work. 🤒

develar pushed a commit that referenced this issue Jul 4, 2021
* fix: migrating to electron-osx-sign package to sync with upstream. Fixes: #6010 & #5190

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

Successfully merging a pull request may close this issue.

2 participants