Fix buggy partial startup on macOS due to flag added by Gatekeeper/XProtect #21861

DeeDeeG · 2021-01-12T21:23:47Z

Requirements for Contributing a Bug Fix (from template, click to expand)

Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
The pull request must only fix an existing bug. To contribute other changes, you must use a different template. You can see all templates at https://github.com/atom/.github/tree/master/.github/PULL_REQUEST_TEMPLATE.
The pull request must update the test suite to demonstrate the changed functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/.github/tree/master/CONTRIBUTING.md#pull-requests.

Identify the Bug

Deeper technical explanation (click to expand)

[This bug appears] related to Gatekeeper.

[This] bug happens [when you] are launching Atom and see Gatekeeper pop up with a loading bar and a message 'Verifying "Atom"', or something about "This was downloaded from the internet, are you sure you want to open this?". This bug happens when Gatekeeper [intervenes in] launching the app [but the user clicks though/the app is verified and launching continues].

I see a command-line flag -psn_0_[some six or seven digits here] (something like -psn_0_1234567) applied to the buggy Atom process, when inspecting using htop or ps -ax. I have seen reports elsewhere that this flag is added by Gatekeeper. For example: godotengine/godot#37719

Our argument parser library yargs thinks this -psn_0_1234567 flag is a bunch of short flags, equivalent to -p, -s, -n, -_, -0, -_, -1, -2, -3, -4, -5, -6, -7. The -_ are converted to boolean true, and then we try to do JS string-only operations on the flags. Doing true.startsWith() results in a TypeError ("path.startsWith is not a function") very early in our initialization process, leaving the app barely started up at all, and unable to respond to most kinds of input other than quitting.

Quitting and restarting means Gatekeeper is already familiar with the binary, and it won't intervene with launching the app. So the flag doesn't get added, and Atom works fine after a restart.

Fixes: #21013 (probably)
Fixes: #21751
Fixes: #21820

Description of the Change

Filter out a flag (-psn_0_[1234567]) that macOS Gatekeeper/XProtect adds to Atom. Filter it out before yargs tries to parse it.
- More-precisely: Filter out any argument passed to Atom that starts with -psn_.
- (This flag is bogus input for our purposes. yargs thinks this is many shortflags (-p -s -n -_ -0 -_...); Parsing and handling all of them without it causing trouble is somewhat difficult. Filtering it out early avoids all the complications and bugs (TypeErrors, partial startups).)
Also, as a further step to avoid problems from potentially similar flags in the future, completely ignore non-string items in the args._ array, in one of the functions that decides whether an argument is a path or a URI.

Alternate Designs

We could use just one of these two fixes instead of both. I included both to be thorough and handle more potential situations more correctly.

Miscellaneous other proposals (click to expand):

