From 15e3732afd29b380f6a12b06303dce49abe46f82 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Sat, 20 Feb 2021 00:09:17 +0100 Subject: [PATCH 1/6] =?UTF-8?q?feat(gw):=20/ipfs/ipfs/{cid}=20=E2=86=92=20?= =?UTF-8?q?/ipfs/{cid}?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will try to recover from invalid paths like /ipfs/ipfs/{cid} and redirect to proper one, when possible. --- core/corehttp/gateway_handler.go | 11 +++++++++++ test/sharness/t0110-gateway.sh | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index e47198174..cf5acf209 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -217,6 +217,17 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request parsedPath := ipath.New(urlPath) if err := parsedPath.IsValid(); err != nil { + // Attempt to fix redundant /ipfs/ namespace as long resulting + // 'intended' path is valid. This is in case gremlins were tickled + // wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} + // like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq + // :^)) + intendedPath := ipath.New(strings.TrimPrefix(urlPath, "/ipfs")) + if err2 := intendedPath.IsValid(); err2 == nil { + intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) + http.Redirect(w, r, intendedURL, http.StatusMovedPermanently) + return + } webError(w, "invalid ipfs path", err, http.StatusBadRequest) return } diff --git a/test/sharness/t0110-gateway.sh b/test/sharness/t0110-gateway.sh index cb0b0cd67..45feca45e 100755 --- a/test/sharness/t0110-gateway.sh +++ b/test/sharness/t0110-gateway.sh @@ -84,6 +84,11 @@ test_expect_success "GET IPFS nonexistent file returns code expected (404)" ' test_curl_resp_http_code "http://127.0.0.1:$port/ipfs/$HASH2/pleaseDontAddMe" "HTTP/1.1 404 Not Found" ' +test_expect_success "GET /ipfs/ipfs/{cid} returns redirect to the valid path" " + curl -sI -o response_with_double_ipfs_ns \"http://127.0.0.1:$port/ipfs/ipfs/bafkqaaa?query=to-remember\" && + test_should_contain \"Location: /ipfs/bafkqaaa?query=to-remember\" response_with_double_ipfs_ns +" + test_expect_failure "GET IPNS path succeeds" ' ipfs name publish --allow-offline "$HASH" && PEERID=$(ipfs config Identity.PeerID) && @@ -95,6 +100,12 @@ test_expect_failure "GET IPNS path output looks good" ' test_cmp expected actual ' +test_expect_success "GET /ipfs/ipns/{peerid} returns redirect to the valid path" ' + PEERID=$(ipfs config Identity.PeerID) && + curl -sI -o response_with_ipfs_ipns_ns "http://127.0.0.1:$port/ipfs/ipns/${PEERID}?query=to-remember" && + test_should_contain "Location: /ipns/${PEERID}?query=to-remember" response_with_ipfs_ipns_ns +' + test_expect_success "GET invalid IPFS path errors" ' test_must_fail curl -sf "http://127.0.0.1:$port/ipfs/12345" ' From dae7387584ed15beebac70e32b878113cdb904b6 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Sat, 27 Feb 2021 00:48:12 +0100 Subject: [PATCH 2/6] refactor: show error, delay redirect This implements error page that does not hide the problem, but still redirects to a valid path after short delay: https://github.com/ipfs/go-ipfs/pull/7930#issuecomment-786882748 --- core/corehttp/gateway_handler.go | 34 +++++++++++++++++++++++++++----- test/sharness/t0110-gateway.sh | 14 +++++++------ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index cf5acf209..289fdd725 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -3,6 +3,7 @@ package corehttp import ( "context" "fmt" + "html/template" "io" "mime" "net/http" @@ -36,6 +37,16 @@ const ( var onlyAscii = regexp.MustCompile("[[:^ascii:]]") +// HTML-based redirect for errors which can be recovered from, but we want +// to provide hint to people that they should fix things on their end. +var redirectTemplate = template.Must(template.New("redirect").Parse(`
{{.ErrorMsg}}
(if a redirect does not happen in 10 seconds, use "{{.SuggestedPath}}" instead)
`)) + +type redirectTemplateData struct { + RedirectURL string + SuggestedPath string + ErrorMsg string +} + // gatewayHandler is a HTTP handler that serves IPFS objects (accessible by default at /ipfs/) // (it serves requests like GET /ipfs/QmVRzPKPzNtSrEzBFm2UZfxmPAgnaLke4DMcerbsGGSaFe/link) type gatewayHandler struct { @@ -216,19 +227,32 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request } parsedPath := ipath.New(urlPath) - if err := parsedPath.IsValid(); err != nil { - // Attempt to fix redundant /ipfs/ namespace as long resulting + if pathErr := parsedPath.IsValid(); pathErr != nil { + // Attempt to fix redundant /ipfs/ namespace as long as resulting // 'intended' path is valid. This is in case gremlins were tickled // wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} // like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq // :^)) intendedPath := ipath.New(strings.TrimPrefix(urlPath, "/ipfs")) - if err2 := intendedPath.IsValid(); err2 == nil { + if err := intendedPath.IsValid(); err == nil { intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) - http.Redirect(w, r, intendedURL, http.StatusMovedPermanently) + // return HTML that + // - points at correct canonical path via header + // - displays error + // - redirects to intendedURL after a delay + err = redirectTemplate.Execute(w, redirectTemplateData{ + RedirectURL: intendedURL, + SuggestedPath: intendedPath.String(), + ErrorMsg: pathErr.Error(), + }) + if err != nil { + internalWebError(w, err) + return + } return } - webError(w, "invalid ipfs path", err, http.StatusBadRequest) + // unable to fix path, returning error + webError(w, "invalid ipfs path", pathErr, http.StatusBadRequest) return } diff --git a/test/sharness/t0110-gateway.sh b/test/sharness/t0110-gateway.sh index 45feca45e..84bc96341 100755 --- a/test/sharness/t0110-gateway.sh +++ b/test/sharness/t0110-gateway.sh @@ -84,10 +84,11 @@ test_expect_success "GET IPFS nonexistent file returns code expected (404)" ' test_curl_resp_http_code "http://127.0.0.1:$port/ipfs/$HASH2/pleaseDontAddMe" "HTTP/1.1 404 Not Found" ' -test_expect_success "GET /ipfs/ipfs/{cid} returns redirect to the valid path" " - curl -sI -o response_with_double_ipfs_ns \"http://127.0.0.1:$port/ipfs/ipfs/bafkqaaa?query=to-remember\" && - test_should_contain \"Location: /ipfs/bafkqaaa?query=to-remember\" response_with_double_ipfs_ns -" +test_expect_success "GET /ipfs/ipfs/{cid} returns redirect to the valid path" ' + curl -sD - "http://127.0.0.1:$port/ipfs/ipfs/bafkqaaa?query=to-remember" > response_with_double_ipfs_ns && + test_should_contain "" response_with_double_ipfs_ns && + test_should_contain "" response_with_double_ipfs_ns +' test_expect_failure "GET IPNS path succeeds" ' ipfs name publish --allow-offline "$HASH" && @@ -102,8 +103,9 @@ test_expect_failure "GET IPNS path output looks good" ' test_expect_success "GET /ipfs/ipns/{peerid} returns redirect to the valid path" ' PEERID=$(ipfs config Identity.PeerID) && - curl -sI -o response_with_ipfs_ipns_ns "http://127.0.0.1:$port/ipfs/ipns/${PEERID}?query=to-remember" && - test_should_contain "Location: /ipns/${PEERID}?query=to-remember" response_with_ipfs_ipns_ns + curl -sD - "http://127.0.0.1:$port/ipfs/ipns/${PEERID}?query=to-remember" > response_with_ipfs_ipns_ns && + test_should_contain "" response_with_ipfs_ipns_ns && + test_should_contain "" response_with_ipfs_ipns_ns ' test_expect_success "GET invalid IPFS path errors" ' From b81b7549d34e0b549c0035286bdd02b2be50030c Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 18 Mar 2021 21:15:07 +0100 Subject: [PATCH 3/6] refactor: addressing review - moved to separate utility function - return Bad Request error - improved escaping of values passed via URL path --- core/corehttp/gateway_handler.go | 60 ++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 289fdd725..2d6b5f037 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -39,7 +39,17 @@ var onlyAscii = regexp.MustCompile("[[:^ascii:]]") // HTML-based redirect for errors which can be recovered from, but we want // to provide hint to people that they should fix things on their end. -var redirectTemplate = template.Must(template.New("redirect").Parse(`
{{.ErrorMsg}}
(if a redirect does not happen in 10 seconds, use "{{.SuggestedPath}}" instead)
`)) +var redirectTemplate = template.Must(template.New("redirect").Parse(` + + + + + + + + 
{{.ErrorMsg}}
(if a redirect does not happen in 10 seconds, use "{{.SuggestedPath}}" instead)
+ +`)) type redirectTemplateData struct { RedirectURL string @@ -228,27 +238,9 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request parsedPath := ipath.New(urlPath) if pathErr := parsedPath.IsValid(); pathErr != nil { - // Attempt to fix redundant /ipfs/ namespace as long as resulting - // 'intended' path is valid. This is in case gremlins were tickled - // wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} - // like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq - // :^)) - intendedPath := ipath.New(strings.TrimPrefix(urlPath, "/ipfs")) - if err := intendedPath.IsValid(); err == nil { - intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) - // return HTML that - // - points at correct canonical path via header - // - displays error - // - redirects to intendedURL after a delay - err = redirectTemplate.Execute(w, redirectTemplateData{ - RedirectURL: intendedURL, - SuggestedPath: intendedPath.String(), - ErrorMsg: pathErr.Error(), - }) - if err != nil { - internalWebError(w, err) - return - } + if fixErr := fixupSuperfluousNamespace(w, r, pathErr, urlPath); fixErr == nil { + // the error was due to redundant namespace, which we were able to fix + // by returning error/redirect page, nothing left to do here return } // unable to fix path, returning error @@ -816,3 +808,27 @@ func preferred404Filename(acceptHeaders []string) (string, string, error) { return "", "", fmt.Errorf("there is no 404 file for the requested content types") } + +// Attempt to fix redundant /ipfs/ namespace as long as resulting +// 'intended' path is valid. This is in case gremlins were tickled +// wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} +// like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq :^)) +func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, pathErr error, urlPath string) error { + intendedPath := ipath.New(strings.TrimPrefix(urlPath, "/ipfs")) + err := intendedPath.IsValid() + if err != nil { + // not a superfluous namespace + return err + } + intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) + // return HTTP 400 (Bad Request) with HTML error page that: + // - points at correct canonical path via header + // - displays human-readable error + // - redirects to intendedURL after a short delay + w.WriteHeader(http.StatusBadRequest) + return redirectTemplate.Execute(w, redirectTemplateData{ + RedirectURL: intendedURL, + SuggestedPath: intendedPath.String(), + ErrorMsg: fmt.Sprintf("invalid path: %q should be %q", urlPath, intendedPath.String()), + }) +} From 450baef0e9c113e4751b559dd0562f7c98302668 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 18 Mar 2021 21:48:04 +0100 Subject: [PATCH 4/6] refactor: explicit prefix check https://github.com/ipfs/go-ipfs/pull/7930#discussion_r584001161 --- core/corehttp/gateway_handler.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 2d6b5f037..07f08656c 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -238,7 +238,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request parsedPath := ipath.New(urlPath) if pathErr := parsedPath.IsValid(); pathErr != nil { - if fixErr := fixupSuperfluousNamespace(w, r, pathErr, urlPath); fixErr == nil { + if fixupSuperfluousNamespace(w, r, urlPath) { // the error was due to redundant namespace, which we were able to fix // by returning error/redirect page, nothing left to do here return @@ -813,12 +813,13 @@ func preferred404Filename(acceptHeaders []string) (string, string, error) { // 'intended' path is valid. This is in case gremlins were tickled // wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} // like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq :^)) -func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, pathErr error, urlPath string) error { +func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, urlPath string) bool { + if !(strings.HasPrefix(urlPath, "/ipfs/ipfs/") || strings.HasPrefix(urlPath, "/ipfs/ipns/")) { + return false // not a superfluous namespace + } intendedPath := ipath.New(strings.TrimPrefix(urlPath, "/ipfs")) - err := intendedPath.IsValid() - if err != nil { - // not a superfluous namespace - return err + if err := intendedPath.IsValid(); err != nil { + return false // not a valid path } intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) // return HTTP 400 (Bad Request) with HTML error page that: @@ -830,5 +831,5 @@ func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, pathErr e RedirectURL: intendedURL, SuggestedPath: intendedPath.String(), ErrorMsg: fmt.Sprintf("invalid path: %q should be %q", urlPath, intendedPath.String()), - }) + }) == nil } From a35ffee1364b72e50f66fdd1a03f7a3665d99309 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 18 Mar 2021 23:48:53 +0100 Subject: [PATCH 5/6] refactor: safer query handling https://github.com/ipfs/go-ipfs/pull/7930#discussion_r597246135 --- core/corehttp/gateway_handler.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 07f08656c..4d0a34c23 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -238,7 +238,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request parsedPath := ipath.New(urlPath) if pathErr := parsedPath.IsValid(); pathErr != nil { - if fixupSuperfluousNamespace(w, r, urlPath) { + if fixupSuperfluousNamespace(w, urlPath, r.URL.RawQuery) { // the error was due to redundant namespace, which we were able to fix // by returning error/redirect page, nothing left to do here return @@ -813,7 +813,7 @@ func preferred404Filename(acceptHeaders []string) (string, string, error) { // 'intended' path is valid. This is in case gremlins were tickled // wrong way and user ended up at /ipfs/ipfs/{cid} or /ipfs/ipns/{id} // like in bafybeien3m7mdn6imm425vc2s22erzyhbvk5n3ofzgikkhmdkh5cuqbpbq :^)) -func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, urlPath string) bool { +func fixupSuperfluousNamespace(w http.ResponseWriter, urlPath string, urlQuery string) bool { if !(strings.HasPrefix(urlPath, "/ipfs/ipfs/") || strings.HasPrefix(urlPath, "/ipfs/ipns/")) { return false // not a superfluous namespace } @@ -821,7 +821,12 @@ func fixupSuperfluousNamespace(w http.ResponseWriter, r *http.Request, urlPath s if err := intendedPath.IsValid(); err != nil { return false // not a valid path } - intendedURL := strings.Replace(r.URL.String(), urlPath, intendedPath.String(), 1) + intendedURL := intendedPath.String() + if urlQuery != "" { + // we render HTML, so ensure query entries are properly escaped + q, _ := url.ParseQuery(urlQuery) + intendedURL = intendedURL + "?" + q.Encode() + } // return HTTP 400 (Bad Request) with HTML error page that: // - points at correct canonical path via header // - displays human-readable error From 763a120ed28c6cd2e77077c6bb0281c583a6b580 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Sat, 20 Mar 2021 00:00:58 +0100 Subject: [PATCH 6/6] fix: no fixup if X-Ipfs-Gateway-Prefix is present https://github.com/ipfs/go-ipfs/pull/7930#discussion_r597976690 --- core/corehttp/gateway_handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 4d0a34c23..0275c873d 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -238,7 +238,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request parsedPath := ipath.New(urlPath) if pathErr := parsedPath.IsValid(); pathErr != nil { - if fixupSuperfluousNamespace(w, urlPath, r.URL.RawQuery) { + if prefix == "" && fixupSuperfluousNamespace(w, urlPath, r.URL.RawQuery) { // the error was due to redundant namespace, which we were able to fix // by returning error/redirect page, nothing left to do here return