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

fix(gw): 404 when a valid DAG is missing link #9126

Merged
merged 1 commit into from
Jul 22, 2022
Merged

fix(gw): 404 when a valid DAG is missing link #9126

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
6 changes: 4 additions & 2 deletions core/corehttp/gateway_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

cid "github.com/ipfs/go-cid"
files "github.com/ipfs/go-ipfs-files"
ipld "github.com/ipfs/go-ipld-format"
dag "github.com/ipfs/go-merkledag"
mfs "github.com/ipfs/go-mfs"
path "github.com/ipfs/go-path"
Expand Down Expand Up @@ -389,8 +390,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request
logger.Debugw("serve pretty 404 if present")
return
}

webError(w, "ipfs resolve -r "+debugStr(contentPath.String()), err, http.StatusNotFound)
webError(w, "ipfs resolve -r "+debugStr(contentPath.String()), err, http.StatusBadRequest)
return
}

Expand Down Expand Up @@ -782,6 +782,8 @@ func webError(w http.ResponseWriter, message string, err error, defaultCode int)
webErrorWithCode(w, message, err, http.StatusNotFound)
} else if err == routing.ErrNotFound {
webErrorWithCode(w, message, err, http.StatusNotFound)
} else if ipld.IsNotFound(err) {
webErrorWithCode(w, message, err, http.StatusNotFound)
Comment on lines +785 to +786
Copy link
Member Author

@lidel lidel Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ℹ️ this makes sure every webError gets 404 when appropriate (even when it was called with http.StatusBadRequest)

} else if err == context.DeadlineExceeded {
webErrorWithCode(w, message, err, http.StatusRequestTimeout)
} else {
Expand Down
2 changes: 1 addition & 1 deletion core/corehttp/gateway_handler_unixfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (i *gatewayHandler) serveUnixFS(ctx context.Context, w http.ResponseWriter,
// Handling UnixFS
dr, err := i.api.Unixfs().Get(ctx, resolvedPath)
if err != nil {
webError(w, "ipfs cat "+html.EscapeString(contentPath.String()), err, http.StatusNotFound)
webError(w, "ipfs cat "+html.EscapeString(contentPath.String()), err, http.StatusBadRequest)
return
}
defer dr.Close()
Expand Down
8 changes: 4 additions & 4 deletions core/corehttp/gateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,17 +235,17 @@ func TestGatewayGet(t *testing.T) {
{"127.0.0.1:8080", "/", http.StatusNotFound, "404 page not found\n"},
{"127.0.0.1:8080", "/" + k.Cid().String(), http.StatusNotFound, "404 page not found\n"},
{"127.0.0.1:8080", k.String(), http.StatusOK, "fnord"},
{"127.0.0.1:8080", "/ipns/nxdomain.example.com", http.StatusNotFound, "ipfs resolve -r /ipns/nxdomain.example.com: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/%0D%0A%0D%0Ahello", http.StatusNotFound, "ipfs resolve -r /ipns/\\r\\n\\r\\nhello: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/nxdomain.example.com", http.StatusBadRequest, "ipfs resolve -r /ipns/nxdomain.example.com: " + namesys.ErrResolveFailed.Error() + "\n"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Note I think resolution errors should be reported as 502 Bad Gateway but that's not what the gateway spec says. I'll raise an issue there instead

Copy link
Member Author

@lidel lidel Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Continued in ipfs/specs#300 👍

{"127.0.0.1:8080", "/ipns/%0D%0A%0D%0Ahello", http.StatusBadRequest, "ipfs resolve -r /ipns/\\r\\n\\r\\nhello: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/example.com", http.StatusOK, "fnord"},
{"example.com", "/", http.StatusOK, "fnord"},

{"working.example.com", "/", http.StatusOK, "fnord"},
{"double.example.com", "/", http.StatusOK, "fnord"},
{"triple.example.com", "/", http.StatusOK, "fnord"},
{"working.example.com", k.String(), http.StatusNotFound, "ipfs resolve -r /ipns/working.example.com" + k.String() + ": no link named \"ipfs\" under " + k.Cid().String() + "\n"},
{"broken.example.com", "/", http.StatusNotFound, "ipfs resolve -r /ipns/broken.example.com/: " + namesys.ErrResolveFailed.Error() + "\n"},
{"broken.example.com", k.String(), http.StatusNotFound, "ipfs resolve -r /ipns/broken.example.com" + k.String() + ": " + namesys.ErrResolveFailed.Error() + "\n"},
{"broken.example.com", "/", http.StatusBadRequest, "ipfs resolve -r /ipns/broken.example.com/: " + namesys.ErrResolveFailed.Error() + "\n"},
{"broken.example.com", k.String(), http.StatusBadRequest, "ipfs resolve -r /ipns/broken.example.com" + k.String() + ": " + namesys.ErrResolveFailed.Error() + "\n"},
// This test case ensures we don't treat the TLD as a file extension.
{"example.man", "/", http.StatusOK, "fnord"},
} {
Expand Down
10 changes: 9 additions & 1 deletion test/sharness/t0110-gateway.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,14 @@ test_expect_success "GET IPFS directory with index.html and trailing slash retur
test_should_contain \"hello i am a webpage\" response_with_slash
"

test_expect_success "GET IPFS nonexistent file returns code expected (404)" '
test_expect_success "GET IPFS nonexistent file returns 404 (Not Found)" '
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 invalid CID returns 400 (Bad Request)" '
test_curl_resp_http_code "http://127.0.0.1:$port/ipfs/QmInvalid/pleaseDontAddMe" "HTTP/1.1 400 Bad Request"
'

# https://github.com/ipfs/go-ipfs/issues/8230
test_expect_success "GET IPFS inlined zero-length data object returns ok code (200)" '
curl -sD - "http://127.0.0.1:$port/ipfs/bafkqaaa" > empty_ok_response &&
Expand All @@ -105,6 +109,10 @@ test_expect_success "GET /ipfs/ipfs/{cid} returns redirect to the valid path" '
test_should_contain "<link rel=\"canonical\" href=\"/ipfs/bafkqaaa?query=to-remember\" />" response_with_double_ipfs_ns
'

test_expect_success "GET invalid IPNS root returns 400 (Bad Request)" '
test_curl_resp_http_code "http://127.0.0.1:$port/ipns/QmInvalid/pleaseDontAddMe" "HTTP/1.1 400 Bad Request"
'

test_expect_failure "GET IPNS path succeeds" '
ipfs name publish --allow-offline "$HASH" &&
PEERID=$(ipfs config Identity.PeerID) &&
Expand Down