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

Add flow support to relative paths #6562

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

davet2001
Copy link
Contributor

@davet2001 davet2001 commented Dec 19, 2023

Description

#6411 added relative paths to allow the web root for mitmproxy to be placed in locations other than '/'.

But some items were missed from this change, especially manipulating and saving flows, see #6525
This PR modifies several js components so that POST API features work even when the web root is not '/'.

Checklist

  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

@davet2001
Copy link
Contributor Author

Ok, this is partially working. I don't think I can get much further without making the js compile locally and running the full test suite.
@mhils are there any instructions on how to set this up? Also is there any way of running them in a devcontainer?

@mhils
Copy link
Member

mhils commented Dec 19, 2023

You can compile the JS with npm start prod: https://github.com/mitmproxy/mitmproxy/blob/main/release/release.py#L79-L83 :)

@davet2001
Copy link
Contributor Author

I am a bit baffled by the test results. Unfortunately am not at all proficient at npm. I was able to reproduce the npm test failure locally, but can't figure out why it is receiving a completely different POST command to what the test expects e.g.:

    Expected: "/flows/d91165be-ca1f-4612-88a9-c0f8696f3e29/kill", {"method": "POST"}
    Received
           1
              "flows/d91165be-ca1f-4612-88a9-c0f8696f3e29/resume",
              {"method": "POST"},
           2
              "flows/resume",
              {"method": "POST"},
           3
              "flows/d91165be-ca1f-4612-88a9-c0f8696f3e29/kill",
              {"method": "POST"},

Have I messed something up with the JS compilation maybe? Please help :)

Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Sorry for the slow RTT. I was trying to get this done today, but there's a bunch of stuff that isn't mergable at the moment. The errors you are seeing are caused by test failures, check out the mentioned tests and things should become relative clear. Thanks!

@@ -124,7 +124,7 @@ describe("onKeyDown", () => {

it("should handle delete action", () => {
store.dispatch(createKeyEvent("d"));
expect(fetchApi).toBeCalledWith("/flows/1", { method: "DELETE" });
expect(fetchApi).toBeCalledWith("flows/1", { method: "DELETE" });
Copy link
Member

Choose a reason for hiding this comment

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

Calls to fetchApi should still be prefixed with /, fetchApi has explicit logic to make them relative.

@@ -70,9 +70,9 @@ export function updateUrlFromStore(store) {

let url;
if (state.flows.selected.length > 0) {
url = `/flows/${state.flows.selected[0]}/${state.ui.flow.tab}`;
url = `flows/${state.flows.selected[0]}/${state.ui.flow.tab}`;
Copy link
Member

Choose a reason for hiding this comment

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

This breaks all existing links to mitmweb instances, which I'd like to avoid. location.hash does not need relative URLs.

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.

None yet

2 participants