Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: relative url path for ui mode #29924

Merged
merged 2 commits into from May 20, 2024
Merged

fix: relative url path for ui mode #29924

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
112d0e8
fix: relative url path for ui mode
bockstaller Mar 13, 2024
966d466
Merge branch 'main' into lbo-relative-hosting
bockstaller May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/trace/viewer/traceViewer.ts
View file
Open in desktop
Expand Up @@ -117,7 +117,7 @@ async function startTraceViewerServer(traceUrls: string[], options?: OpenTraceVi
const url = await server.start({ preferredPort: port, host });
const { app } = options || {};
const searchQuery = params.length ? '?' + params.join('&') : '';
const urlPath = `/trace/${app || 'index.html'}${searchQuery}`;
const urlPath = `./trace/${app || 'index.html'}${searchQuery}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How doe it fix the issue from the description? AFAIU the urlPath will be used as redirect location only for requests to / on the server and that won't match /code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the world of relative URLs and subpath proxying, we don't care if the traceviewer is accessible via /code /trace-viewer or /base-url-of-companies-xy-test-tooling/playwright´. All queries hitting one of these proxy subpaths are forwarded to the proxy viewers /` route.
The traceViewer is none the wiser that it is exposed via a subpath.

However, this indirection is dependent on the fact that the server only returns relative URLs. This allows the browser to construct the new complete URL.

The problematic behavior now occurs only when there are nested proxies.

  • Infra Proxy: Reverse proxy exposing code via /code/
  • App Proxy: Exposing processes running in a code instance via ./proxy/{{port}}

Without this change:

  1. Browser requests /code/proxy/8090
  2. Infra proxy strips part of the request /code/proxy/8090 -> /proxy/8090
    3 App proxy strips part of the request /proxy/8090 -> /
  3. Trace Viewer receives / and returns 302: /trace/{...}
  4. App proxy adds back its subpath to isolate the different proxied processes from each other /proxy/8090 + /trace/{...} -> /proxy/8090/trace/{...}. The absolute nature of the request propagates.
  5. Infra proxy has no domain knowledge and returns absolute URL /proxy/8090/trace/{...}
  6. Browser receives (via the proxy) 302: /proxy/8090/trace/{...}
  7. Browser follows redirect but hits a 404 because /code is missing.

With this change:

  1. Browser requests /code/proxy/8090
  2. Both proxies strip their proxy paths from the request /code/proxy/8090 -> /
  3. Trace Viewer receives / and returns 302: ./trace/{...}
  4. Proxies build back up the path for the response from / to /code/proxy/8090
  5. Browser receives 302: ./trace/{...} coming from /code/proxy/8090
  6. The Browser combines it with the original request URL (because it is relative). /code/proxy/8090 + ./trace/{...} = /code/proxy/8090/trace/{...}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details explanation. I'm fine with the change, but reading you description of how it works with the app proxy today I get a feeling it will still not work:

  1. App proxy adds back its subpath to isolate the different proxied processes from each other /proxy/8090 + /trace/{...} -> /proxy/8090/trace/{...}. The absolute nature of the request propagates.

Why with the relative url, it won't become /proxy/8090 + ./trace/{...} -> /proxy/8090/trace/{...} ? Does the app proxy treat relative redirect location differently?


server.routePath('/', (request, response) => {
response.statusCode = 302;
Expand Down