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

allow () paren chars in defensive path check #2295

Closed

Conversation

dginev
Copy link
Collaborator

@dginev dginev commented Jan 4, 2024

Fixes #2293 .

This PR is an alternative - or supplemental - approach to #2294. Here, I still keep the defensive checks as they are, but generally allow the () characters to be present. There may be a good case to merge both PRs.

I also started wondering how many other subtle cases may be silently prevented due to the pathname safety check, so I added a warning. Would be curious to see what variations we see in the next arXiv rerun.

@xworld21
Copy link
Contributor

xworld21 commented Jan 4, 2024

() are special characters for POSIX shells, so pathname_is_nasty is correct in rejecting those before calling pathname_kpsewhich (in fact, it may be time to move the defensive checks inside pathname_kpsewhich – or even better, call kpsewhich in a safe way instead of using backticks).

@xworld21
Copy link
Contributor

xworld21 commented Jan 4, 2024

call kpsewhich in a safe way instead of using backticks

PS: I know how to do this (requires Perl >=v5.8, which I am sure is fine, plus a small extra dependency on Windows because... Windows). I'll send a PR soon.

@xworld21
Copy link
Contributor

xworld21 commented Jan 4, 2024

...

PS: I know how to do this (requires Perl >=v5.8, which I am sure is fine, plus a small extra dependency on Windows because... Windows). I'll send a PR soon.

Scratch that: I have already done that! See

if ($kpsewhich && open(my $resfh, '-|', $kpsewhich, @candidates)) {

The open call uses the list form, which does not spawn a shell, and is thus safe in the face of arbitrary file names. Well, except for Windows – let me send a Windows-specific PR.

So it's actually safe to remove pathname_is_nasty altogether, modulo the Windows change.

@dginev
Copy link
Collaborator Author

dginev commented Jan 4, 2024

Right, the nasty check has been trying to reject both malformed and suspect paths historically. I don't think we want to accept say a path

$(dirname $(kpsewhich latex.ltx))/..

but your issue rightfully pointed out that parens on their own are acceptable literal characters, as with article/figures (1200x1800px). The parens would have to be properly escaped if any system calls are made, agreed. But I think that was already the case for paths with spaces in the name, which are also allowed.

@dginev
Copy link
Collaborator Author

dginev commented Jan 4, 2024

Removing reliance on pathname_is_nasty entirely could be seen as code cleanup (since regex checks are always fishy), so that sounds appealing to me.

If we have a consensus way of doing that, might as well?

@xworld21
Copy link
Contributor

xworld21 commented Jan 4, 2024

But I think that was already the case for paths with spaces in the name, which are also allowed

I think spaces didn't work before #1592. Before the PR, kpsewhich was called with backticks and no attempt at escaping: arguments with spaces were safe, but the behaviour was broken.

If we have a consensus way of doing that, might as well?

Let me add: after adding proper escaping on Windows.

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.

kpsewhich fails if parent directory contains parentheses
2 participants