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

{q} isn't escaped properly on Windows #3789

Closed
4 of 10 tasks
ibhagwan opened this issue May 13, 2024 · 12 comments
Closed
4 of 10 tasks

{q} isn't escaped properly on Windows #3789

ibhagwan opened this issue May 13, 2024 · 12 comments
Labels

Comments

@ibhagwan
Copy link

Checklist

  • I have read through the manual page (man fzf)
  • I have searched through the existing issues
  • For bug reports, I have checked if the bug is reproducible in the latest version of fzf

Output of fzf --version

0.52.0 (bcda25a)

OS

  • Linux
  • macOS
  • Windows
  • Etc.

Shell

  • bash
  • zsh
  • fish

Problem / Steps to reproduce

Follwing up on #3626, we now have a different issue.

I found the current escaping sequence isn't yet working properly, while the command described in #3626 (basically, "live grep") is working it will only work when {q} is the last argument of the command.

Consider we want to display rg error messages inside fzf, we have to redirect the error stream to stdout by adding 2>&1:

fzf --ansi --disabled --bind="change:reload:rg --line-number --column --color=always {q} 2>&1"

Now if we have an open \ we see the error inside fzf (instead of "Command failed: ... "):
image

But since {q} isn't escaped properly if we try to search for a single " , the command fails:
image

Another use-case where this fails is if we want to silently fail rg and never display "Command failed", we could do that by adding || break (similar to || true on UNIX):

fzf --ansi --disabled --bind="change:reload:rg --line-number --column --color=always {q} || break"

Again, if we search for " the command will fail:
image

@ibhagwan
Copy link
Author

FYI, the last working version is 0.50, due to the fact the " is prefixed with a caret ^.

Using the below command we fail on purpose so we can look at the escaping sequence:

fzf --ansi --disabled --bind="change:IF {q}"

0.52 escape sequence
image

0.50 escape sequnece
image

@junegunn
Copy link
Owner

Searching for " works for me.

It works on cmd.exe,

image

And it works with :RG command of fzf.vim

image

I'm testing this on an EC2 Windows 16 server instance. Not sure if things are different on different versions of Windows.

@ibhagwan
Copy link
Author

ibhagwan commented May 13, 2024

Searching for " works for me.

Does it also work with (when {q} isn't the last argument of the cmd)?

fzf --ansi --disabled --bind="change:reload:rg --line-number --column --color=always {q} 2>&1"

@ibhagwan
Copy link
Author

@junegunn, you didn't test my use-case properly, I can see the command you ran in the screenshot:

THIS WORKS

fzf --disabled --preview-window up --preview "rg --line-number --column --color=always -- {q}"

BOTH WORK ON 0.50 AND FAIL ON 0.52

fzf --disabled --preview-window up --preview "rg --line-number --column --color=always -- {q} 2>&1"
fzf --disabled --preview-window up --preview "rg --line-number --column --color=always -- {q} || break"

The reason I need to add 2&1 || break is explained in the OP.

@ibhagwan
Copy link
Author

ibhagwan commented May 13, 2024

@junegunn, the current escape sequnce also fails when searching for special characters prefixed by ", try:

fzf --disabled --preview-window up --preview "rg --line-number --column --color=always -- {q}"

Search for "\|

image

Search for ".*&, the & part of the regex is ignored, effectively searching for ".* only
image

Similarly serching for "\^ ignores the caret
image

Searching for "< or ">
image

fzf.vim's RG will also fail with these examples:
image

image

@junegunn
Copy link
Owner

Thanks for the input. Will check again later, for some reason I can't connect to a Windows EC2 instance. 🤷

@junegunn
Copy link
Owner

junegunn commented May 13, 2024

Thanks for reporting this serious issue. 0.52.1 is a (hurried) attempt to fix the issue. Please test if it helps. I'm sure there are some other issues lurking, but at least it should be safer than what we had in 0.52.0.

@ibhagwan
Copy link
Author

Thanks for reporting this serious issue. 0.52.1 is a (hurried) attempt to fix the issue. Please test if it helps. I'm sure there are some other issues lurking, but at least it should be safer than what we had in 0.52.0.

Tysm @junegunn! I have seen some other oddities happening but will have to test this throughly so I can pinpoint the exact issue and whether this is a bug or just different behavior for which fzf-lua will have to adjust.

Will report back tomorrow.

@ibhagwan
Copy link
Author

@junegunn, everything seems to be in order at first glance, but I think that ; also needs to be escaped with a caret.

Consider the following which attempts to run rg only when the user inputs a non-empty string (to avoid feeding all project lines unnecessarily):

fzf --ansi --disabled --bind="change:reload:IF {q} NEQ \^"^^\" rg --line-number --column --color=always {q}"

The condition works fine in all special characters I tested aside from ;, using ^; (in the code) solves the issue:
image

I believe ; should be added to the list of special characters in:

var escapeRegex = regexp.MustCompile(`[&|<>()^%!"]`)

So that it will later be preceded by ^ in:

return escapeRegex.ReplaceAllStringFunc(string(b), func(match string) string {
return "^" + match
})

@junegunn
Copy link
Owner

So I guess the problem with ; is relatively benign compared to the previous flaw, which would allow all kinds of crazy stuff?
Didn't know ; needed escaping. I'll address it in the next release. But I'm not going to rush it and take some time to hear from other Windows users (#3790).

In the meantime, maybe you could work around it by using a batch file and not directly writing conditionals in the binding expression? Just guessing.

@ibhagwan
Copy link
Author

So I guess the problem with ; is relatively benign compared to the previous flaw, which would allow all kinds of crazy stuff? Didn't know ; needed escaping. I'll address it in the next release. But I'm not going to rush it and take some time to hear from other Windows users (#3790).

I also had no idea ; was a meta character, definitely no rush in this it’s totally benign.

In the meantime, maybe you could work around it by using a batch file and not directly writing conditionals in the binding expression? Just guessing.

I already have this handled in the lua wrapper this was just something that was popping up in the native versions of the pickers, it’s an edge case of an edge case.

@junegunn
Copy link
Owner

Good to hear that, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants