Don't read body, just set the Content-Length header#68
Don't read body, just set the Content-Length header#68Aldo Giambelluca (xoen) wants to merge 4 commits intomasterfrom
Content-Length header#68Conversation
Aldo Giambelluca (xoen)
requested review from
Andy Driver (andyhd) and
Ravi Kotecha (r4vi)
|
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. |
| proxy.on('proxyReq', function (proxyReq, req, res, options) { | ||
| proxyReq.setHeader('Cookie', insert_auth_cookie(req)); | ||
| proxy_body(req, proxyReq); | ||
| set_content_lenght(req, proxyReq); |
There was a problem hiding this comment.
yes, sorry.
Ravi Kotecha (r4vi)
left a comment
Ravi Kotecha (r4vi)
left a comment
There was a problem hiding this comment.
spelling mistake but otherwise fine, give it a go
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
Aldo Giambelluca (xoen)
force-pushed
the
ag--dont-copy-body
branch
from
1ceef1d to
2384642
Compare
|
(rebased on top of changes Ravi merged yesterday) |
|
Andy Driver (@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)
|
Andy Driver (@andyhd) / Ravi Kotecha (@r4vi) Does this make sense? |
|
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). |
|
Ravi Kotecha (@r4vi) Moved to camelCase, at least in the file touched by this PR: b9cb9cd |
3 participants
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