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(darwin): ensure chrome-sandbox is owned by root #246

Merged
merged 9 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,16 @@ on:

jobs:
build:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
matrix:
node-version: [10.x, 12.x, 14.x]
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add testing for node 16 in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather deal with that in the Node 10 PR.

os: [macOS-latest, ubuntu-latest]

steps:
- name: Install dependencies
run: sudo apt-get --yes --quiet install lintian
- uses: actions/checkout@v2
- name: Install dependencies
run: ci/install-os-dependencies.sh
- uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
Expand All @@ -33,6 +34,7 @@ jobs:
npm install --engine-strict
npm update
- name: Fix permission for test fixtures
if: runner.os == 'ubuntu-latest'
run: chmod --recursive g-w test/fixtures
- name: Test
run: npm test
Expand Down
3 changes: 0 additions & 3 deletions .npmignore

This file was deleted.

10 changes: 10 additions & 0 deletions ci/install-os-dependencies.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env bash

case "$(uname -s)" in
Darwin)
brew install dpkg fakeroot
;;
Linux)
sudo apt-get --yes --quiet install lintian
;;
esac
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
"bin": {
"electron-installer-debian": "src/cli.js"
},
"files": [
"bin",
"src",
"resources"
],
"scripts": {
"lint": "eslint .",
"spec": "nyc mocha",
Expand Down
8 changes: 7 additions & 1 deletion src/installer.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,13 @@ class DebianInstaller extends common.ElectronInstaller {
async createPackage () {
this.options.logger(`Creating package at ${this.stagingDir}`)

const output = await spawn('fakeroot', ['dpkg-deb', '--build', this.stagingDir], this.options.logger)
const command = ['--build', this.stagingDir]
if (process.platform === 'darwin') {
command.unshift('--root-owner-group')
}
command.unshift('dpkg-deb')

const output = await spawn('fakeroot', command, this.options.logger)
this.options.logger(`dpkg-deb output: ${output}`)
}

Expand Down
Empty file.
94 changes: 62 additions & 32 deletions test/installer.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,47 +129,77 @@ describe('module', function () {
/^No Description or ProductDescription provided/
)

if (process.platform !== 'darwin') {
describeInstaller(
'with debian scripts and lintian overrides',
{
src: 'test/fixtures/app-with-asar/',
options: {
productDescription: 'Just a test.',
arch: 'i386',
scripts: {
preinst: 'test/fixtures/debian-scripts/preinst.sh',
postinst: 'test/fixtures/debian-scripts/postinst.sh',
prerm: 'test/fixtures/debian-scripts/prerm.sh',
postrm: 'test/fixtures/debian-scripts/postrm.sh'
},
lintianOverrides: [
'binary-without-manpage',
'changelog-file-missing-in-native-package',
'executable-not-elf-or-script'
]
}
},
'passes lintian checks',
async outputDir => {
await assertASARDebExists(outputDir)
try {
await spawn('lintian', [path.join(outputDir, 'footest_i386.deb')], {
updateErrorCallback: (err) => {
if (err.code === 'ENOENT' && err.syscall === 'spawn lintian') {
err.message = 'Your system is missing the lintian package'
}
}
})
} catch (err) {
if (!err.stdout) {
throw err
}
const stdout = err.stdout.toString()
const lineCount = stdout.match(/\n/g).length
if (lineCount > 1) {
throw new Error(`Warnings not overriding:\n${stdout}`)
}
}
}
)
}

describeInstaller(
'with debian scripts and lintian overrides',
'correct owner and permissions for chrome-sandbox',
{
src: 'test/fixtures/app-with-asar/',
options: {
productDescription: 'Just a test.',
arch: 'i386',
scripts: {
preinst: 'test/fixtures/debian-scripts/preinst.sh',
postinst: 'test/fixtures/debian-scripts/postinst.sh',
prerm: 'test/fixtures/debian-scripts/prerm.sh',
postrm: 'test/fixtures/debian-scripts/postrm.sh'
},
lintianOverrides: [
'binary-without-manpage',
'changelog-file-missing-in-native-package',
'executable-not-elf-or-script'
]
arch: 'i386'
}
},
'passes lintian checks',
'chrome-sandbox is owned by root and has the suid bit',
async outputDir => {
await assertASARDebExists(outputDir)
try {
await spawn('lintian', [path.join(outputDir, 'footest_i386.deb')], {
updateErrorCallback: (err) => {
if (err.code === 'ENOENT' && err.syscall === 'spawn lintian') {
err.message = 'Your system is missing the lintian package'
}
}
})
} catch (err) {
if (!err.stdout) {
throw err
}
const stdout = err.stdout.toString()
const lineCount = stdout.match(/\n/g).length
if (lineCount > 1) {
throw new Error(`Warnings not overriding:\n${stdout}`)
}

const output = await spawn('dpkg-deb', ['--contents', path.join(outputDir, 'footest_i386.deb')])
const entries = output.split('\n').map(line => line.split(/\s+/))

const chromeSandbox = entries.find(entry => entry[5].endsWith('/chrome-sandbox'))
if (chromeSandbox === undefined) {
throw new Error('Could not find chrome-sandbox')
}

const permissions = chromeSandbox[0]
chai.expect(permissions).to.equal('-rwsr-xr-x')

const owner = chromeSandbox[1]
chai.expect(owner).to.equal('root/root')
}
)

Expand Down