This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Don't read body, just set the Content-Length header
#68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As said, I've tested this locally (commenting out the authentication logic) and it seems to work but I'd like to try and use this proxy version in front of my RStudio instance and see if it works as expected. |
r4vi
reviewed
May 16, 2019
app/proxy.js
Outdated
| @@ -8,12 +8,12 @@ var proxy = httpProxy.createProxyServer(config.proxy); | |||
|
|
|||
| proxy.on('proxyReq', function (proxyReq, req, res, options) { | |||
| proxyReq.setHeader('Cookie', insert_auth_cookie(req)); | |||
| proxy_body(req, proxyReq); | |||
| set_content_lenght(req, proxyReq); | |||
r4vi
approved these changes
May 16, 2019
We used to read the request body to calculate the `Content-Length`, we now just copy the velue of the `Content-Length` header. The request body is proxied by default, as you'd expect. Tested locally against a running RStudio container and it seems to work. I've uploaded a 37MB ISO image and checked the MD5 of the file uploaded against the checksum of the original file and the file was uploaded without corruption. **NOTE**: This is to simplify the `rstudio-proxy` logic before moving into the other `auth-proxy`. Part of ticket: https://trello.com/c/5twZHrb4/215-try-to-replace-custom-rstudio-auth-proxy-with-our-usual-generic-one
|
(rebased on top of changes Ravi merged yesterday) |
andyhd
approved these changes
May 16, 2019
|
@andyhd I was actually thinking about that as the function become so simple but I left as function to not have duplication. But I have a better idea, move the |
- both `proxyReq` and `proxyReqWs` were doing the same thing. these events are now handled by the same function - removed a layer of indirection in the logic that set the `Cookie` header and using some constants to make what's happening more explicit - export `auth.secureCookie()` as `auth.secureCookie()`, having different names here doesn't add any value and it can be confusing (I was always finding it confusing)
|
FYI: I've tried to use this proxy in front of my RStudio instance and it seems to work fine (tested upload of 37MB file and also to run the usual Shiny App in the viewer). |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We used to read the request body to calculate the
Content-Length,we now just copy the velue of the
Content-Lengthheader.The request body is proxied by default, as you'd expect.
Tested locally against a running RStudio container and it seems to
work. I've uploaded a 37MB ISO image and checked the MD5 of the file
uploaded against the checksum of the original file and the file was
uploaded without corruption.
NOTE: This is to simplify the
rstudio-proxylogic before moving itinto the other
auth-proxy.Part of ticket: https://trello.com/c/5twZHrb4/215-try-to-replace-custom-rstudio-auth-proxy-with-our-usual-generic-one