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(core): replace binjumper with cmd script without prompt #1939

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Oct 7, 2020

What's the problem this PR addresses?

Some users are running in protected systems where running unknown binaries is not allowed.

Fixes #1938

How did you fix it?

Revert #1808 and use cmd again but with a workaround to get rid of the Terminate batch job prompt as described in microsoft/terminal#217 (comment) by @rivy

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz
Copy link
Member Author

merceyz commented Oct 7, 2020

@markerikson Could you check if this works for you? It includes the changes from #1934

yarn set version from sources --branch 1939

@arcanis
Copy link
Member

arcanis commented Oct 7, 2020

That will work from scripts, but not if it calls a JS script that calls spawn itself, right? Since it won't get called by powershell.

@merceyz
Copy link
Member Author

merceyz commented Oct 7, 2020

Correct, it only gets rid of the prompt when the bin is spawned by us. Going to add a config to enable the native bins instead of using powershell if @markerikson confirms this works.

@arcanis
Copy link
Member

arcanis commented Oct 7, 2020

Correct, it only gets rid of the prompt when the bin is spawned by us

I'd tend to prefer the setting to fallback on the classic legacy behavior (cmd scripts), not on powershell. If one magic isn't enough, I'm reticent about including a second one (ie locating the powershell binary etc) that has its own drawbacks.

@markerikson
Copy link

I can confirm that this attempts to execute things via Powershell. Unfortunately, the Bit9 security software still flags this as a "suspicious pattern".

It's likely this will work fine for everyone else, and I can try to report this to our internal IT center to see if they can tweak the settings to allow it, but unfortunately it doesn't work for me personally atm.

@merceyz merceyz force-pushed the merceyz/powershell branch from a382b7f to e13193f Compare October 7, 2020 18:32
@merceyz
Copy link
Member Author

merceyz commented Oct 7, 2020

That's what I was afraid of, I've reverted the powershell change and disabled the native binjumper so it should be as it was before now. Mind trying again?

@markerikson
Copy link

Hey, that actually looks like it worked!

Haven't run any of the app yet, but:

➤ YN0000: ┌ Link step
➤ YN0062: │ fsevents@patch:fsevents@npm%3A2.1.2#builtin<compat/fsevents>::version=2.1.2&hash=127e8e The platform win32 is incompatible with this module, building skipped.
➤ YN0062: │ fsevents@patch:fsevents@npm%3A1.2.12#builtin<compat/fsevents>::version=1.2.12&hash=127e8e The platform win32 is incompatible with this module, building skipped.
➤ YN0007: │ nodemailer@npm:6.4.6 must be built because it never did before or the last one failed
➤ YN0007: │ core-js@npm:2.6.11 must be built because it never did before or the last one failed
➤ YN0007: │ core-js-pure@npm:3.6.4 must be built because it never did before or the last one failed
➤ YN0007: │ core-js@npm:3.6.4 must be built because it never did before or the last one failed
➤ YN0000: └ Completed in 14s 498ms
➤ YN0000: Done with warnings in 17s 673ms

and no security software popup warnings this time.

@merceyz
Copy link
Member Author

merceyz commented Oct 7, 2020

Perfect, if you need any help with anything feel free to pop by on discord - https://discord.gg/yarnpkg

@merceyz merceyz force-pushed the merceyz/powershell branch from a9379e3 to 114ec70 Compare October 25, 2020 12:57
@merceyz
Copy link
Member Author

merceyz commented Oct 25, 2020

Hey @markerikson 👋
I'm curious if it was just the node.exe file that got blocked because it was unsigned, so I've made yarn use a hardlink for it and the "normal" flow for everything else, could you try this PR again please?

@merceyz merceyz force-pushed the merceyz/powershell branch 2 times, most recently from cb2b4d0 to a5bedcb Compare November 10, 2020 10:49
@markerikson
Copy link

Hey. I've had to set aside playing around with Yarn v2 for the moment. It's still something I want to get back to, but there's other priorities right now.

@merceyz
Copy link
Member Author

merceyz commented Nov 10, 2020

Totally understandable :)

@merceyz merceyz force-pushed the merceyz/powershell branch 4 times, most recently from 93e09bf to ef43d1f Compare December 2, 2020 16:44
@merceyz merceyz force-pushed the merceyz/powershell branch from 39a9872 to 1b5ef31 Compare December 3, 2020 09:43
@merceyz merceyz changed the title fix(core): use powershell instead of binjumper fix(core): replace binjumper with cmd script without prompt Dec 3, 2020
@merceyz merceyz marked this pull request as ready for review December 3, 2020 14:12
@merceyz merceyz requested a review from arcanis as a code owner December 3, 2020 14:12
@merceyz merceyz mentioned this pull request Dec 10, 2020
11 tasks
@arcanis arcanis merged commit ae72e6f into master Dec 15, 2020
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 this pull request may close these issues.

[Case Study] BinJumper side-effects
4 participants