Unescaped path param causes failure on initial direct access to proxied app

[sclevine]

Unexpected Behavior

(Tested with Teleport v10.1.4, with both the cluster and agent deployed via helm.)

For me, #13832 appears to have introduced a regression in the following scenario:

1. Given app.example.com, proxied by a teleport cluster at example.com
2. Log out of example.com (or open an anonymous browser window)
3. Navigate to https://example.com
4. Login as a user with access to app.example.com
5. Navigate directly to https://app.example.com (without using the /web/cluster/example.com/apps launcher)

If the multi-app launcher page at /web/cluster/example.com/apps is used to launch the app, the error does not occur.
Once the auth workflow is complete, the error does not appear on subsequent app access while the user is still authenticated.

Analysis

On initial redirect, the backend escapes ? in the URL into %3F:

$ curl https://app.example.com
...
< HTTP/2 302
< content-type: text/html; charset=utf-8
< location: https://example.com:443/web/launch/app.example.com%3Fpath=/
< content-length: 71
< date: Tue, 16 Aug 2022 04:03:21 GMT

This causes the frontend to pass app.example.com%3Fpath as fqdnHint to /v1/webapi/apps/:fqdnHint.
Finally, the backend parses :fqdnHint as if it were a domain name.

Call to backend:
https://github.com/gravitational/webapps/blob/v10.1.4/packages/teleport/src/AppLauncher/useAppLauncher.ts#L56
https://github.com/gravitational/webapps/blob/v10.1.4/packages/teleport/src/services/apps/apps.ts#L55

Backend code that fails with escaped path:
https://github.com/gravitational/teleport/blob/v10.1.4/lib/web/apps.go#L106

Eventually reaches this error, matching the screenshot:
https://github.com/gravitational/teleport/blob/v10.1.4/lib/web/app/match.go#L138

The escaped ? is intentional looking at these tests as well as the discussion in #13832.

Maybe the FQDN hint should be parsed before being passed along here?
https://github.com/gravitational/teleport/blob/v10.1.4/lib/web/apps.go#L291

Happy to open a PR if that seems right.

(By the way, this is my first week working at Teleport on the Cloud Team -- feedback very welcome 🙂)

[zmb3]

@avatus is off today, but let's have him take a look. I tend to agree with the analysis above.

[sclevine]

A few notes after chatting with @avatus today:

• The escaped ? does not appear to be intentional. The intent here was not to encode the query as a path segment:
  

  teleport/lib/web/app/handler.go

  Line 386 in f095c6f

  Path: fmt.Sprintf("/web/launch/%v?path=%v", raddr.Host(), r.URL.Path),

• Because & is a valid character in a path segment (r.URL.Path above), the path query param should be escaped. Otherwise a crafted path coming from a link could be used to pass additional query params to the /web/launch endpoint. Just removing the escape ? is not sufficient. Both of these are bad results:

  stephen@teleport cloud % curl -v 'https://dc.example.com/pathwith&malicious=value' 2>&1|grep location:
  < location: https://example.com:443/web/launch/dc.example.com%3Fpath=/pathwith&malicious=value

  stephen@teleport cloud % curl -v 'https://dc.example.com/pathwith&malicious=value' 2>&1|grep location:
  < location: https://example.com:443/web/launch/dc.example.com?path=/pathwith&malicious=value

[ravicious]

The fix has been shipped in 10.1.9.