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

[web] fix query editing #5574

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

yesmeck
Copy link

@yesmeck yesmeck commented Sep 9, 2022

Description

As #4565 (comment) said, an empty body would show when editing a query string. This PR tries to fix the query editing logic. Also, fixed query replacement.

I added a new API for retrieving and updating query string

fix #4565

Checklist

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

@yesmeck yesmeck force-pushed the fix/query-editing branch 14 times, most recently from 6c80fde to d0863fc Compare September 9, 2022 18:45
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.

Thanks for the PR and sorry for getting at this so late. :) Please see my question below. 😃

@@ -32,7 +32,13 @@ export default function HttpMessage({flow, message}: HttpMessageProps) {
} else {
url = MessageUtils.getContentURL(flow, message, contentView, maxLines + 1);
}
const content = useContent(url, message.contentHash);
let content = useContent(url, message.contentHash);
if (flow.request.method === "GET" && edit) {
Copy link
Member

Choose a reason for hiding this comment

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

Just from the diff it looks like this will break response editing, or am I missing something here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right. I will try to find a more proper way to fix this issue.

Copy link
Author

Choose a reason for hiding this comment

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

Updated and rebased.

@yesmeck yesmeck force-pushed the fix/query-editing branch 12 times, most recently from 59eaa93 to 485ea2f Compare October 26, 2022 09:46
@mhils
Copy link
Member

mhils commented Oct 26, 2022

Thanks. How come you updated this PR from using JS APIs to adding a new REST API endpoint for queries?

@yesmeck
Copy link
Author

yesmeck commented Oct 26, 2022

Do you mean where did I get this idea from, or how did I update this PR?

@mhils
Copy link
Member

mhils commented Oct 26, 2022

If I remember correctly, the initial version in this PR used JavaScript to extract the query from the path (I can't take a look at the code again because you force-pushed). Now there is a new REST endpoint instead. What motivated that change?

@yesmeck
Copy link
Author

yesmeck commented Oct 26, 2022

Before using JavaScript to extract the query from the path, I tried to make the content.data endpoint returns queries. But soon, I realised that POST requests could also have queries that make content.data endpoint not sufficient to handle both bodies and queries. That is why I chose to parse queries from the client side.

After you pointed out that my changes would break response editing, I found that the query replacement is also broken, so I plan to fix it as well. This requires changing the content.data endpoint. But as I said above, using content.data endpoint to handle both bodies and queries is a bit confusing for me. So I decided to create a new API endpoint for query replacement. Now that I have an endpoint for query replacement, I thought, why not get queries from the new endpoint as well.

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.

Show "Query" contentview in mitmweb even if request body is empty
2 participants