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

fileserver: do not double-escape paths #4447

Merged
merged 3 commits into from
Dec 11, 2021
Merged

Conversation

mohammed90
Copy link
Member

Fixes the issue specified in #4329 (comment).

@mohammed90 mohammed90 added the bug 🐞 Something isn't working label Nov 26, 2021
@mohammed90
Copy link
Member Author

@bmichotte and @ueffel, please check if the patch resolves your issue and doesn't break new stuff.

@ueffel
Copy link
Contributor

ueffel commented Nov 26, 2021

$ ./caddy version
v2.4.7-0.20211126140144-80fbec0102d5 h1:mdBZC644zlsG4dWbTG3YcsU34linp0ODDoMfaogqSB0=

It does not seem to be fixed.
image

I think the escaping here is causing the double encoding. This bit:

u := url.URL{Path: url.PathEscape(name)}

@francislavoie francislavoie added this to the v2.5.0 milestone Nov 28, 2021
@mohammed90
Copy link
Member Author

Now I can see why. While we're escaping it in that line, the template also calls the html function from the text/template package of stdlib:

<a href="{{html .URL}}">
{{- if .IsDir}}
<svg width="1.5em" height="1em" version="1.1" viewBox="0 0 317 259"><use xlink:href="#folder{{if .IsSymlink}}-shortcut{{end}}"></use></svg>
{{- else}}
<svg width="1.5em" height="1em" version="1.1" viewBox="0 0 265 323"><use xlink:href="#file{{if .IsSymlink}}-shortcut{{end}}"></use></svg>
{{- end}}
<span class="name">{{html .Name}}</span>

The documentation of the function is:

html
Returns the escaped HTML equivalent of the textual
representation of its arguments. This function is unavailable
in html/template, with a few exceptions.

I've updated the code. This effectively reverts the buggy earlier PR but keeps the tests, which were passing anyways.

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good 👍

@francislavoie francislavoie merged commit 78b5356 into master Dec 11, 2021
@francislavoie francislavoie deleted the partially-revert-4332 branch December 11, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants