Handle HTTP redirects #754

hadley · 2023-03-09T22:24:18Z

Fixes #674

Uploading to shinyapps.io works without redirects because the result of client$updateBundleStatus(), which needs the redirect, is not used.

@aronatkins any ideas for testing? I verified that this works with an interactive deployment to shinyapps, and the httpRequest() path is tested via validateConnectUrl(). But currently no automated tests hit the httpRequestWithBody() path. Would you mind also verifying that connect/shinyapps doesn't emit any 307 or 308 redirects that I should be respecting?

Fixes #674

hadley · 2023-03-09T22:36:58Z

R/http-libcurl.R

@@ -78,40 +79,22 @@ httpLibCurl <- function(protocol,
    con <- file(contentFile, "rb")
    on.exit(if (!is.null(con)) close(con), add = TRUE)

-    if (identical(method, "POST")) {


This is possibly overly aggressive, but the comment doesn't make sense to me as the 3xx redirects only apply to the result of the upload, not the upload itself. This is possibly a misdiagnosis of an older problem, because I don't think there should be a general difference between PUTs and POSTs. It's possible that this might have also resolved #544.

Looks like this code existed from day one of the libcurl implementation: #348

hadley · 2023-03-10T00:02:47Z

R/http-libcurl.R

-      else
-        stop(e)
-    }))
+  time <- system.time(


I also simplified this since the user interrupt error message has changed so that had no effect and I'm pretty sure that "transfer closed" is a legit error we do want to bubble up.

I'm pretty sure that some of this logic was trying to trap auth errors during uploads. When there's an auth failure, the client may not see the entire body as sent. This code was trying to handle that situation, but never quite handled most of the cases.

In poking around a little, we see terminated uploads for rpubs -- things that are larger than the bundle upload limit. #450 (comment) for quite a bit of discussion about it, but I never really figured out the cleanest way to handle those errors.

We wanted to try to ignore the "upload" problem and extract the real HTTP error in the response, if possible.

I don't know the best way to react to this type of error, nor if we really need to maintain the old code -- mostly, this helps inform a testing strategy.

Ok, I'll have a go with some uploading some really large files and see if I can recreate it.

I uploaded a 1 Gb app to shinyapps and Posit connect, and didn't see any problems. I'll follow up on the rpubs issue later since it's already individually tracked.

Connect users generally see these errors when there is an intermediate proxy which limits the upload payload size. For example, nginx may use a restricted client_max_body_size.

https://docs.posit.co/connect/admin/proxy/#simple-configuration

Focus on the rpubs case, since I know that's reliable.

I uploaded a 33 Mb html with no problems. You can see it in all its glory at https://rpubs.com/hadley/large-file.

Also tested updating, since that uses a PUT instead of POST.

Was that a large Rmd or a large bundle? Because we gzip, it's harder to get a large bundle. I ended up using a specific document when last playing with that issue, but it's likely changed since I tried.. I think I have a copy locally somewhere. #450 (comment)

@aronatkins From the progress bar 😄 Uploaded 23796573 bytes... so ~22 Mb.

aronatkins

I'll dig into some of the possible response codes tomorrow.

aronatkins · 2023-03-10T00:14:17Z

R/http-libcurl.R

-      else
-        stop(e)
-    }))
+  time <- system.time(


I'm pretty sure that some of this logic was trying to trap auth errors during uploads. When there's an auth failure, the client may not see the entire body as sent. This code was trying to handle that situation, but never quite handled most of the cases.

In poking around a little, we see terminated uploads for rpubs -- things that are larger than the bundle upload limit. #450 (comment) for quite a bit of discussion about it, but I never really figured out the cleanest way to handle those errors.

We wanted to try to ignore the "upload" problem and extract the real HTTP error in the response, if possible.

I don't know the best way to react to this type of error, nor if we really need to maintain the old code -- mostly, this helps inform a testing strategy.

R/http.R

aronatkins · 2023-03-10T02:12:34Z

R/http.R

+    # https://www.rfc-editor.org/rfc/rfc9110.html#name-redirection-3xx
+    service <- parseHttpUrl(httpResponse$location)
+    authed_headers <- c(headers, authHeaders(authInfo, "GET", service$path))
+    httpResponse <- http(


This redirect handling is assuming that the original request (and its payload) do not need to be re-sent. Is that the situation you're seeing?

It feels like the redirection that's set-up for colorado might demand a replay.

curl -v http://colorado.rstudio.com/rsc/__api__/server_settings #> HTTP/1.1 301 Moved Permanently #> Location: https://colorado.rstudio.com/rsc/__api__/server_settings curl -v https://colorado.rstudio.com/rsc/__api__/server_settings #> HTTP/1.1 302 Found #> Location: http://colorado.posit.co/rsc/__api__/server_settings

The original request never landed.

Some systems support 100-continue request/response to avoid sending the payload before receiving confirmation; I'm not sure that any of these servers support that workflow.

Most resources will indicate that you should not automatically replay the request in cases like colorado - a shift from one hostname to another - without explicit confirmation.

We probably want at least:

Be transparent about the redirect.

Offer an option to err rather than walk through the redirect.

To precisely follow the spec we should re-POST with body only for 307 and 308. I'm happy to do that, but I suspect it won't appreciably change behaviour because most folks won't be using those status codes.

To be clear, I don't think code is a panacea for all redirect related issues, it's just an important first step. If we really want to handle a server changing host names, I think we'll need code that detects the problem and then updates the record on disk. But I'm not sure how important that problem is and I'm not sure we need to do it proactively, or should wait on someone to file an issue.

The colorado examples give us two motivating examples:

HTTP => HTTPS

Host rename

I'm not sure that there's a good answer, especially since the colorado example shows us that 301/302 are much more common than 307/308.

Thinking out loud:

Automatic replay of all redirects requires no user intervention, which is a smooth experience.

Server URLs are left stale, meaning every subsequent request hits the redirect penalty.

Server names are left stale, meaning the UI / metadata will always reflect the old name.

Account records reflect the stale server name.

If we ignore updating the server and account records for a moment, we could run a pre-flight check against the client URL to make sure it is still "correct" and update the URL in-memory. That would correct the following requests without hitting multiple redirects and would give us a single place to ask for confirmation, log, or reject the redirect.

In other words, run something like validateServerUrl before using the client to do meaningful work.

Notes to self: need something like validateConnectServer() that we run in deployApp() before hitting any endpoints. If url has redirected, need to prompt the user to update the record on disk (if interactive), or otherwise warn or continue. Might want to consider automatically updating if the hostname stays the name.

While making coffee this morning, I suddenly realised that we can't just update an existing server record on disk, because that will break existing deployments. We need some way to also "redirect" existing server records to a new server record. So I won't tackle that here, but consider it in a future PR.

Yeah; the association between the server records and the accounts and deployments records came up during our chat. The pre-flight can ask if folks should proceed through the redirect, but it's not straightforward to rewrite the records.

Tracking this in #760.

aronatkins · 2023-03-10T02:12:52Z

R/http.R

@@ -355,6 +391,7 @@ authHeaders <- function(authInfo, method, path, file = NULL) {
  }
 }

+# https://github.com/rstudio/connect/wiki/token-authentication#request-signing-rsconnect


fyi: this is an internal link.

DESCRIPTION

R/http-libcurl.R

R/deployApp.R

aronatkins · 2023-03-13T13:43:15Z

R/http-libcurl.R

-      else
-        stop(e)
-    }))
+  time <- system.time(


Was that a large Rmd or a large bundle? Because we gzip, it's harder to get a large bundle. I ended up using a specific document when last playing with that issue, but it's likely changed since I tried.. I think I have a copy locally somewhere. #450 (comment)

aronatkins · 2023-03-13T13:50:14Z

R/http-libcurl.R

+    curl::handle_setopt(
+      handle,
+      noprogress = !progress,
+      upload = TRUE,


Have we lost post = TRUE, which uses a POST request? Is that redundant with the curl::handle_setopt(handle, customrequest = method)?

Hmmm, good catch and good question. The post is a pretty complicated option that changes a bunch of stuff behind the scenes (e.g. adds Content-Type: application/x-www-form-urlencoded, Expect: 100-continue, headers etc). I have a vague recollection that this also changes the redirect handling (because it'll follow the spec for 307/308), but that doesn't matter here since we've suppressed that.

I'm neutral on whether to preserve it or drop it.

In favour of dropping it: the code in this PR works and it's very clear exactly how we're modifying the request.

In favour of keeping: I know curl applies more special POST logic if it's set, and that may save us from grief down the road.

OK dropping it.

#Conflicts: # R/ide.R # tests/testthat/test-ide.R

hadley added 2 commits March 9, 2023 16:20

Handle HTTP redirects

1435998

Fixes #674

libCurl updates

a1dbd1d

hadley commented Mar 9, 2023

View reviewed changes

hadley requested a review from aronatkins March 9, 2023 22:44

Removed special case error handling

2626fbd

hadley commented Mar 10, 2023

View reviewed changes

hadley added 2 commits March 9, 2023 18:03

Merged origin/main into redirects

b1f29af

Add news bullet

40adec3

aronatkins reviewed Mar 10, 2023

View reviewed changes

Conveniences for uploading large files

07ae847

hadley commented Mar 12, 2023

View reviewed changes

DESCRIPTION Outdated Show resolved Hide resolved

hadley commented Mar 12, 2023

View reviewed changes

R/http-libcurl.R Outdated Show resolved Hide resolved

hadley added 4 commits March 13, 2023 07:57

Conditionally set larger buffer size

bb78ca3

Remove buffersize; it's probably not worth it

bc96f64

Only show progress in interactive contexts

04795c3

Narrow redirect

98fed09

hadley mentioned this pull request Mar 13, 2023

Handle host changes #760

Open

Add news bullet

7a8e908

aronatkins approved these changes Mar 13, 2023

View reviewed changes

hadley added 2 commits March 13, 2023 09:41

Simplify bytes formatting

4302c69

Merge commit 'cf0972d0d69357cf2bc81ab140f940085aac92cb'

9dd2437

#Conflicts: # R/ide.R # tests/testthat/test-ide.R

hadley merged commit 1342535 into main Mar 13, 2023

hadley deleted the redirects branch March 13, 2023 15:58

aronatkins mentioned this pull request Jun 10, 2024

Handle redirects rstudio/connectapi#201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle HTTP redirects #754

Handle HTTP redirects #754

hadley commented Mar 9, 2023 •

edited

Loading

hadley Mar 9, 2023

hadley Mar 9, 2023

hadley Mar 10, 2023 •

edited

Loading

aronatkins Mar 10, 2023

hadley Mar 10, 2023

hadley Mar 12, 2023

aronatkins Mar 13, 2023

hadley Mar 13, 2023

hadley Mar 13, 2023

aronatkins Mar 13, 2023

hadley Mar 13, 2023

aronatkins left a comment

aronatkins Mar 10, 2023

aronatkins Mar 10, 2023

hadley Mar 10, 2023

aronatkins Mar 10, 2023

hadley Mar 10, 2023

hadley Mar 12, 2023

aronatkins Mar 13, 2023

hadley Mar 13, 2023

aronatkins Mar 10, 2023

aronatkins Mar 13, 2023

aronatkins Mar 13, 2023

hadley Mar 13, 2023

aronatkins Mar 13, 2023

Handle HTTP redirects #754

Handle HTTP redirects #754

Conversation

hadley commented Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aronatkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Mar 9, 2023 •

edited

Loading

hadley Mar 10, 2023 •

edited

Loading