From c1b8ceaf0d70a62691222769a24deb139ca7ef8f Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Thu, 6 Oct 2022 11:44:32 +0200 Subject: [PATCH 1/8] fix: add Trailer header to CAR responses --- core/corehttp/gateway_handler_car.go | 1 + 1 file changed, 1 insertion(+) diff --git a/core/corehttp/gateway_handler_car.go b/core/corehttp/gateway_handler_car.go index 3363c5199e2..828416b4cda 100644 --- a/core/corehttp/gateway_handler_car.go +++ b/core/corehttp/gateway_handler_car.go @@ -67,6 +67,7 @@ func (i *gatewayHandler) serveCAR(ctx context.Context, w http.ResponseWriter, r w.Header().Set("Content-Type", "application/vnd.ipld.car; version=1") w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^) + w.Header().Set("Trailer", "X-Stream-Error") // Same go-car settings as dag.export command store := dagStore{dag: i.api.Dag(), ctx: ctx} From 9d40055a5266adbb070c1228b7ab8b0a1631599c Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Tue, 11 Oct 2022 11:05:10 +0200 Subject: [PATCH 2/8] refactor: force close connection with error --- core/corehttp/gateway_handler.go | 27 +++++++++++++++++++++++++++ core/corehttp/gateway_handler_car.go | 7 +------ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 1222b17bcd7..dc7ff09de20 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -1104,3 +1104,30 @@ func (i *gatewayHandler) setCommonHeaders(w http.ResponseWriter, r *http.Request return nil } + +// closeConnWithError forcefully closes an HTTP/1.x connection, leading to a network +// error on the user side. If it is not possible to forcefully close the connection, +// we call webError which prints the error to the client, leading to a corrupt file. +// This is a way of showing the client that there was an error while streaming the contents. +// Currently, there are no great ways of telling the client that an error occurred while +// streaming in HTTP. +func closeConnWithError(w http.ResponseWriter, err error) { + // There are no good ways of showing an error during a stream. Therefore, we try + // to hijack the connection to forcefully close it, causing a network error. + hj, ok := w.(http.Hijacker) + if !ok { + // If we could not Hijack the connection, we write the original error. This will hopefully + // corrupt the generated TAR file, such that the client will receive an error unpacking. + webError(w, "could not build tar archive", err, http.StatusInternalServerError) + return + } + + conn, _, hijackErr := hj.Hijack() + if hijackErr != nil { + // Deliberately pass the original tar error here instead of the hijacking error. + webError(w, "could not build tar archive", err, http.StatusInternalServerError) + return + } + + conn.Close() +} diff --git a/core/corehttp/gateway_handler_car.go b/core/corehttp/gateway_handler_car.go index 828416b4cda..fde991b2348 100644 --- a/core/corehttp/gateway_handler_car.go +++ b/core/corehttp/gateway_handler_car.go @@ -67,7 +67,6 @@ func (i *gatewayHandler) serveCAR(ctx context.Context, w http.ResponseWriter, r w.Header().Set("Content-Type", "application/vnd.ipld.car; version=1") w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^) - w.Header().Set("Trailer", "X-Stream-Error") // Same go-car settings as dag.export command store := dagStore{dag: i.api.Dag(), ctx: ctx} @@ -77,11 +76,7 @@ func (i *gatewayHandler) serveCAR(ctx context.Context, w http.ResponseWriter, r car := gocar.NewSelectiveCar(ctx, store, []gocar.Dag{dag}, gocar.TraverseLinksOnlyOnce()) if err := car.Write(w); err != nil { - // We return error as a trailer, however it is not something browsers can access - // (https://github.com/mdn/browser-compat-data/issues/14703) - // Due to this, we suggest client always verify that - // the received CAR stream response is matching requested DAG selector - w.Header().Set("X-Stream-Error", err.Error()) + closeConnWithError(w, err) return } From a37a0ccaf5fbe52656dadb4abd337c8d941a2beb Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Tue, 11 Oct 2022 11:11:57 +0200 Subject: [PATCH 3/8] refactor: force-close when streaming all --- core/corehttp/gateway_handler_block.go | 6 +++++- core/corehttp/gateway_handler_unixfs_dir.go | 2 +- core/corehttp/gateway_handler_unixfs_file.go | 6 +++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/core/corehttp/gateway_handler_block.go b/core/corehttp/gateway_handler_block.go index 3bf7c76be2d..df47e720044 100644 --- a/core/corehttp/gateway_handler_block.go +++ b/core/corehttp/gateway_handler_block.go @@ -46,7 +46,11 @@ func (i *gatewayHandler) serveRawBlock(ctx context.Context, w http.ResponseWrite // ServeContent will take care of // If-None-Match+Etag, Content-Length and range requests - _, dataSent, _ := ServeContent(w, r, name, modtime, content) + _, dataSent, err := ServeContent(w, r, name, modtime, content) + + if err != nil { + closeConnWithError(w, err) + } if dataSent { // Update metrics diff --git a/core/corehttp/gateway_handler_unixfs_dir.go b/core/corehttp/gateway_handler_unixfs_dir.go index 1c803b13b34..424d9f7e4dd 100644 --- a/core/corehttp/gateway_handler_unixfs_dir.go +++ b/core/corehttp/gateway_handler_unixfs_dir.go @@ -209,7 +209,7 @@ func (i *gatewayHandler) serveDirectory(ctx context.Context, w http.ResponseWrit logger.Debugw("request processed", "tplDataDNSLink", dnslink, "tplDataSize", size, "tplDataBackLink", backLink, "tplDataHash", hash) if err := listingTemplate.Execute(w, tplData); err != nil { - internalWebError(w, err) + closeConnWithError(w, err) return } diff --git a/core/corehttp/gateway_handler_unixfs_file.go b/core/corehttp/gateway_handler_unixfs_file.go index 9463be1acf8..7d64c41def3 100644 --- a/core/corehttp/gateway_handler_unixfs_file.go +++ b/core/corehttp/gateway_handler_unixfs_file.go @@ -94,7 +94,11 @@ func (i *gatewayHandler) serveFile(ctx context.Context, w http.ResponseWriter, r // ServeContent will take care of // If-None-Match+Etag, Content-Length and range requests - _, dataSent, _ := ServeContent(w, r, name, modtime, content) + _, dataSent, err := ServeContent(w, r, name, modtime, content) + + if err != nil { + closeConnWithError(w, err) + } // Was response successful? if dataSent { From 15261b94ae0a333d38256c5f5a207c42a0598259 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 12 Oct 2022 16:14:49 +0200 Subject: [PATCH 4/8] refactor: use http.ErrAbortHandler if hijack fails --- core/corehttp/gateway_handler.go | 29 ++++++++------------ core/corehttp/gateway_handler_block.go | 3 +- core/corehttp/gateway_handler_car.go | 2 +- core/corehttp/gateway_handler_unixfs_dir.go | 2 +- core/corehttp/gateway_handler_unixfs_file.go | 3 +- 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index dc7ff09de20..9b59d3411c5 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -1105,28 +1105,21 @@ func (i *gatewayHandler) setCommonHeaders(w http.ResponseWriter, r *http.Request return nil } -// closeConnWithError forcefully closes an HTTP/1.x connection, leading to a network -// error on the user side. If it is not possible to forcefully close the connection, -// we call webError which prints the error to the client, leading to a corrupt file. -// This is a way of showing the client that there was an error while streaming the contents. -// Currently, there are no great ways of telling the client that an error occurred while -// streaming in HTTP. -func closeConnWithError(w http.ResponseWriter, err error) { - // There are no good ways of showing an error during a stream. Therefore, we try - // to hijack the connection to forcefully close it, causing a network error. +// abortConn forcefully closes an HTTP connection, leading to a network error on the +// client side. This is a way of showing the client that there was an error while streaming +// the contents since there are no good ways of showing an error during a stream. +func abortConn(w http.ResponseWriter) { hj, ok := w.(http.Hijacker) if !ok { - // If we could not Hijack the connection, we write the original error. This will hopefully - // corrupt the generated TAR file, such that the client will receive an error unpacking. - webError(w, "could not build tar archive", err, http.StatusInternalServerError) - return + // If we could not Hijack the connection (such as in HTTP/2.x) connections, we + // panic using http.ErrAbortHandler, which aborts the response. + // https://pkg.go.dev/net/http#ErrAbortHandler + panic(http.ErrAbortHandler) } - conn, _, hijackErr := hj.Hijack() - if hijackErr != nil { - // Deliberately pass the original tar error here instead of the hijacking error. - webError(w, "could not build tar archive", err, http.StatusInternalServerError) - return + conn, _, err := hj.Hijack() + if err != nil { + panic(http.ErrAbortHandler) } conn.Close() diff --git a/core/corehttp/gateway_handler_block.go b/core/corehttp/gateway_handler_block.go index df47e720044..2ac714d69ad 100644 --- a/core/corehttp/gateway_handler_block.go +++ b/core/corehttp/gateway_handler_block.go @@ -49,7 +49,8 @@ func (i *gatewayHandler) serveRawBlock(ctx context.Context, w http.ResponseWrite _, dataSent, err := ServeContent(w, r, name, modtime, content) if err != nil { - closeConnWithError(w, err) + abortConn(w) + return } if dataSent { diff --git a/core/corehttp/gateway_handler_car.go b/core/corehttp/gateway_handler_car.go index fde991b2348..2c03b104624 100644 --- a/core/corehttp/gateway_handler_car.go +++ b/core/corehttp/gateway_handler_car.go @@ -76,7 +76,7 @@ func (i *gatewayHandler) serveCAR(ctx context.Context, w http.ResponseWriter, r car := gocar.NewSelectiveCar(ctx, store, []gocar.Dag{dag}, gocar.TraverseLinksOnlyOnce()) if err := car.Write(w); err != nil { - closeConnWithError(w, err) + abortConn(w) return } diff --git a/core/corehttp/gateway_handler_unixfs_dir.go b/core/corehttp/gateway_handler_unixfs_dir.go index 424d9f7e4dd..52c0327c326 100644 --- a/core/corehttp/gateway_handler_unixfs_dir.go +++ b/core/corehttp/gateway_handler_unixfs_dir.go @@ -209,7 +209,7 @@ func (i *gatewayHandler) serveDirectory(ctx context.Context, w http.ResponseWrit logger.Debugw("request processed", "tplDataDNSLink", dnslink, "tplDataSize", size, "tplDataBackLink", backLink, "tplDataHash", hash) if err := listingTemplate.Execute(w, tplData); err != nil { - closeConnWithError(w, err) + abortConn(w) return } diff --git a/core/corehttp/gateway_handler_unixfs_file.go b/core/corehttp/gateway_handler_unixfs_file.go index 7d64c41def3..6bb507359d5 100644 --- a/core/corehttp/gateway_handler_unixfs_file.go +++ b/core/corehttp/gateway_handler_unixfs_file.go @@ -97,7 +97,8 @@ func (i *gatewayHandler) serveFile(ctx context.Context, w http.ResponseWriter, r _, dataSent, err := ServeContent(w, r, name, modtime, content) if err != nil { - closeConnWithError(w, err) + abortConn(w) + return } // Was response successful? From 28b8c59d7d166ccff002438111072a6ee1ab2fe2 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 12 Oct 2022 16:25:45 +0200 Subject: [PATCH 5/8] Update core/corehttp/gateway_handler.go Co-authored-by: Jorropo --- core/corehttp/gateway_handler.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 9b59d3411c5..8c03d0f6689 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -1122,5 +1122,8 @@ func abortConn(w http.ResponseWriter) { panic(http.ErrAbortHandler) } - conn.Close() + err = conn.Close() + if err != nil { + panic(http.ErrAbortHandler) + } } From bf5d3133dd6f89394e3a3f46c3d3ead6be1a0a52 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 12 Oct 2022 16:29:42 +0200 Subject: [PATCH 6/8] fix lint --- 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 8c03d0f6689..b310b63f3c8 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -1124,6 +1124,6 @@ func abortConn(w http.ResponseWriter) { err = conn.Close() if err != nil { - panic(http.ErrAbortHandler) + panic(http.ErrAbortHandler) } } From 4aa8461fe6f89f2ceb3e03b701ed7cf108786ea5 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Thu, 10 Nov 2022 11:56:49 +0100 Subject: [PATCH 7/8] refactor: add abortConn in tar handler --- core/corehttp/gateway_handler_tar.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/core/corehttp/gateway_handler_tar.go b/core/corehttp/gateway_handler_tar.go index 532d8875760..884b7e8b452 100644 --- a/core/corehttp/gateway_handler_tar.go +++ b/core/corehttp/gateway_handler_tar.go @@ -79,14 +79,6 @@ func (i *gatewayHandler) serveTAR(ctx context.Context, w http.ResponseWriter, r // The TAR has a top-level directory (or file) named by the CID. if err := tarw.WriteFile(file, rootCid.String()); err != nil { - w.Header().Set("X-Stream-Error", err.Error()) - // Trailer headers do not work in web browsers - // (see https://github.com/mdn/browser-compat-data/issues/14703) - // and we have limited options around error handling in browser contexts. - // To improve UX/DX, we finish response stream with error message, allowing client to - // (1) detect error by having corrupted TAR - // (2) be able to reason what went wrong by instecting the tail of TAR stream - _, _ = w.Write([]byte(err.Error())) - return + abortConn(w) } } From f0d76b561dbc883a357f5594240d606bae2f4041 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Thu, 10 Nov 2022 12:11:09 +0100 Subject: [PATCH 8/8] fix: update tar test --- test/sharness/t0122-gateway-tar.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/sharness/t0122-gateway-tar.sh b/test/sharness/t0122-gateway-tar.sh index 34dc1ba12c8..b1f55d2d564 100755 --- a/test/sharness/t0122-gateway-tar.sh +++ b/test/sharness/t0122-gateway-tar.sh @@ -76,8 +76,7 @@ test_expect_success "Add CARs with relative paths to test with" ' ' test_expect_success "GET TAR with relative paths outside root fails" ' - curl -o - "http://127.0.0.1:$GWAY_PORT/ipfs/$OUTSIDE_ROOT_CID?format=tar" > curl_output_filename && - test_should_contain "relative UnixFS paths outside the root are now allowed" curl_output_filename + ! curl "http://127.0.0.1:$GWAY_PORT/ipfs/$OUTSIDE_ROOT_CID?format=tar" ' test_expect_success "GET TAR with relative paths inside root works" '