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: disable app sandbox when --no-sandbox is present #184897

Merged
merged 2 commits into from Jun 12, 2023

Conversation

deepak1556
Copy link
Contributor

@deepak1556 deepak1556 commented Jun 12, 2023

Refs #184687

@deepak1556 deepak1556 requested a review from bpasero June 12, 2023 10:26
@deepak1556 deepak1556 self-assigned this Jun 12, 2023
@deepak1556 deepak1556 added this to the June 2023 milestone Jun 12, 2023
@deepak1556 deepak1556 enabled auto-merge (squash) June 12, 2023 10:27
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I would have expected us to simply go strict for --no-sandbox and not introduce a new thing --sandbox to keep the change small and match our expected args.

@deepak1556
Copy link
Contributor Author

@bpasero it is not possible to rely on the boolean attribute of no-sandbox because of how minimist works, see

// Minimist incorrectly parses keys that start with `--no`
// https://github.com/substack/minimist/blob/aeb3e27dae0412de5c0494e9563a5f10c82cc7a9/index.js#L118-L121
// If --no-sandbox is passed via cli wrapper it will be treated as --sandbox which is incorrect, we use
// the alias here to make sure --no-sandbox is always respected.
// For https://github.com/microsoft/vscode/issues/128279
'no-sandbox': { type: 'boolean', alias: 'sandbox' },
for reference. We have been aliasing sandbox to no-sandbox, so this PR does not introduce a new flag. It works like this,

code <folder> // sandbox: true, no-sandbox: undefined (from minimist)

code <folder> --no-sandbox // sandbox: false, no-sandbox: false (from minimist)

@deepak1556 deepak1556 merged commit 02ec02e into main Jun 12, 2023
6 checks passed
@deepak1556 deepak1556 deleted the robo/fix_sandbox_parsing branch June 12, 2023 13:01
@bpasero
Copy link
Member

bpasero commented Jun 12, 2023

Thanks!

deepak1556 added a commit that referenced this pull request Jun 13, 2023
* fix: disable app sandbox when --no-sandbox is present (#184897)

* fix: loading minimist in packaged builds
@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants