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

Use net/url to escape paths in web-ui #2092

Closed
wants to merge 1 commit into from
Closed

Use net/url to escape paths in web-ui #2092

wants to merge 1 commit into from

Conversation

adrian-bl
Copy link
Contributor

This change uses net/url to escape href's in the gateway directory listing instead of the builtin escaper by html/template which does not escape fragments (#) and other characters such as '?'.

Example:

$ wget -q -O - "http://localhost:8080/ipfs/QmcoMbJVCufZQEs1k3ez56JhHKTK9Npwo5UNmsQiwz7NYg/tis%23is/a'strange%3Fpath" | grep file
            <a href="/ipfs/QmcoMbJVCufZQEs1k3ez56JhHKTK9Npwo5UNmsQiwz7NYg/tis%23is/a%27strange%3Fpath/with%23one-filey%3F.txt">with#one-filey?.txt</a>

Fixes #2061

Note: I'm not sure about what to do with the vendor dir: should i re-publish it to ipfs and use the new hash? i was unable to find (and re-construct, using ipfs add -r) the currently used hash.

License: MIT
Signed-off-by: Adrian Ulrich <adrian@blinkenlights.ch>
@whyrusleeping
Copy link
Member

@adrian-bl for the license signoff thing, you can just amend your commit and force push (saves notification noise).

Also, don't worry about the vendor directory stuff. Thats all going to be 'fixed' after 0.4.0 with gx, so i'll handle it all then.

@adrian-bl
Copy link
Contributor Author

@whyrusleeping ok, thanks. I already configured a git hook, so it shouldn't happen anymore anyways :-)

// custom template-escaping function to escape a full path, including '#' and '?'
urlEscape := func(rawUrl string) string {
pathUrl := url.URL{Path: rawUrl}
return pathUrl.String()
Copy link
Member

Choose a reason for hiding this comment

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

this looks right to me

@jbenet
Copy link
Member

jbenet commented Dec 17, 2015

@adrian-bl this LGTM! thanks!

i think we want a test to go in with this PR though to ensure it's not broken in the future.

cc @cryptix as he knows this area better than me.

@ghost
Copy link

ghost commented Dec 17, 2015

though not sure where the directory listing stuff is tested, if at all.

Same file :) The "backlinks" related code

@ghost
Copy link

ghost commented Dec 17, 2015

Does this also fix request URIs containing escaped characters, as in #2061 (comment)?

@adrian-bl
Copy link
Contributor Author

@lgierth Yes, as net/url escapes the whole path according to RFC 3986, so the percent sign itself will be escaped to '%25':

$ echo bar >  'funky%20named+file.txt'
$ ipfs add -r .
added QmYNmQKp6SuaVrpgWRsPTgCQCnpxUYGq76YEKBXuj2N4H6 foo/funky%20named+file.txt
added QmTU5nJJEHaGSjDgqtawBSVUWmDbx9PPkoExaZbFkDNHxm foo
$ wget -O - http://localhost:8080/ipfs/QmTU5nJJEHaGSjDgqtawBSVUWmDbx9PPkoExaZbFkDNHxm|grep named
<a href="/ipfs/QmTU5nJJEHaGSjDgqtawBSVUWmDbx9PPkoExaZbFkDNHxm/funky%2520named&#43;file.txt">funky%20named&#43;file.txt</a>

About the testing: IMO, the simplest way to test this would be to rename the 'foo' directory into something that needs escaping (adding a new directory would just duplicate most of the testing code).

I hacked this up here: https://gist.github.com/adrian-bl/74fb3eb3a91ee58691a5
The code then verifies the correct url AND html escaping of the 'foo? #<' node.
Does this look ok?

@ghost ghost added the topic/gateway Topic gateway label Dec 22, 2015
@daviddias daviddias removed the backlog label Jan 2, 2016
@rht
Copy link
Contributor

rht commented Jan 22, 2016

I just verified this pr by testing the directory listing directly; it works.
@adrian-bl you should include the test file from the gist.
Also, this needs a rebase.

This test can be added to sharness either here or in dir-index-html repo

test_expect_success "gateway dir listing escapes the characters" '
    mkdir -p ttt &&
    echo foo >ttt/@\#\#_\? &&
    ipfs add -r ttt &&
    curl 'localhost:8080/ipfs/QmantKYPCRwwifWxr3DcrqrStqULS4vJ7x1n9R8B4gCSrJ/' >curl_out &&
    grep "QmantKYPCRwwifWxr3DcrqrStqULS4vJ7x1n9R8B4gCSrJ/@%23%23_%3F" curl_out
'

@jbenet
Copy link
Member

jbenet commented Jan 24, 2016

@rht thanks for resurfacing this + the review. want to pick this up and rebase the PR on top of master + add test?

@whyrusleeping
Copy link
Member

ping @noffle, think you could pick this up?

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

Successfully merging this pull request may close these issues.

5 participants