Fix hijacking when fulfilling requests with empty body. #603
Conversation
|
Your change will be overwritten: Line 1 in c50eada |
|
Can you add a test case that will fail when we use |
Yes. I was too excited to see it work after hours of trying to find the problem with my code. So what can we do about this? |
|
You can use this patch.go file to patch the description of the |
Probably, but I did not dig into the code and what is really happening. From what my tests looked like at first was that the Twirp frontend code kind of ignores the whole request without an error. My (crafted) response looks like this. ctx.Response.SetBody([]byte{}).SetHeader(
"Access-Control-Allow-Origin", siteURL,
"Access-Control-Expose-Headers", "Link",
"Cache-Control", "public, max-age=60",
"Content-Length", "0",
"Content-Type", "application/protobuf",
"Vary", "Origin",
)I did some more testing and when I use this with the unmodified rod, the Twirp code does not return from sending the request. After removing the To make it a bit more interesting the Twirp code runs as WebAssembly code in the browser front end. It is a goroutine and I am sure that it does not crash. My guess is that it waits for more data from the server as it expects a body (there is a content-length header which besides the status code also means that there is a body. So it may just wait and hangs and may eventually time out, but at that time the tests are already completed. I am writing my own little test framework and it was based on ChromeDP at first. I did the same interceptions there and it worked, so there must be something different. I know that I have to base64 convert my body there, but did not check how they do the serialization for the CDP. So yes, I may be able to figure out how to test for it, but I actually would prefer to just reason about the fact that not having a body in the response (200 OK) somehow is "illegal" anyway. It would need to be That thought again leads me to a possible test when checking the raw TCP response and looking at how many lines are sent after the headers. But that is not possible, I guess because there is not really a TCP port to listen to :) |
|
The info you provide is overwhelming for me, I don't know Twirp.
It's not us, but chrome devtools protocol wants us to Can you provide steps to reproduce the issue? |
|
Sorry, but I had the hope, that giving as much detail as I can ring a bell. For reference Twirp For steps to reproduce the problem, I need to figure out a minimal example project. I guess I can build one but I need some time for that. So I checked it again: The chromed implementation also has One difference between using chromedp and rod I noticed is that I have to use base64 encoding for the protobuf data in chromedp and not in rod. I also see that in the chromedp code they are handling the The further investigation makes me think that using chromedp implies to use I still can't find a reason why the response with an empty body fails. So I try to create an example that reproduces this problem. |
|
You can print the raw cdp log from both rod and chromedp, and compare the difference. You can check their doc to print the raw cdp. For rod you can simply use cli flag Lines 45 to 56 in 0bb4269 |
Good idea! I am currently trying to write a minimal example. But may need to stop and do something else. I come back to it soon. |
|
FYI chromedp.NewContext(allocCtx, chromedp.WithDebugf(log.Printf)) |
|
Thank you. I already have quite the large testing framework and logging the CDP is part of that (together with console, exceptions, and request logging). Currently, I suspect the problem is somehow related to the fetch implementation of WASM, as it seems to work fine with |
|
Maybe as a little background: I started to write my own testing framework based on chromedp and made it especially to test PWA that are written using go-app which uses WASM for the frontend. It abstracts many things and makes everything easily traceable with nice logging output (supposed to be structural later).
For the implementation, I am partially close to the ideas that are rod using. So I was thinking about replacing chromedp with rod for my framework as I am currently having quite some overhead as I "run" every action right now. I also like the clear code of rod much better. I use a chain for the tests which traces every call and when an error happens it skips all further steps to the end or to a "catch" phrase (this is WIP). I also have something like "slow motion" as "TimeBetweenActions", "wait for idle requests" as "RequestsIdle" and "hijacking" as "AddInterception" var settings sp.Settings
sr.Start().
CallerLog(false).
SetTimeout(30*time.Second).
//TimeBetweenActions(2*time.Second).
TimeBetweenActions(20*time.Millisecond).
NavigateWait(url, "app-wasm-loader", sp.ByID, sp.NodeNotPresent).
Log("Fake Login as User").
Click(`a[href="/als-benutzer-anmelden"]`, sp.ByQuery, sp.NodeVisible, sp.NodeReady).
WaitVisible("#pagecontent div.prose").
Text("#pagecontent", &text).
Dispatch(func(c *sp.Snoop) error {
c.Logf("Text is %d bytes", len(text))
return nil
}).
MustContainText("#pagecontent", "Lorem ipsum").
Dispatch(func(c *sp.Snoop) error {
return login(c, "test", "Me")
}).
Catch().
// Execute this as fast as possible (not sure if my implementation is good enough)
// The input in the textarea is denounced by 200 ms so we need to wait idle for quite some time
Log("# Test Markdown").
StoreSettings(&settings).
TimeBetweenActions(0*time.Second).
ConsoleLog(true).
RequestLog(true).
AddInterception("**/twirp/v1.API/ConvertToMarkdown", interceptMarkdownPassthrough).
Click(`div.d-sidebar a[data-pcode="md2html"]`).
Query("#pagecontent textarea", sp.NodeVisible).
RequestsIdle(300*time.Millisecond).
SetValue("#pagecontent textarea", "").
RequestsIdle(300*time.Millisecond).
InnerHTML("#pagecontent div.prose", &text, sp.NodeReady).
Dispatch(func(c *sp.Snoop) error {
c.InnerHTML("#pagecontent div.prose", &text).
Logf("InnerHTML is %q", text)
return nil
}).
// Replaces the existing one
AddInterception("**/twirp/v1.API/ConvertToMarkdown", interceptMarkdownReplace).
SendKeys("#pagecontent textarea", "# Überschrift\nTextinhalt\n", sp.ByQuery).
// Need to wait for backend update (something that should actually guarantee it)
RequestsIdle(300*time.Millisecond).
InnerHTML("#pagecontent div.prose", &text, sp.NodeReady).
Dispatch(func(c *sp.Snoop) error {
c.InnerHTML("#pagecontent div.prose", &text).
Logf("InnerHTML is %q", text)
return nil
}).
//
DelInterception("**/twirp/v1.API/ConvertToMarkdown").
SendKeys("#pagecontent textarea", "> Interception removed", sp.ByQuery).
RequestsIdle(300*time.Millisecond).
InnerHTML("#pagecontent div.prose", &text, sp.NodeReady).
Dispatch(func(c *sp.Snoop) error {
c.InnerHTML("#pagecontent div.prose", &text).
Logf("InnerHTML is %q", text)
return nil
}).
RestoreSettings(&settings).
Click(`//div[contains(@class,'d-sidebar')]//div/p[text()="Examples"]/..`).
WaitVisible(`div.d-sidebar a[data-pcode="form"]`).
Click(`div.d-sidebar a[data-pcode="mdmissing"]`).
RequestsIdle(25*time.Millisecond).
MustContainText("#pagecontent", "Error: Data could not be loaded").
CatchEnd()This is how it all started. |
The issue occurred when I was writing some tests for a backend that uses Protobuf messages (with Twirp as the transport layer). An empty body is the optimized version of "all fields" in the response message are empty. If I hijack such a request and just call 'ctx.MustLoadResponse()` without doing anything else, this request suddenly failed. I also could not create this kind of response when I needed it for testing. After removing the omitempty, it works fine in all my test cases. I actually don't see any reason why you would want to omit when empty. However, maybe I am forgetting some use cases.
|
I wrote a (fairly) simple tester as an example of how to reproduce the problem. It does some tests and shows what actually works well and where the problem arises. P.S.: There is no Twirp needed to get the problem to show up. It happens with the standard @maxence-charriere you probably like to have a look at the testing demonstration (I know you are very busy) |
|
I have tried your demo code. The second I think it's safe to remove the |
The problem with that is, that it will be not possible to generate valid requests that have no body. They quite very seldom though. I may try to find out why it works with chromedp again first. |
The difference between them is |
|
I create the proto patch and tried it with and without I could also add test cases for checking if the empty body works and for the no body at all case. Both tests depend on the browser locking up (and there is not even WASM needed) which is kind of the behavior it has without removing the While I am at it, what do you think about my observation that Rod does a raw |
Added a patch for proto / fetch and tests for empty and no body responses
Can you put the raw cdp output of chromedp here to invest?
If we use FetchFulfillRequest, the browser won't actually send the request to the server, so we don't have to worry about it. I plan to drop the use of the |
But I am talking about https://chromedevtools.github.io/devtools-protocol/tot/Fetch#method-getResponseBody which first fetches the original response and then lets me modify it. Rod does this using the raw HTTP request. I use that in my test framework to modify answers instead of building them from scratch.
Yes, I just cutted it and changed the hostnames a bit. The log even contains our private repository paths otherwise. It starts with the requestWillBeSent for Then this is the part where I hook into And use |
I see. I think we need to redesign the hijack a little bit to support it, or the users can use
There's no difference. Can it be that you have used the getResponseBody before the |
No, and the request also does not show up in the backend log. I can show you my (crude) code: gotException := make(chan bool, 1)
chromedp.ListenTarget(sr.Context(), func(ev interface{}) {
switch ev := ev.(type) {
case *rt.EventConsoleAPICalled:
//fmt.Printf("* console.%s call:\n", ev.Type)
if sr.logConsole {
for _, arg := range ev.Args {
sr.logf("%s[C] %s%s\n", color.Cyan, strings.Trim(string(arg.Value), `"`), color.Reset)
}
}
case *network.EventRequestWillBeSent:
sr.openRequests++
if sr.logRequests {
sr.logf("%s[R#%s< %s %s%s", color.Blue, ev.RequestID, ev.Request.Method, elipse(ev.Request.URL, 80), color.Reset)
}
case *network.EventResponseReceived:
sr.openRequests--
sr.lastResponse = time.Now()
if sr.expectRequests != 0 && sr.lastResponse.After(sr.expectAfter) {
sr.expectHappened++
}
if sr.logRequests {
sr.logf("%s>R#%s] (%d) %s%s", color.Blue, ev.RequestID, ev.Response.Status, elipse(ev.Response.URL, 80), color.Reset)
}
case *fetch.EventRequestPaused:
go func(ctx context.Context, ev *fetch.EventRequestPaused) {
// this is where we intercept requests
var a chromedp.Action
if ev.ResponseStatusCode != 0 {
a = fetch.ContinueResponse(ev.RequestID)
} else {
for _, ic := range sr.Interceptions {
match := ic.glob.Match(ev.Request.URL)
if match {
sr.logf("%s>R#Intercept< %s%s", color.LightBlue, ev.Request.URL, color.Reset)
a = ic.Interceptor(sr, ev.RequestID, ev.Request)
}
}
if a == nil {
a = fetch.ContinueRequest(ev.RequestID)
}
}
if err := chromedp.Run(ctx, a); err != nil {
log.Println(fmt.Errorf("EventRP Error: %w", err))
}
}(sr.ctx, ev)
}
})func interceptMarkdownEmpty(t *sp.Snooper, rid fetch.RequestID, req *network.Request) chromedp.Action {
h := req.Headers
if h["accept"] == "application/protobuf" {
return fetch.FulfillRequest(rid, 200).WithBody("")
//return fetch.FulfillRequest(rid, 200) //.WithBody(base64.StdEncoding.EncodeToString(respBytes))
} else {
panic("Intercepting 'accept' of unknown format")
}
} |
|
OK, let's merge it first. I created the issue for getResponseBody: #607 |
The issue occurred when I was writing some tests for a backend that uses Protobuf messages (with Twirp as the transport layer). An empty body is the optimized version of "all fields" in the response message are empty.
If I hijack such a request and just call 'ctx.MustLoadResponse()` without doing anything else, this request suddenly failed. I also could not create this kind of response when I needed it for testing.
After removing the
omitempty, it works fine in all my test cases. I actually don't see any reason why you would want to omit when empty. However, maybe I am forgetting some use cases.