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.

Sign up for GitHub

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

Jump to bottom

gateway: add path prefix for directory listings #1971

Merged
merged 1 commit into from
Nov 20, 2015
Merged

gateway: add path prefix for directory listings #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions core/corehttp/gateway_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,24 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request

urlPath := r.URL.Path

// If the gateway is behind a reverse proxy and mounted at a sub-path,
// the prefix header can be set to signal this sub-path.
// It will be prepended to links in directory listings and the index.html redirect.
prefix := ""
if prefixHdr := r.Header["X-Ipfs-Gateway-Prefix"]; len(prefixHdr) > 0 {
log.Debugf("X-Ipfs-Gateway-Prefix: %s", prefixHdr[0])
prefix = prefixHdr[0]
}
Copy link
Member

Choose a reason for hiding this comment

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

this SGTM. though not sure if this gives anyone power by putting it as a req header. cant think of an attack now, but there may be one.

Copy link
Author

Choose a reason for hiding this comment

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

I'm unsetting this header for all other vhosts in nginx. The prefix "only" affects rendering of URLs (and redirects), do you think that opens up attack surface in other setups? One thing I should definitely make sure is that you can pass full URLs, but only what's expected (paths). An attacker must not be able to trick a server into arbitrary redirects.

Copy link
Author

Choose a reason for hiding this comment

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

EDIT: can't pass full URLs

Copy link
Member

Choose a reason for hiding this comment

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

ah didnt see this before merging-- does this need a fixup too then? (making sure it's a path)

Copy link
Author

Choose a reason for hiding this comment

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

Yep but I'll get to it today


// IPNSHostnameOption might have constructed an IPNS path using the Host header.
// In this case, we need the original path for constructing redirects
// and links that match the requested URL.
// For example, http://example.net would become /ipns/example.net, and
// the redirects and links would end up as http://example.net/ipns/example.net
originalUrlPath := urlPath
originalUrlPath := prefix + urlPath
ipnsHostname := false
hdr := r.Header["X-IPNS-Original-Path"]
if len(hdr) > 0 {
originalUrlPath = hdr[0]
if hdr := r.Header["X-Ipns-Original-Path"]; len(hdr) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

is this change going to break anyone? or are headers case insensitive?

Copy link
Author

Choose a reason for hiding this comment

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

net/http seems to convert response header keys to camel-case. I passed X-IPFS-Gateway-Prefix from nginx, and it arrived in the gateway handler as X-Ipfs-Gateway-Prefix. I only changed X-IPNS-Original-Path too for the sake of consistency and avoiding happy accidents.

Copy link
Author

Choose a reason for hiding this comment

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

Regarding breaking existing stuff by changing the header: it's implementation detail between IPNSHostnameOption and gateway handler

originalUrlPath = prefix + hdr[0]
ipnsHostname = true
}

Expand Down Expand Up @@ -211,7 +219,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request
if r.Method != "HEAD" {
// construct the correct back link
// https://github.com/ipfs/go-ipfs/issues/1365
var backLink string = urlPath
var backLink string = prefix + urlPath

// don't go further up than /ipfs/$hash/
pathSplit := strings.Split(backLink, "/")
Expand All @@ -233,7 +241,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request

// strip /ipfs/$hash from backlink if IPNSHostnameOption touched the path.
if ipnsHostname {
backLink = "/"
backLink = prefix + "/"
if len(pathSplit) > 5 {
// also strip the trailing segment, because it's a backlink
backLinkParts := pathSplit[3 : len(pathSplit)-2]
Expand Down
59 changes: 57 additions & 2 deletions core/corehttp/gateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,30 @@ func TestIPNSHostnameRedirect(t *testing.T) {
} else if hdr[0] != "/foo/" {
t.Errorf("location header is %v, expected /foo/", hdr[0])
}

// make request with prefix to directory containing index.html
req, err = http.NewRequest("GET", ts.URL+"/foo", nil)
if err != nil {
t.Fatal(err)
}
req.Host = "example.net"
req.Header.Set("X-Ipfs-Gateway-Prefix", "/prefix")

res, err = doWithoutRedirect(req)
if err != nil {
t.Fatal(err)
}

// expect 302 redirect to same path, but with prefix and trailing slash
if res.StatusCode != 302 {
t.Errorf("status is %d, expected 302", res.StatusCode)
}
hdr = res.Header["Location"]
if len(hdr) < 1 {
t.Errorf("location header not present")
} else if hdr[0] != "/prefix/foo/" {
t.Errorf("location header is %v, expected /prefix/foo/", hdr[0])
}
}

func TestIPNSHostnameBacklinks(t *testing.T) {
Expand Down Expand Up @@ -282,7 +306,7 @@ func TestIPNSHostnameBacklinks(t *testing.T) {
t.Fatalf("expected file in directory listing")
}

// make request to directory listing
// make request to directory listing at root
req, err = http.NewRequest("GET", ts.URL, nil)
if err != nil {
t.Fatal(err)
Expand All @@ -294,7 +318,7 @@ func TestIPNSHostnameBacklinks(t *testing.T) {
t.Fatal(err)
}

// expect correct backlinks
// expect correct backlinks at root
body, err = ioutil.ReadAll(res.Body)
if err != nil {
t.Fatalf("error reading response: %s", err)
Expand Down Expand Up @@ -341,4 +365,35 @@ func TestIPNSHostnameBacklinks(t *testing.T) {
if !strings.Contains(s, "<a href=\"/foo/bar/file.txt\">") {
t.Fatalf("expected file in directory listing")
}

// make request to directory listing with prefix
req, err = http.NewRequest("GET", ts.URL, nil)
if err != nil {
t.Fatal(err)
}
req.Host = "example.net"
req.Header.Set("X-Ipfs-Gateway-Prefix", "/prefix")

res, err = doWithoutRedirect(req)
if err != nil {
t.Fatal(err)
}

// expect correct backlinks with prefix
body, err = ioutil.ReadAll(res.Body)
if err != nil {
t.Fatalf("error reading response: %s", err)
}
s = string(body)
t.Logf("body: %s\n", string(body))

if !strings.Contains(s, "Index of /prefix") {
t.Fatalf("expected a path in directory listing")
}
if !strings.Contains(s, "<a href=\"/prefix/\">") {
t.Fatalf("expected backlink in directory listing")
}
if !strings.Contains(s, "<a href=\"/prefix/file.txt\">") {
t.Fatalf("expected file in directory listing")
}
}
2 changes: 1 addition & 1 deletion core/corehttp/ipns_hostname.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func IPNSHostnameOption() ServeOption {
if len(host) > 0 && isd.IsDomain(host) {
name := "/ipns/" + host
if _, err := n.Namesys.Resolve(ctx, name); err == nil {
r.Header["X-IPNS-Original-Path"] = []string{r.URL.Path}
r.Header["X-Ipns-Original-Path"] = []string{r.URL.Path}
r.URL.Path = name + r.URL.Path
}
}
Expand Down