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 proxy body write condition #350

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Conversation

TheSharpieOne
Copy link
Collaborator

AFAICT the current condition can never be true?

image
image

With this change, it will be true when proxyObject.statusCode is not 304
image
image

Copy link
Contributor

@robmcguinness robmcguinness left a comment

Choose a reason for hiding this comment

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

LGTM

@robmcguinness robmcguinness merged commit d53e8df into master Oct 29, 2019
@TheSharpieOne TheSharpieOne deleted the TheSharpieOne-patch-1 branch October 29, 2019 20:19
@GoPro16
Copy link
Contributor

GoPro16 commented Oct 31, 2019

This PR broke proxying as a whole. I think it should have been status === 304

@TheSharpieOne
Copy link
Collaborator Author

but 304 indicates that there would be no body and nothing to rewrite? Checking that it is not 304 before attempting to rewrite the body makes sense. Checking that it has to be 304 in order to perform the rewrite doesn't make sense.

https://httpstatuses.com/304

there is no need for the server to transfer a representation of the target resource because the request indicates that the client, which made the request conditional, already has a valid representation; the server is therefore redirecting the client to make use of that stored representation as if it were the payload of a 200 OK response

Also, I made this change locally (since I needed it to rewrite stuff in the body) and it worked fine for me. Previously it was not rewriting the body as I believe it was supposed to so I went looking and found this.

@GoPro16
Copy link
Contributor

GoPro16 commented Oct 31, 2019

What http client are you using to fetch axios? We were getting errors with all our service calls.

net::ERR_CONTENT_LENGTH_MISMATCH 200 (OK)

That to me indicates we successfully rewrote the payload but the content length didn't get auto updated using the node module.

From looking at it with @robmcguinness it seems like the middleware was added but the logic never worked for 2 years which is why we didn't run into this error the first time we added it is what I am thinking

Sidenote

I reverted it for now and commented out the logic until we can find the issue that was happening and fix it for good.

@TheSharpieOne
Copy link
Collaborator Author

TheSharpieOne commented Oct 31, 2019

Yeah, I am using axios. Maybe some coincidence that the lengths matched for the origin and the rewrite in my case.
It looks like the server I am proxying to doesn't send content-length header...

Actually, it probably sends the content-length, but the node-http-proxy-json lib removes it before sending: https://github.com/langjt/node-http-proxy-json/blob/07d9c270ca941e3d39e32a51f49e5eba0a6d200f/index.js#L23
Either way, the logic should be right, just need to get it to also rewrite the length header.

@GoPro16
Copy link
Contributor

GoPro16 commented Oct 31, 2019

Created an issue so we don't lose track of this

@TheSharpieOne
Copy link
Collaborator Author

You only need to bump the version of node-http-proxy-json. Looks like the one you were using was really old.

@robmcguinness
Copy link
Contributor

It also appears the lib handles empty payloads now langjt/node-http-proxy-json@9b9f7d3

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

3 participants