gateway: make IPNSHostname work on responses too #1577
Conversation
|
@lgierth i think i get it, but to be sure, can maybe describe a bit more in the code as a comment somewhere? or maybe want a test case that shows the cases it's supposed to be manipulating? |
|
I clarified the comments.
Yes agreed, if we start serving our website using this, we better make sure it does what it says on the tin. |
|
Yep. 👍 — On Sat, Aug 15, 2015 at 3:17 PM, Lars Gierth notifications@github.com
|
| t.Logf("k: %s\n", k) | ||
| ns["/ipns/example.net"] = path.FromString("/ipfs/" + k.String()) | ||
|
|
||
| req, err := http.NewRequest("GET", ts.URL+"/ipfs/"+k.String(), nil) |
|
There are a few unneccessary comments and a bit of duplication in there, I can clean that up, or we leave it for the great gateway bonfire (go-ipfs-gateway). |
| // we could use originalUrlPath like we do with the redirect, | ||
| // but i don't wanna break the switch-case right above, not now. | ||
| if urlPath != originalUrlPath { | ||
| backLink = "/" + strings.Join(pathSplit[4:], "/") |
There was a problem hiding this comment.
is that the only case where urlPath != originalUrlPath? because if not, we might panic the process (by accessing pathSplit[4:].
Maybe:
if urlPath != originalUrlPath && pathSplit[1] == "ipns" {
backLink = "/" + strings.Join(pathSplit[4:], "/")
}also, shouldn't it be pathSplit[3:] (3) ?
There was a problem hiding this comment.
Yeah I'm piggybacking "path was touched by IPNSHostname" here, will change.
yeah should be fine |
|
@lgierth ok, CRed o/ -- a few comments |
| originalUrlPath := urlPath | ||
| hdr := r.Header["X-IPNS-Original-Path"] | ||
| if len(hdr) > 0 { | ||
| originalUrlPath = hdr[0] |
There was a problem hiding this comment.
maybe want to set here: ipnsHostRedirection = true and use that for the conditions below?
There was a problem hiding this comment.
Yeah I introduced a variable in order to avoid the piggybacking. I also fixed backlink edge-cases and added more tests for them.
It should be good to go now.
|
@lgierth LGTM! one small comment above o/, and this:
after that, Ready For Merge, i think |
License: MIT Signed-off-by: Lars Gierth <larsg@systemli.org>
IPNSHostnameOption() touches the URL path only on the way in, but not on the way out. This commit makes it complete by touching the following URLs in responses: - Heading, file links, back links in directory listings - Redirecting /foo to /foo/ if there's an index.html link - Omit Suborigin header License: MIT Signed-off-by: Lars Gierth <larsg@systemli.org>
|
Here we go! I don't know if any of the previous commits failed, but I squashed them regardless. That Travis failure is from my jumping between lgierth:gateway-host-header and ipfs:gateway-host-header... |
|
looks like one of travises OSX machines failed pretty badly. Other than that, the tests look fine. |
|
This LGTM! |
gateway: make IPNSHostname work on responses too
IPNSHostnameOption() touches the URL path only on the way in,
but not on the way out. This commit makes it complete by
touching the following URLs in responses:
License: MIT
Signed-off-by: Lars Gierth larsg@systemli.org