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

make windows work to some degree #83

Closed
wants to merge 2 commits into from

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Nov 25, 2024

Hi,

I'm working on a similar extension (too much wip that I would put it to marketplace anytime soon), let's get yours working on windows.

Fixes #78

I didn't figure out a way around your log file redirection today, but now it no longer throws the mentioned error and didn't want to discard my codespace.

What seems to work good is the dependency setup, I would exclude your gif from your vsix as this make the whole package oversized.

@ChristopherHX ChristopherHX marked this pull request as ready for review November 25, 2024 20:32
@SanjulaGanepola
Copy link
Owner

Thanks @ChristopherHX for opening this PR. I spent some time looking into the linked issue and came up with a solution that should address this (#85) and it removes pipefail all together. I opted for this solution because I planned on moving towards CustomExecution with a child process to eventually support #69.

If you have some time, could you try out this new PR to see if there are any unaddressed issues related to this?

@SanjulaGanepola
Copy link
Owner

@ChristopherHX I am going to go ahead with closing this PR as these issues were resolved with the previous PRs which are now merged.

@ChristopherHX
Copy link
Contributor Author

Shure yes this is fine

The reason why I prever the args array instead of shell execute is for example following:

  • Store $PATH in the vars settings tree
  • Notice that it contains your systems path variable in your workflow
  • Store Hello World and notice World is missing, where did it end up?

There are many other ways to do shell injection
"$(sh -c 'echo exploit!')" in shell execute.

Your settings of act command beeing a string field make a transition not so easy.
Only windows uses an args string to create child processes and is therefore at most an optional platform dependend parameter in golang and node that use args arrays.

While dotnet coming from windows prefers the windows api surface even on linux and want always an args string.

The args code for some of your act parameter could be later recycled by you or me.

@SanjulaGanepola
Copy link
Owner

notice World is missing, where did it end up?

Just gave it a try as well, and indeed you are right there is an issue here when there are spaces. Is there an issue if we simply wrap the value in double quotes. So instead of --var MY_VAR=Hello World its --var MY_VAR="Hello World"?

The args code for some of your act parameter could be later recycled by you or me.

We should probably fix this soon or else setting these parameters is pretty much broken. I suppose PR #68 could also be impacted by this because some of these option values can have spaces as well.

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.

"Cannot bind parameter 'Option'" When running workflow
2 participants