storj / edge

Storj edge services (including multi-tenant, S3-compatible server to interact with the Storj network)
GNU Affero General Public License v3.0
48 stars 18 forks source link

Link Sharing Service should mostly use unescaped paths internally #371

Closed amwolff closed 8 months ago

amwolff commented 8 months ago

Current Behavior

This is not a bug per se, but it's something that we need to verify doesn't happen in this codebase anymore.

Here's the Link Sharing Service's example link:

https://link.storjshare.io/s/jxsf2fq6vw5gkgtglgdvd3nsadnq/link/it%27s%20working%21.webp?wrap=1

Because certain characters cannot be sent within the URL, these characters are escaped.

Something I've been suggesting in code reviews (and what turned out to be a noticeable mistake, so this is an attempt to redeem myself) is using unescaped paths internally to preserve whatever the client sent to us. In general, there are multiple possible escaped forms of any path, and this distinction is currently only important for https://pkg.go.dev/storj.io/gateway-mt@v1.64.1/pkg/linksharing/sharing/internal/signed, but even there it's only used to make the signing code less fragile.

So one of the issues we quickly discovered was that redirects were broken: because we were using unescaped paths internally while redirecting from http:// to https:// or doing a backward-compatible redirect (without to with /s/ in the path) and because we didn't maintain the context of the escaped path hint, we were double-escaping URLs, redirecting users to nonexistent locations.

Example

Input:

http://link.storjshare.io/s/jxsf2fq6vw5gkgtglgdvd3nsadnq/link/it%27s%20working%21.webp?wrap=1

Redirect:

https://link.storjshare.io/s/jxsf2fq6vw5gkgtglgdvd3nsadnq/link/it%2527s%2520working%2521.webp?wrap=1

Correct:

https://link.storjshare.io/s/jxsf2fq6vw5gkgtglgdvd3nsadnq/link/it%27s%20working%21.webp?wrap=1

Possible Solution

If users want to distinguish between / and %2f in the object's path, they should double-encode the forward slash. We could detect this on our side, but I'm unsure if it's worth the hassle. @ferristocrat do you think this is reasonable?

Possible Implementation

There are already changes that address this issue:

  1. https://review.dev.storj.io/c/storj/gateway-mt/+/11447
  2. https://review.dev.storj.io/c/storj/gateway-mt/+/11527
  3. Chain of changes by Sean: a. https://review.dev.storj.io/c/storj/gateway-mt/+/11474 b. https://review.dev.storj.io/c/storj/gateway-mt/+/11528 c. https://review.dev.storj.io/c/storj/gateway-mt/+/11530

AC

We need to merge all of the above and check for further misuse in the codebase.

storj-gerrit[bot] commented 8 months ago

Change cmd/authservice-admin: use unescaped paths internally mentions this issue.