Skip to content

Conversation

BenceSzalai
Copy link

@BenceSzalai BenceSzalai commented Sep 15, 2025

Should address #1609, without replacing exec with spawn as #1610 did.

Escaping any argument could be a rather complex thing, but here we only need to handle paths, and that simplifies the solution, because:

  • if there is no " in the path we can always just wrap it in "s
  • on windows " is not allowed in file names so with this we don't need to think about weird ways of escaping (like ^" or "")
  • also, if the path does not contain ' we can wrap it in 's and call it a day
  • every other major systems agree on escaping " in shell by \", so if both " and ' is present in the path, we replace occurrences of " with \" and wrap the result in '

Also note that BackstopJS remote so far would simply fail for anyone with either ' or " in their path, just like with spaces, so basically no matter what the wrapPath() function does, it cannot really make things worse for them.

On the other hand I could make wrapPath() return the path without any wrapping if it did not contain a space, that way ensuring exact same behaviour as so far, but with that it'd still be broken for ' and " in the path, so this is a bit more risky, but this way it'll not break with quotation marks in the paths neither.

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.

1 participant