diff --git a/.github/workflows/gateway-conformance.yml b/.github/workflows/gateway-conformance.yml index a6c1b6c6e..60a2cfc18 100644 --- a/.github/workflows/gateway-conformance.yml +++ b/.github/workflows/gateway-conformance.yml @@ -22,7 +22,7 @@ jobs: steps: # 1. Download the gateway-conformance fixtures - name: Download gateway-conformance fixtures - uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.6 + uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.7 with: output: fixtures merged: true @@ -47,7 +47,7 @@ jobs: # 4. Run the gateway-conformance tests - name: Run gateway-conformance tests - uses: ipfs/gateway-conformance/.github/actions/test@v0.6 + uses: ipfs/gateway-conformance/.github/actions/test@v0.7 with: gateway-url: http://127.0.0.1:8040 subdomain-url: http://example.net:8040 @@ -84,7 +84,7 @@ jobs: steps: # 1. Download the gateway-conformance fixtures - name: Download gateway-conformance fixtures - uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.6 + uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.7 with: output: fixtures merged: true @@ -114,7 +114,7 @@ jobs: # 4. Run the gateway-conformance tests - name: Run gateway-conformance tests - uses: ipfs/gateway-conformance/.github/actions/test@v0.6 + uses: ipfs/gateway-conformance/.github/actions/test@v0.7 with: gateway-url: http://127.0.0.1:8040 # we test gateway that is backed by a remote block gateway subdomain-url: http://example.net:8040 @@ -152,7 +152,7 @@ jobs: steps: # 1. Download the gateway-conformance fixtures - name: Download gateway-conformance fixtures - uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.6 + uses: ipfs/gateway-conformance/.github/actions/extract-fixtures@v0.7 with: output: fixtures merged: true @@ -182,7 +182,7 @@ jobs: # 4. Run the gateway-conformance tests - name: Run gateway-conformance tests - uses: ipfs/gateway-conformance/.github/actions/test@v0.6 + uses: ipfs/gateway-conformance/.github/actions/test@v0.7 with: gateway-url: http://127.0.0.1:8040 # we test gateway that is backed by a remote car gateway subdomain-url: http://example.net:8040 diff --git a/CHANGELOG.md b/CHANGELOG.md index 199b69066..fa46c5b84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ The following emojis are used to highlight certain changes: ### Fixed +- `gateway` Fix redirect URLs for subdirectories with characters that need escaping. [#779](https://github.com/ipfs/boxo/pull/779) + ### Security diff --git a/bitswap/README.md b/bitswap/README.md index c8fd819e8..6f699daa5 100644 --- a/bitswap/README.md +++ b/bitswap/README.md @@ -23,7 +23,7 @@ contain wantlists or blocks. A node sends a wantlist to tell peers which blocks it wants. When a node receives a wantlist it should check which blocks it has from the wantlist, and consider -sending the matching blocks to the requestor. +sending the matching blocks to the requester. When a node receives blocks that it asked for, the node should send out a notification called a 'Cancel' to tell its peers that the node no longer @@ -74,7 +74,7 @@ block, err := exchange.GetBlock(ctx, c) Parameter Notes: -1. `ctx` is the context for this request, which can be cancelled to cancel the request +1. `ctx` is the context for this request, which can be canceled to cancel the request 2. `c` is the content ID of the block you're requesting ### Get Several Blocks Asynchronously @@ -89,7 +89,7 @@ blockChannel, err := exchange.GetBlocks(ctx, cids) Parameter Notes: -1. `ctx` is the context for this request, which can be cancelled to cancel the request +1. `ctx` is the context for this request, which can be canceled to cancel the request 2. `cids` is a slice of content IDs for the blocks you're requesting ### Get Related Blocks Faster With Sessions diff --git a/bitswap/client/internal/messagequeue/messagequeue.go b/bitswap/client/internal/messagequeue/messagequeue.go index e427d18c2..37a783ac7 100644 --- a/bitswap/client/internal/messagequeue/messagequeue.go +++ b/bitswap/client/internal/messagequeue/messagequeue.go @@ -93,7 +93,7 @@ type MessageQueue struct { cancels *cid.Set priority int32 - // Dont touch any of these variables outside of run loop + // Don't touch any of these variables outside of run loop sender bsnet.MessageSender rebroadcastNow chan struct{} // For performance reasons we just clear out the fields of the message @@ -160,7 +160,7 @@ func (r *recallWantlist) MarkSent(e bswl.Entry) bool { // SentAt records the time at which a want was sent func (r *recallWantlist) SentAt(c cid.Cid, at time.Time) { - // The want may have been cancelled in the interim + // The want may have been canceled in the interim if _, ok := r.sent.Contains(c); ok { if _, ok := r.sentAt[c]; !ok { r.sentAt[c] = at @@ -222,7 +222,7 @@ type DontHaveTimeoutManager interface { // Shutdown the manager (Shutdown is final, manager cannot be restarted) Shutdown() // AddPending adds the wants as pending a response. If the are not - // cancelled before the timeout, the OnDontHaveTimeout method will be called. + // canceled before the timeout, the OnDontHaveTimeout method will be called. AddPending([]cid.Cid) // CancelPending removes the wants CancelPending([]cid.Cid) @@ -597,7 +597,7 @@ func (mq *MessageQueue) sendMessage() { } } -// If want-block times out, simulate a DONT_HAVE reponse. +// If want-block times out, simulate a DONT_HAVE response. // This is necessary when making requests to peers running an older version of // Bitswap that doesn't support the DONT_HAVE response, and is also useful to // mitigate getting blocked by a peer that takes a long time to respond. @@ -847,13 +847,13 @@ FINISH: defer mq.wllock.Unlock() for _, e := range peerEntries[:sentPeerEntries] { - if e.Cid.Defined() { // Check if want was cancelled in the interim + if e.Cid.Defined() { // Check if want was canceled in the interim mq.peerWants.SentAt(e.Cid, now) } } for _, e := range bcstEntries[:sentBcstEntries] { - if e.Cid.Defined() { // Check if want was cancelled in the interim + if e.Cid.Defined() { // Check if want was canceled in the interim mq.bcstWants.SentAt(e.Cid, now) } } diff --git a/bitswap/server/internal/decision/engine.go b/bitswap/server/internal/decision/engine.go index 9b929b99c..308673696 100644 --- a/bitswap/server/internal/decision/engine.go +++ b/bitswap/server/internal/decision/engine.go @@ -916,7 +916,7 @@ func (e *Engine) handleOverflow(ctx context.Context, p peer.ID, overflow, wants return wants } -// Split the want-havek entries from the cancel and deny entries. +// Split the want, cancel, and deny entries. func (e *Engine) splitWantsCancelsDenials(p peer.ID, m bsmsg.BitSwapMessage) ([]bsmsg.Entry, []bsmsg.Entry, []bsmsg.Entry, error) { entries := m.Wantlist() // creates copy; safe to modify if len(entries) == 0 { diff --git a/gateway/gateway_test.go b/gateway/gateway_test.go index e4a9935ac..349d76a0e 100644 --- a/gateway/gateway_test.go +++ b/gateway/gateway_test.go @@ -700,13 +700,13 @@ func TestRedirects(t *testing.T) { // - Browsers will send original URI in URL-escaped form // - We expect query parameters to be persisted // - We drop fragments, as those should not be sent by a browser - {"/ipfs/?uri=ipfs%3A%2F%2FQmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html%23header-%C4%85", http.StatusMovedPermanently, "/ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Foo_%c4%85%c4%99.html?filename=test-%c4%99.html"}, - {"/ipfs/?uri=ipns%3A%2F%2Fexample.com%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html", http.StatusMovedPermanently, "/ipns/example.com/wiki/Foo_%c4%85%c4%99.html?filename=test-%c4%99.html"}, + {"/ipfs/?uri=ipfs%3A%2F%2FQmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html%23header-%C4%85", http.StatusMovedPermanently, "/ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Foo_%C4%85%C4%99.html?filename=test-%C4%99.html"}, + {"/ipfs/?uri=ipns%3A%2F%2Fexample.com%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html", http.StatusMovedPermanently, "/ipns/example.com/wiki/Foo_%C4%85%C4%99.html?filename=test-%C4%99.html"}, {"/ipfs/?uri=ipfs://" + cid, http.StatusMovedPermanently, "/ipfs/" + cid}, {"/ipfs?uri=ipfs://" + cid, http.StatusMovedPermanently, "/ipfs/" + cid}, {"/ipfs/?uri=ipns://" + cid, http.StatusMovedPermanently, "/ipns/" + cid}, - {"/ipns/?uri=ipfs%3A%2F%2FQmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html%23header-%C4%85", http.StatusMovedPermanently, "/ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Foo_%c4%85%c4%99.html?filename=test-%c4%99.html"}, - {"/ipns/?uri=ipns%3A%2F%2Fexample.com%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html", http.StatusMovedPermanently, "/ipns/example.com/wiki/Foo_%c4%85%c4%99.html?filename=test-%c4%99.html"}, + {"/ipns/?uri=ipfs%3A%2F%2FQmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html%23header-%C4%85", http.StatusMovedPermanently, "/ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Foo_%C4%85%C4%99.html?filename=test-%C4%99.html"}, + {"/ipns/?uri=ipns%3A%2F%2Fexample.com%2Fwiki%2FFoo_%C4%85%C4%99.html%3Ffilename%3Dtest-%C4%99.html", http.StatusMovedPermanently, "/ipns/example.com/wiki/Foo_%C4%85%C4%99.html?filename=test-%C4%99.html"}, {"/ipns?uri=ipns://" + cid, http.StatusMovedPermanently, "/ipns/" + cid}, {"/ipns/?uri=ipns://" + cid, http.StatusMovedPermanently, "/ipns/" + cid}, {"/ipns/?uri=ipfs://" + cid, http.StatusMovedPermanently, "/ipfs/" + cid}, diff --git a/gateway/handler.go b/gateway/handler.go index d6e1b7054..3281788fe 100644 --- a/gateway/handler.go +++ b/gateway/handler.go @@ -906,9 +906,10 @@ func handleProtocolHandlerRedirect(w http.ResponseWriter, r *http.Request, c *Co webError(w, r, c, fmt.Errorf("uri query parameter scheme must be ipfs or ipns: %w", err), http.StatusBadRequest) return true } - path := u.Path + + path := u.EscapedPath() if u.RawQuery != "" { // preserve query if present - path = path + "?" + u.RawQuery + path += "?" + url.PathEscape(u.RawQuery) } redirectURL := gopath.Join("/", u.Scheme, u.Host, path) @@ -989,7 +990,7 @@ func (i *handler) handleSuperfluousNamespace(w http.ResponseWriter, r *http.Requ } // Attempt to fix the superflous namespace - intendedPath, err := path.NewPath(strings.TrimPrefix(r.URL.Path, "/ipfs")) + intendedPath, err := path.NewPath(strings.TrimPrefix(r.URL.EscapedPath(), "/ipfs")) if err != nil { i.webError(w, r, fmt.Errorf("invalid ipfs path: %w", err), http.StatusBadRequest) return true diff --git a/gateway/handler_codec.go b/gateway/handler_codec.go index 89bff966e..b3b1fcce2 100644 --- a/gateway/handler_codec.go +++ b/gateway/handler_codec.go @@ -170,10 +170,15 @@ func (i *handler) serveCodecHTML(ctx context.Context, w http.ResponseWriter, r * suffix := "/" // preserve query parameters if r.URL.RawQuery != "" { - suffix = suffix + "?" + r.URL.RawQuery + suffix = suffix + "?" + url.PathEscape(r.URL.RawQuery) + } + // Re-escape path instead of reusing RawPath to avod mix of lawer + // and upper hex that may come from RawPath. + if strings.ContainsRune(requestURI.RawPath, '%') { + requestURI.RawPath = "" } // /ipfs/cid/foo?bar must be redirected to /ipfs/cid/foo/?bar - redirectURL := requestURI.Path + suffix + redirectURL := requestURI.EscapedPath() + suffix http.Redirect(w, r, redirectURL, http.StatusMovedPermanently) return true } diff --git a/gateway/handler_unixfs_dir.go b/gateway/handler_unixfs_dir.go index 6f9f856d1..39d7fe4a7 100644 --- a/gateway/handler_unixfs_dir.go +++ b/gateway/handler_unixfs_dir.go @@ -47,11 +47,17 @@ func (i *handler) serveDirectory(ctx context.Context, w http.ResponseWriter, r * suffix := "/" // preserve query parameters if r.URL.RawQuery != "" { - suffix = suffix + "?" + r.URL.RawQuery + suffix = suffix + "?" + url.PathEscape(r.URL.RawQuery) + } + // Re-escape path instead of reusing RawPath to avod mix of lawer + // and upper hex that may come from RawPath. + if strings.ContainsRune(requestURI.RawPath, '%') { + requestURI.RawPath = "" } // /ipfs/cid/foo?bar must be redirected to /ipfs/cid/foo/?bar - redirectURL := originalURLPath + suffix + redirectURL := requestURI.EscapedPath() + suffix rq.logger.Debugw("directory location moved permanently", "status", http.StatusMovedPermanently) + http.Redirect(w, r, redirectURL, http.StatusMovedPermanently) return true } diff --git a/namesys/interface.go b/namesys/interface.go index 1befee855..cef8d8c05 100644 --- a/namesys/interface.go +++ b/namesys/interface.go @@ -207,7 +207,7 @@ func PublishWithEOL(eol time.Time) PublishOption { } } -// PublishWithEOL sets [PublishOptions.TTL]. +// PublishWithTTL sets [PublishOptions.TTL]. func PublishWithTTL(ttl time.Duration) PublishOption { return func(o *PublishOptions) { o.TTL = ttl diff --git a/pinning/pinner/dspinner/pin_test.go b/pinning/pinner/dspinner/pin_test.go index cbc1e6c34..153a3bc16 100644 --- a/pinning/pinner/dspinner/pin_test.go +++ b/pinning/pinner/dspinner/pin_test.go @@ -367,7 +367,7 @@ func TestAddLoadPin(t *testing.T) { t.Fatal(err) } if pinData.Mode != mode { - t.Error("worng pin mode") + t.Error("wrong pin mode") } if pinData.Cid != ak { t.Error("wrong pin cid") @@ -428,8 +428,8 @@ func TestPinAddOverwriteName(t *testing.T) { } func TestIsPinnedLookup(t *testing.T) { - // Test that lookups work in pins which share - // the same branches. For that construct this tree: + // Test that lookups work in pins which share the same branches. For that + // construct this tree: // // A5->A4->A3->A2->A1->A0 // / / @@ -437,8 +437,8 @@ func TestIsPinnedLookup(t *testing.T) { // \ / // C--------------- // - // This ensures that IsPinned works for all objects both when they - // are pinned and once they have been unpinned. + // This ensures that IsPinned works for all objects both when they are + // pinned and once they have been unpinned. aBranchLen := 6 ctx, cancel := context.WithCancel(context.Background()) @@ -449,8 +449,8 @@ func TestIsPinnedLookup(t *testing.T) { dserv := mdag.NewDAGService(bserv) - // Create new pinner. New will not load anything since there are - // no pins saved in the datastore yet. + // Create new pinner. New will not load anything since there are no pins + // saved in the datastore yet. p, err := New(ctx, dstore, dserv) if err != nil { t.Fatal(err) @@ -563,7 +563,7 @@ func TestPinRecursiveFail(t *testing.T) { t.Fatal(err) } - // NOTE: This isnt a time based test, we expect the pin to fail + // NOTE: This isn't a time based test, we expect the pin to fail mctx, cancel := context.WithTimeout(ctx, time.Millisecond) defer cancel() @@ -582,7 +582,7 @@ func TestPinRecursiveFail(t *testing.T) { t.Fatal(err) } - // this one is time based... but shouldnt cause any issues + // this one is time based... but shouldn't cause any issues mctx, cancel = context.WithTimeout(ctx, time.Second) defer cancel() err = p.Pin(mctx, a, true, "") @@ -732,7 +732,7 @@ func TestLoadDirty(t *testing.T) { t.Fatal("index should be deleted") } - // Create new pinner on same datastore that was never flushed. This should + // Create new pinner on same datastore that was never flushed. This should // detect the dirty flag and repair the indexes. p, err = New(ctx, dstore, dserv) if err != nil { @@ -950,7 +950,7 @@ func makeStore() (ds.Datastore, ipld.DAGService) { return dstore, dserv } -// BenchmarkLoadRebuild loads a pinner that has some number of saved pins, and +// BenchmarkLoad loads a pinner that has some number of saved pins, and // compares the load time when rebuilding indexes to loading without rebuilding // indexes. func BenchmarkLoad(b *testing.B) { @@ -995,8 +995,8 @@ func BenchmarkLoad(b *testing.B) { }) } -// BenchmarkNthPins shows the time it takes to create/save 1 pin when a number -// of other pins already exist. Each run in the series shows performance for +// BenchmarkNthPin shows the time it takes to create/save 1 pin when a number +// of other pins already exist. Each run in the series shows performance for // creating a pin in a larger number of existing pins. func BenchmarkNthPin(b *testing.B) { dstore, dserv := makeStore() @@ -1044,8 +1044,8 @@ func benchmarkNthPin(b *testing.B, count int, pinner ipfspin.Pinner, dserv ipld. } } -// BenchmarkNPins demonstrates creating individual pins. Each run in the -// series shows performance for a larger number of individual pins. +// BenchmarkNPins demonstrates creating individual pins. Each run in the series +// shows performance for a larger number of individual pins. func BenchmarkNPins(b *testing.B) { for count := 128; count < 16386; count <<= 1 { b.Run(fmt.Sprint("PinDS-", count), func(b *testing.B) { @@ -1270,7 +1270,7 @@ func TestCidIndex(t *testing.T) { defer results.Close() // Iterate all pins and check if the corresponding recursive or direct - // index is missing. If the index is missing then create the index. + // index is missing. If the index is missing then create the index. seen = false for r := range results.Next() { if seen { diff --git a/routing/http/filters/filters.go b/routing/http/filters/filters.go index 122f625de..c88bbcf41 100644 --- a/routing/http/filters/filters.go +++ b/routing/http/filters/filters.go @@ -49,7 +49,7 @@ func AddFiltersToURL(baseURL string, protocolFilter, addrFilter []string) string return parsedURL.String() } -// applyFiltersToIter applies the filters to the given iterator and returns a new iterator. +// ApplyFiltersToIter applies the filters to the given iterator and returns a new iterator. // // The function iterates over the input iterator, applying the specified filters to each record. // It supports both positive and negative filters for both addresses and protocols. diff --git a/routing/http/internal/goroutines.go b/routing/http/internal/goroutines.go index 4c44428ab..af5c14e68 100644 --- a/routing/http/internal/goroutines.go +++ b/routing/http/internal/goroutines.go @@ -7,8 +7,10 @@ import ( "github.com/samber/lo" ) -// DoBatch processes a slice of items with concurrency no higher than maxConcurrency by splitting it into batches no larger than maxBatchSize. -// If an error is returned for any batch, the process is short-circuited and the error is immediately returned. +// DoBatch processes a slice of items with concurrency no higher than +// maxConcurrency by splitting it into batches no larger than maxBatchSize. If +// an error is returned for any batch, the process is short-circuited and the +// error is immediately returned. func DoBatch[A any](ctx context.Context, maxBatchSize, maxConcurrency int, items []A, f func(context.Context, []A) error) error { if len(items) == 0 { return nil @@ -62,8 +64,8 @@ func DoBatch[A any](ctx context.Context, maxBatchSize, maxConcurrency int, items // we finished without any errors, congratulations return nil } - // short circuit on the first error we get - // canceling the worker ctx and thus all workers, + // short circuit on the first error we get canceling the worker ctx and + // thus all workers, return err case <-ctx.Done(): return ctx.Err()