I also considered converting number arguments to strings, so users can easily open a file like 55 or 100 in the current directory (no file extension or relative/absolute path needed). But I left that out because it felt like it was veering off-topic for a bug-fix. (yargs has an option that supposedly would do that for us, but it doesn't help very much in my testing; we would still need to filter out the zero-length strings it converts boolean true to with that option, and we would still need to manually convert numbers to strings, because that part doesn't work. I think that's a bug in yargs.)

Possible Drawbacks

None.

(I expected tiny slowdowns, but the relevant tests run at the same ~16-18ms speed on this branch as master branch on my machine.)

Verification Process

Added tests to describe new expected behavior. Tests pass.

macOS Gatekeeper flag no-longer prevents Atom from completely starting up.

To reproduce the problem, run this in the Terminal app: atom "-psn_0_1234567". You should see the partial startup bug as described in the issues (linked at the top of this PR).

Try again on this branch to confirm the bug goes away; For example, using Atom in dev mode:

cd atom
git remote add deedeeg https://github.com/DeeDeeG/atom
git remote update deedeeg
git switch bugfix-deal-with-macos-gatekeeper-flag
./out/Atom\ Dev.app/Contents/MacOS/Atom\ Dev --dev --resource-path=/Users/[you]/path/where/you/cloned/atom -psn_0_1234567

Release Notes

Fixed buggy partial startup on macOS due to flag added by Gatekeeper/XProtect

We don't need this flag, and yargs thinks it is many short flags. So, we filter it out before it causes too much trouble. ("Trouble" meaning TypeErrors, partial startup, no windows opened...) --- Background info: When launching Atom on macOS, if Gatekeeper/XProtect intercepts the app launch, and the user clicks through/if launching continues, Gatekeeper/XProtect add a flag of the form: `-psn_0_[1234567]`. (The six or seven digits at the end are apparently a "Process Serial Number", which the internets say dates back to the old "Carbon" framework...) We should filter this out at some point, as it's not desirable input. Ideally, we should filter it out before it gets to yargs. (Otherwise yargs will see it as twelve or thirteen short flags: `-p`, `-s`, `-n`, `-_`, `-0`, `-_` (again), `-1`... which is a lot messier and harder to sort out than a single string.) --- Fixes: atom#21013 (probably) Fixes: atom#21751 Fixes: atom#21820

(This flag gets added by Gatekeeper/XProtect sometimes, but it's not wanted, and it trips up yargs, so we filter it out).

The parseCommandLine() function only accepts an array now.

This is an additional workaround for potential arbitrary flags starting with `-` and including an underscore `_`. The idea is to keep starting up with whatever valid inputs were given, rather than quietly fail to initialize after getting a TypeError.

DeeDeeG · 2021-01-12T21:32:21Z

@sadick254 and @Aerijo this is another bugfix PR for macOS. Please take a look if you have time.

I am confident this works well, but as for the particular approach, I would be interested in any feedback for this implementation and feedback on the tests I added.

For example: I added somee of the filtering in one function, but there is a different function I didn't update that handles these inputs if the --uri-handler flag is passed to Atom. Since the checks and filtering added in this PR ended up not being noticeably slower than master, I could move the filtering out of the one function to earlier before either of the two functions.

Also, converting number arguments (for example: 55 100) to strings would let you open files with those names. No extension or path hints (./55, /absolute/path/to/55, 55.txt) needed. I think that could be neat, but fairly niche in usage and arguably off-topic for this PR. But I could add that in, or do it as another PR.
Example implementation: DeeDeeG@874af56

As mentioned in the body of the PR, this PR includes two similar and largely redundant fixes, but as I also point out this hasn't resulted in slowdowns, and I think safer, more-cautious code during startup may be worth it. That said, I would understand a "Keep It Simple" approach and so I wonder if just one of these solutions is adequate.

DeeDeeG · 2021-01-12T22:30:15Z

Also I understand if looking deep into this bugfix PR isn't terribly interesting. I tested this implementation a lot, and it works, and it's fast. So, no worries if not interested in giving in-depth feedback for this admittedly simple bugfix.

Aerijo · 2021-01-13T06:16:57Z

I expected tiny slowdowns, but the relevant tests run at the same ~16-18ms speed on this branch as master branch on my machine

Working at all is better than sometimes failing for the sake of 'performance'
If filtering an array of (like 3 / 4 ?) values once, in the entire app lifetime, caused a measurable slowdown, there are bigger problems here.

I was interested in how VS Code handles -psn_..., but it seems they don't. I guess they just type check before using values.

And I would like to see the positional arguments always be treated as strings, but that can go in a different PR.

Aerijo · 2021-01-13T06:16:39Z

spec/main-process/parse-command-line.test.js

      assert.deepEqual(args.pathsToOpen, ['/some/path']);
    });
