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.

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

fix: relative url path for ui mode #29924

Merged
merged 2 commits into from
May 20, 2024

Conversation

bockstaller
Copy link
Contributor

@bockstaller bockstaller commented Mar 13, 2024

This is a follow up #29564

I did a deep dive on a redirect issue I observed in my infrastructure and originally attributed to some configuration mistakes on my part.
I have code hosted on example.com/code and use subdomain proxying. This leads to the uimode being exposed on example.com/code/proxy/{{port}}.

Clicking on the open uimode link shown by vscode redirected with a 302 to example.com/proxy/{{port}}

The absolute redirect url overruled the relative path handling reverse proxies rely on.

This PR turns the absolute into a relative url to avoid this issue.

This comment has been minimized.

@bockstaller
Copy link
Contributor Author

@pavelfeldman I tried to add a test by implementing a proxy in typescript but struggle with the combination of proxying http and ws as well as the multiple testing layers.

Is that something I should invest time in or is continuing without tests ok?

In addition I took a look at tests 1 / ubuntu-22.04 (firefox - Node.js 18) (pull_request) that failure seems like flaky behaviour to me

@@ -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?

@bockstaller bockstaller requested a review from yury-s April 9, 2024 09:04
Signed-off-by: Lukas Bockstaller <lukas.bockstaller@everest-erp.com>
Copy link
Contributor

github-actions bot commented May 7, 2024

Test results for "tests 1"

2 flaky ⚠️ [firefox-page] › page/page-click.spec.ts:850:3 › should not hang when frame is detached
⚠️ [chromium-library] › library/trace-viewer.spec.ts:521:1 › should handle multiple headers

27315 passed, 661 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s merged commit 437b14a into microsoft:main May 20, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants