fix: encode filenames

[zebateira]

This PR fixes upload not working on the website when the filename includes a non ISO-8859-1 character.
We were missing an encode/decodeURIComponent. This only affects the files because the filename is sent via header X-NAME.
I made the fix in the lib/api.js file on the website code. Should this done on the js client instead?

Fixes #532

[olizilla]

Should this done on the js client instead?

Yes. The client should do the encode. The api should decode it before storing it in the db, and return the decoded version in responses, so the user doesn't see them.

We need to add tests and to document that when using the /car and /upload enpoints directly you need to encode the file name, and that the client will do this for you.

@zebateira are you up for trying out this fix on the api and client? we could pair on it some if that would help?

[zebateira]

@olizilla yeah sounds good 👍

[zebateira]

Updated:

• removed logic from website
• added encoding on js client
• added decoding on api

In order to have a test on the api, I added the name to the return body:

web3.storage/packages/api/src/car.js

Line 215 in b195e5b

return new JSONResponse({ cid, name })

I don't think we can test this in another way because of the DB mock right? My intuition was to (1) upload (2) get files list (3) check filename is correct, but since we are not connected to the DB we can't do this?

[zebateira]

Right now this can't be tested fully on the preview link because it points to staging, so only after merging will we see this working fully.

[vasco-santos]

Code looks good, but I think we should not return the name in create just for testing purposes. Name is obtained in listUploads, so it seems like we are adding a test just because.

With this in mind, I think it would be more useful to just add to the mock response (here) a validation where if we receive an invalid name we throw an error instead of a "good" answer. This way, the test is just sending a name that needs to be encoded and validate it does not make it way to the DB request

[vasco-santos]

@zebateira we should also document this. Not sure where it fits better, but perhaps adding information here and we also need the docs

[zebateira]

@vasco-santos yeah, the api test is not the best.

I think it would be more useful to just add to the mock response (here) a validation where if we receive an invalid name we throw an error instead of a "good" answer.

What do you mean add a validation in the mock response? A validation assertion in the mock response code here?
But, how would that assert that we are running this decoding loc:

web3.storage/packages/api/src/car.js

Line 153 in b195e5b

let name = xName && decodeURIComponent(xName)

[vasco-santos]

What do you mean add a validation in the mock response? A validation in the mock response code here?

In the mock server for the graphql post, you can access what data is sent in that post, which means you can inspect there the content of name from https://github.com/web3-storage/web3.storage/blob/fix/filename-encoding/packages/api/src/car.js#L167

The two options are:

• API receives and encoded string (what we did in the client)
• API receives a not encoded problematic string

So two tests for the API to exercise both of these paths. In the mock server (condition that the performed operation is the createUpload), you can get the content of the name property from the request and do something like:

const encoded = encodeURIComponent(name)
const decoded = decodeURIComponent(encoded)
if (decoded !== name) {
  return gqlResponse(500, {
    errors: [{ message: `error message` }]
  })
}

This will fail when you provide to the API a name such as upload name. So you can have the test check if the request failed.

[zebateira]

@vasco-santos thank you for the help!
I added the validation to the db mock as you suggested. The test fails if the filename is not decoded.
Let me know if this works better now.

[vasco-santos]

Cool, I think it is better now 🎉 just another detail similar but for client testing

[vasco-santos]

This looks good! Can we add docs for this to the API as Oli suggested?

[zebateira]

Docs added!
Will PR the docs site in a sec. (Here)

[vasco-santos]

LGTM