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

Review of fix for issue 209 #272

Closed
yungtravla opened this issue May 9, 2018 · 34 comments

Comments

2 participants
@yungtravla
Copy link
Member

commented May 9, 2018

1f8e97d was intended to fix issue #209

What if we set res.Body = ""?

Wouldn't the WasModified() function in modules/http_proxy_js_response.go return false even if we want to serve an empty response body?

} else if j.Body != "" {

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 9, 2018

Mmmm suggestions for a clean approach?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

I think it was fine the way it was: if we want to read/write a body, we must first read it with req.ReadBody() and res.ReadBody().

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 9, 2018

yeah but what about res.Body = "something" ?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

You mean doing that on a request?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Oh I see what you mean, maybe we should only use the bodyRead boolean (and drop the WasModified() function) in order to create a new proxy response?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Actually I think it would be cleaner if we make functions to get/set the body like we have for the headers:

function onRequest(req, res) {
  res.GetBody("default") // Returns "default" because the response was not set yet
  res.SetBody("something")
  res.GetBody("") == "something" // Returns true
}

I think it's best to add a default value if the response was not set, because an empty response body is sometimes intended, and doesn't always mean that the body wasn't set or modified.

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

If you agree with that approach I'm happy to update the caplets afterwards too.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 9, 2018

nope, i don't like, otherwise i would have added getters and setters for everything already :D i need to think a better way, will let you know

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Ok 👍

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

What about rather than having a bodyRead bool in the JSResponse struct, we could have a spoofed bool, so whenever a property like res.Status or res.Body is spoofed in the onRequest() function, we serve the spoofed response instead of the proxied one?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

And if the onResponse() function was modified, we still proxy the response (but don't serve it) for logging purposes.

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Looking deeper into the code I realize that these things are already the case. The problem is that a new JSResponse struct already has the Body param set to an empty string, which could be the result of both proxying and spoofing.

Maybe we can find a way to set the default value of the Body param to nil, and later on replace it with a string? I think goproxy would need to accept a nil value for the Body param in its .NewResponse() function.

I think the patch was not required. Having to call req.ReadBody() and res.ReadBody() before you are able to do req.Body = "" and res.Body = "" makes perfect sense to me.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 10, 2018

so? not sure what to do with this issue :/

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 12, 2018

thinking about this over and over, we basically have two options (unless we don't want to mess with reflection to see if and how the object changed, but i don't event know if that's possible, will research):

  1. We forbid direct access to the fields and implement every single getter and setter (both of the request and response for consistency).
  2. We only implement a ClearBody method that will set the Body to "" but still signaling (somehow) the object changed.

I vote for 2 ...

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 13, 2018

Me too 😄

res.ToResponse(req)
req.ToRequest()

These functions seem to only be of use in other Go functions, but it is part of the JS bindings. Right now it tries to return a http.Request or http.Response (instead of an object) in the JS interpreter.

What if we enable these inside proxy scripts?

And if we can also create new packets, for example:

http_proxy_js_request.go

func Request(j *JSRequest) {
  // Check if j is missing required params
  // Return j
}

http_proxy_js_response.go

func Response(j *JSResponse) {
  // Create goproxy response with values from j
  // Return j
}

We can then detect a truly empty res parameter in the onRequest() call.
If it's not empty (it has at least a status code) then we serve it, otherwise we proxy the response.

Because sending requests is restricted to Golang, there are some limitations for the JS proxy scripts. If we can do this in JS, it could allow for more proxy modules, for example:

Bypass HTTPS redirect loop

function onResponse(req, res) {
  if (res.Status == 301 || res.Status == 302) {
    location = res.GetHeader("Location", "")
    if ( location.match(/https:\/\//i) ) {
      ssl_req        = req
      ssl_req.Scheme = "https"

      ssl_res = new Response({})
      ssl_res = ssl_res.ToResponse(ssl_req)
      // This could be simplified to:
      //ssl_res = toResponse(ssl_req)
    }
  }
}

Serve empty response without proxying

function onRequest(req, res) { // res == false
  res = new Response({
    "Status":  200,
    "Headers": {},
    "Body":    ""
  })
}

We can then reflect on the type of the res param returned by the onRequest() call, and if it's a valid JSResponse then we serve it, if it's false, we proxy it.

Thoughts?

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 13, 2018

man, this issue is just for Body = "", how this new function is related? i'm getting confused

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 13, 2018

Basically i'm saying what you said here

mess with reflection to see if and how the object changed

But instead of just being able to show that the object changed, we create the object in the js proxy script, that way the proxy module always knows when we want to serve a handcrafted response or not, and it would also allow us to fetch multiple responses before serving a request.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

i don't know, i don't like the idea of breaking backward compatibility just because of Body = "" ... i say we should stop changing the JS api :)

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Also, and we already had this conversation:

... fetch multiple responses before serving a request.

no, this stuff should not happen inside a js module as there's a global lock every time a callback is executed, meaning, if you do I/O inside, it'll block until it has done.

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

So we leave this res.Body = "" issue?

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

Oh just saw f48fedd

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

Ok, so it wouldn't be smart to send off multiple requests asynchronously, is there no way to do it synchronous? Because I feel like the problem of res.Body = "" being ignored is still the case.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Man, reading your initial report, the problem is that if we set Body to an empty string, the response is not detected as changed ... now it is, if using ClearBody as we both already agreed about.

Not sure how this is strictly related to performing requests from JS modules, let's not mix topics in github issues ... one problem, one fix.

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

Yeah, the issue I began with is not fixed.

What if we set res.Body = ""?

Wouldn't the WasModified() function in modules/http_proxy_js_response.go return false even if we want to serve an empty response body?

This still happens...

The code I posted is meant as a way to get around this issue too.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

yes, because if you want to clear the body, you have to use ClearBody as, again, we both agreed:

agree

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

"Me too" was in response to

thinking about this over and over

Might not have been clear, but the issue remains.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

The issue is "having the ability to clear the body", which is solved with ClearBody from my pov. I don't like the idea of creating the whole response or request object inside the javascript itself as it leaves too much freedom to the user (we should validate every single key => value being set in the object) ... that was the workaround to overcome this problem, which was solved in a easier and cleaner way ...

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

think how many (wrong) ways the user could create that response object from a javascript object ... and we should handle all of them ... nope :D

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

res.ClearBody() doesn't solve the whole issue but it does allow us to set an empty response body without having to do res.ReadBody() or change anything else about the response.
res.Body = "" is still being seen as an unchanged response at this stage though.

I suppose it would require more parsing, but I do think it makes sense to mirror as much http stuff in the JS api as we can :)

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

res.Body = "" is still being seen as an unchanged response at this stage though.

Yes, as, for instance:

res.IamAfieldThatdoesNotExist = ""

would work, where "work" = won't throw errors but won't have any effect at all ... since the only way for us to handle that would be entirely wrapping the object, but that'd lead to a bad design.

Since an API has been created to do what res.Body = "" was supposed to be doing, functionally the issue has been solved.

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

True, but res.Body does exist, and I don't see why res.Body = "" should not do the exact same as res.ClearBody().

Plus, having to do res.ClearBody() in onRequest() just to serve an empty response body doesn't really make sense to me, it sounds more like a function you'd call in onResponse() to clear the proxied body.

@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

ok, we disagree, IMO it's overkill if it can be done with a method ¯\_(ツ)_/¯

@yungtravla

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

But do you see my point?

res.Body = "" will effectively wipe the body in the onRequest() call (which is already empty to begin with), then it will ignore the fact that we said the response body should be empty, proxy it, and then serve the proxied response. This requires you to download every response even if you weren't gonna serve it anyway.

res.ClearBody() does exactly the same except it also states that the body was set to empty with an additional bool to avoid unwanted proxying.

Doing res.ClearBody() is no different than doing:

res.ReadBody()
res.Body = ""
@evilsocket

This comment has been minimized.

Copy link
Member

commented May 14, 2018

yes i see your point but i think it's not worth overcomplicating things since now there's an API for that purpose ... res.Body = "" is simply something we do not support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.