+
+    describe('and --_ or -_ are passed', () => {


Why single out and ignore underscore flags? I guess it's related to -psn_... containing an underscore? Is there a drawback to just allowing (and ignoring) it?

Passing -_ or --_, as either the last argument, or "without a non-flag argument immediately after", is the minimal reproducer for the Gatekeeper partial startup bug.

Maybe I should add this as a code comment in the test file? Or incorporate that into the test's describe/it text?

yargs stores non-flag arguments in an array called argv._, but this can easily be polluted with the content of actual underscore _ flags. (See Gatekeeper's -psn_0_[1234567] as a prime example). And yargs treats any flag with "empty content" (no non-flag argument passed right after the flag) as boolean trues.

Passing the "underscore flag" is the main way to get stray boolean trues into the non-flag argument pool. It's an easy and convenient way to test the Gatekeeper flag bug more generally than a flag starting with -psn_.

Restated in terms relative to the normal non-bug behavior: This test handles boolean pollution in the non-flag argument array, which is the one where we look for paths and URIs that Atom should open.

I thought of it as a natural counterpart to the "number pollution" test right below it.

Huge long reply, since I'm not totally sure what else you may need clarifying, and I was kind of on a roll typing this out. And it's late where I am. (click to expand if there's anything else you were wondering, I may have answered it here):

(Before I get into it: Admittedly none of the following complicated information would make sense from just looking at the test as I wrote it. So maybe I should add comments in the test file. Or make the purpose of the test more obvious from the describe and it text. TODO: make the test self-explanatory.)

Also to address just this question directly:

Why single out and ignore underscore flags? I guess it's related to -psn_... containing an underscore? Is there a drawback to just allowing (and ignoring) it?

I felt that it was just a matter of time before we saw a similarly-formed flag. If it didn't happen to start with exactly -psn_, then we wouldn't be filtering it out, and then we're back to experiencing essentially this same issue.

The general solution of handling/ignoring shortflag sequences containing effectively -_... is very simple. It happens to also filter out --_. I may have gone overboard in explicitly testing for both, but yargs treats them the same under the hood right now, the solution to handle all of this comprehensively is a few lines of code, and I just wanted to be thorough. I figured being mildly paranoid about it now would make our code and test coverage robust to whatever strange inputs may come, and alert us to whatever undocumented minor behavior changes may happen in yargs, for instance.

So, the reason I went there at all is a bit of a rabbit-hole of how yargs works. Well, it seems like a rabbit-hole if you don't know where it ends up. I will try to make this concise but complete.

Background info: what's the normal, desired behavior?

Starting with the normal, desired behavior: yargs stores all non-flag arguments in an array called argv._.

So, Atom (rather reasonably) expects non-flag arguments to be filenames or URLs we want to open. For example:

Running atom file1 ./file2 /path/to/file3 will launch Atom with tabs open for these files.

Quirks in yargs

A few things make it complicated:

Users and the OS can specify a literal _ flag of either the short form -_ or --_, and yargs will dump it right into the same argv._ array with the "non-flag" arguments.

yargs by default treats any flag with no (non-flag) argument after it as boolean true

Bonus complication that's not strictly a part of the bug I'm trying to fix: Any bare number passed as a non-flag argument, say 33, will be turned into a JS number. (There is a feature to treat everything in yargs.argv._ as strings, but it didn't work for numbers in my testing. It didn't coerce numbers to strings in my testing, it left them as JS number types. I think this is a bug in yargs).

Side note: (yargs seems to follow a Linux-style convention about shortflags. It's a conventional behavior on Linux to treat a string of characters after a single dash as being a number of shortflags. For example, a typical tar invocation: tar -xvf my_tarball.tgz; in English that's "tar, extract verbosely a file named my_tarball.tgz.")

The purpose of that test (filtering out booleans)

The purpose of that test is to test an array of inputs that has been polluted with booleans. This is the scenario that the -psn_0_[1234567] flag presents.

Passing -_ or --_, without a non-flag argument immediately after, is the minimal reproducer for the Gatekeeper partial startup bug.

Why I didn't just pass booleans into the parser function

Passing non-strings to this function doesn't resemble its real-world use. The function is only used in one place. Here in src/main-process/start.js. Atom always, 100% of the time, passes the parseCommandLine() function either an empty array, or an array of strings. These are read from Node's process.argv immediately before passing to the parseCommandLine() function. Node's process.argv will only ever contain strings, apparently. The diverse typing come in only after yargs has taken a look at the inputs and decided whether they are strings, numbers, booleans...

Makes sense. I think a short comment on that test to explain why would be useful, even if it's just a link to the conversation in this PR.

(turns out yargs has an open bug related to this too yargs/yargs#878)

DeeDeeG · 2021-01-13T09:05:51Z

I was interested in how VS Code handles -psn_..., but it seems they don't. I guess they just type check before using values.

I suppose total and strict coercion to strings (and ignoring/filtering out the things that shouldn't become strings) would be adequate. I found that surprisingly involved/difficult to do, based on how yargs was acting.

Particularly, the option to treat everything as strings seemed to miss bare numbers (they stayed number type), and the booleans turned into zero-length strings that would trigger a blank tab/new document to be opened in Atom (just one blank tab, though, so it's not bad compared to "the app doesn't finish starting up"). So we are back to explicitly filtering out numbers and deleting what would have been the boolean trues, deleting them as zero-length strings instead. (That admittedly sounds safer, in a way; never even touching a boolean is one small additional degree away from TypeErrors).

I didn't use this feature as it added a line of code and some processing time in the yargs library to practically almost zero effect. (The effect is this PR would be filtering out zero-length strings, not booleans).

DeeDeeG · 2021-01-13T23:01:48Z

Thinking of how VSCode doesn't specifically handle this flag:

The supposed disadvantage of that for us would be letting all the psuedo-random number shortflags through. (-1, -2... whatever the Process Serial Number happens to be.)

We don't use any of these for anything right now. But I'm thinking ahead that we might want to use them for something in the future? That's a hypothetical concern as of the moment though. In actuality there's not much real-world-relevant justification I can give for specifically and specially handling that flag.

(It's fast/practically free performance-wise. So there's not really any harm either.)

I'm running out of things to discuss on this one, so I think it might be ready.

ThatXliner · 2021-01-13T23:05:00Z

If your comparing it to VS CODE, you might as well just ~~plagiarise~~ copy the code because that’s what coders do.

DeeDeeG · 2021-01-13T23:29:50Z

It's more food for thought/sanity check, two heads are better than one, etc. Considering the reasonable alternatives. But I sense you may not be entirely literally serious in your comment.

DeeDeeG · 2021-01-13T23:32:03Z

Test failure is unrelated. It's the timeout on the pane-spec test on the Windows renderer-1 tests job again.

I've seen that on a number of unrelated branches.

DeeDeeG · 2021-01-16T00:35:43Z

Here's the other function that processes the array meant to contain paths and URIs. Just like the function I did add protections to, this one also absolutely expects its inputs to be strings (or it gives TypeErrors). It is the --uri-handler function:

https://github.com/atom/atom/blob/v1.54.0/src/main-process/parse-command-line.js#L137-L144

I left this function alone and didn't add any protections to it, because I wanted a small diff and to keep the parser simple and readable, and because in my testing, this flag is only organically used on Windows, so the offending macOS-only bug is not applicable.

Miscellaneous notes on this (click to expand):

When a web browser uses Atom as an atom:// URI handler (for example, from a package's page at https://atom.io/packages, it only passes two flags: --uri-handler and atom://settings-view/show-package?package=[insert package name here]
- This constrained scenario has no obvious way to give bad inputs to the --uri-handler function. It should be safe enough for real-world use.
In theory, it would be good to handle non-string inputs to this function, in case someone is making some bash/batch scripts to take advantage of this flag. Or has a habit of using atom:// URIs from the command-line. But that is both a niche use-case, and a scenario in which the person writing the script or manually composing the flags can do some due diligence in order to avoid passing non-strings.
Surprisingly, the Linux app doesn't even try to register itself to the OS as a handler of atom:// URIs? Should probably add this functionality if possible.
On macOS, paths and URIs are not handed as CLI arguments to Atom. There are apparently macOS APIs that trigger Electron events open-file or open-url that give the URL or file to Electron/Atom. So this function is never "organically" used on macOS. Only via typing it manually, or potentially via scripting, I suppose.
This flag is not publicly documented, so you'd pretty much have to be hacking on Atom to be aware of the possibility of using it. And this flag wouldn't bring most users any advantage over not using this flag (Atom will happily process an atom:// URI from the command-line or from a script without setting the --uri-handler flag), so the chance of this being a popular thing to do are slim.
This flag is also not part of a public API, so by implication we've not made any guarantees about supporting its use in unexpected ways.

(That's the last topic I can think of to discuss here. Recapping some previous comments I posted: I'm on the fence about whether to remove the specific filter for -psn_ flags, but I'm of the opinion the general fix for ignoring non-string inputs in the main path/URI parser function should definitely stay. Open to any feedback or alternate opinions though.)

DeeDeeG · 2021-01-18T16:44:22Z

@sadick254 Thank you for merging!

…rotect (atom#21861) macOS Gatekeeper adds a flag ("-psn_0_[six or seven digits here]") when it intercepts Atom launches. This happens for fresh downloads, new installs, or first launches after upgrading). We don't need this flag, and yargs interprets it as many short flags. So, we filter it out. (cherry picked from commit 7928a1b)

* 🍎 Don't quit upon 'window-all-closed' event on macOS (#21847) * 🍎 Don't quit on 'window-all-closed' on macOS This prevents a lot of buggy partial-quitting stuff from happening. Fixes: #17672 Fixes: #20831 (cherry picked from commit 72709dc) Co-authored-by: Benjamin Gray <[email protected]> * Fix buggy partial startup on macOS due to flag added by Gatekeeper/XProtect (#21861) macOS Gatekeeper adds a flag ("-psn_0_[six or seven digits here]") when it intercepts Atom launches. This happens for fresh downloads, new installs, or first launches after upgrading). We don't need this flag, and yargs interprets it as many short flags. So, we filter it out. (cherry picked from commit 7928a1b) Co-authored-by: Benjamin Gray <[email protected]>

DeeDeeG added 5 commits January 11, 2021 14:03

spec: Add test for ignoring -psn_0_[1234567] flag

fb90aa3

(This flag gets added by Gatekeeper/XProtect sometimes, but it's not wanted, and it trips up yargs, so we filter it out).

spec: Fix two command-line behavior tests

735d6ac

The parseCommandLine() function only accepts an array now.

Add tests for boolean and number CLI arguments

2a38a2a

sadick254 added the triaged label Jan 12, 2021

DeeDeeG added 2 commits January 12, 2021 17:37

spec: Fix lints, remove unused getPsnFlag() declarations

1b42aa3

src/main-process/parse-command-line.js: Fix lint

3036d4a

Aerijo reviewed Jan 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

spec: Add comment to explain boolean filtering test

53db9df

sadick254 approved these changes Jan 18, 2021

View reviewed changes

sadick254 merged commit 7928a1b into atom:master Jan 18, 2021

DeeDeeG mentioned this pull request Feb 10, 2021

Backport macOS startup and shutdown fixes to Atom 1.55 #21948

Merged

DeeDeeG mentioned this pull request Feb 16, 2021

Atom does not open from clicking a file when Atom is already closed #21826

Open

Fix buggy partial startup on macOS due to flag added by Gatekeeper/XProtect #21861

Fix buggy partial startup on macOS due to flag added by Gatekeeper/XProtect #21861

Uh oh!

Conversation

DeeDeeG commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Identify the Bug

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Release Notes

Uh oh!

DeeDeeG commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeeDeeG commented Jan 12, 2021

Uh oh!

Aerijo commented Jan 13, 2021

Uh oh!

Aerijo Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Background info: what's the normal, desired behavior?

Quirks in yargs

The purpose of that test (filtering out booleans)

Why I didn't just pass booleans into the parser function

Uh oh!

Aerijo Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeeDeeG commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

DeeDeeG commented Jan 13, 2021

Uh oh!

ThatXliner commented Jan 13, 2021

Uh oh!

DeeDeeG commented Jan 13, 2021

Uh oh!

DeeDeeG commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeeDeeG commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeeDeeG commented Jan 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeeDeeG commented Jan 12, 2021 •

edited

Loading

DeeDeeG commented Jan 12, 2021 •

edited

Loading

DeeDeeG Jan 13, 2021 •

edited

Loading

Aerijo Jan 13, 2021 •

edited

Loading

DeeDeeG commented Jan 13, 2021 •

edited

Loading

DeeDeeG commented Jan 13, 2021 •

edited

Loading

DeeDeeG commented Jan 16, 2021 •

edited

Loading