fix!: add escaped abspath header

[hacdias]

See ipfs/kubo#10065 and golang/go#60674.

Problem: we used to send binary data in abspath header when sending multipart requests. This is not compliant to the RFC but used to be allowed by Go (see linked issue). It is no longer possible to read multipart requests with binary data on their headers from Go 1.20.

Solution: add a new header with the encoded version of the abspath. Unfortunately, we cannot send both the old and the new header, as Go 1.20 parses all headers when reading the multipart "part" and immediately fails. Therefore, we have to be careful. In this PR, we add an option to NewMultiFileReader: this option will tell which header to use. See code for more details.

[codecov]

Codecov Report

Merging #434 (dfc4949) into main (db4e43a) will decrease coverage by 1.25%.
Report is 1 commits behind head on main.
The diff coverage is 36.53%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
- Coverage   50.11%   48.86%   -1.25%     
==========================================
  Files         249      246       -3     
  Lines       29998    29923      -75     
==========================================
- Hits        15033    14622     -411     
- Misses      13532    13874     +342     
+ Partials     1433     1427       -6     

Files Changed	Coverage Δ
bitswap/network/connecteventmanager.go	0.00% <0.00%> (-78.81%)	⬇️
files/multipartfile.go	84.72% <70.00%> (-1.21%)	⬇️
files/multifilereader.go	82.79% <100.00%> (+0.97%)	⬆️

... and 16 files with indirect coverage changes

[Jorropo]

So after discussions with @aschmahmann about concerns of silent breaking changes and @hacdias about fixing this, we are gonna go with this solution for binary file names in the ipfs add endpoint:

• leave current abspath header as-is, this header wont support binary encoded filepaths anymore due to the change in golang std;
• add a new abspath-percent header that is exactly like abspath but url.QueryEscapeed, kubo will then prefer using abspath-percent if present but use abspath if not
• new clients will send both headers

This gives us this compatibility table:

	New Client	Old Client
New Server	Works fully through abspath-percent	Binary file paths do not work
Old Server	Works fully through abspath	Works fully through abspath

I don't know if headers are lazily or not decoded, so we might need to add an other response header indicating if abspath-percent is supported and do a HEAD request to identify if we are talking to a new or old server.

[hacdias]

Note that this will be more involved than initially planned. The current PR is not yet ready. We cannot send both headers at the same time. The net/textproto package will decode all headers immediately and therefore fail if it sees the new header. Therefore, we need to be able to detect at the client if the server is "new enough".

My idea is to add a new argument to NewMultiFileReader where we indicate if we want to send the raw absolute path or the new encoded one. I don't see where else to do this. This is used in the following places:

• kubo/client/rpc in 2 places
• go-ipfs-cmds (I would argue this should be part of Kubo) in 2 places
• go-ipfs-api in 6 places

In those 3 packages. My idea is to just fetch /api/v0/version once at the first time it is required. Check if Kubo version if >= 0.23.0 (including dev versions). I don't think this will be a necessarily easy change.

[Jorropo]

I was thinking the server would have a new Support-Percent-Abspath-Header then NewMultiFileReader would start with a HEAD request. I guess this would be less efficient because we would need to check each time, or cache it once.
I think a new option is fine as long as you follow the functional option parameters (so it defaults to % encoding unless you explicitly ask it not to)

Edit: this code does not do HTTP stuff, it just handles readers and writers, @hacdias options looks the least awful one that is still somewhat backward compatible.

[Jorropo]

LGTM

[lidel]

Seems to be missing test for scenario with non-ASCII abspath this PR is trying to fix?
(+ might be simplified – see comment inline, but that is fine to disregard)

[hacdias]

@lidel, as asked (makes sense), I added tests with the new header and binary filenames. Surprise surprise: our CI is still running on Go 1.19 for go this. Therefore, all tests will fail on go this and pass on go next. Therefore, this is likely blocked by #431 unless we want to be merging failing tests.

Fixed with separate tests with tags as per @Jorropo's suggestion.

[lidel]

The tests look hairy, but acceptable given this header is only used for experimental filestore feature in Kubo. We will have to revisit code related to --nocopy at some point anyway.
Small nit inline but lgtm.

[hacdias]

@Jorropo @lidel we need to release this before we bubble up to ipfs/go-ipfs-api#305 and ipfs/go-ipfs-cmds#243 so we don't have commit tags. Only when those two are merged can we also update Kubo.