redirect_uri vs added /

[jonthegeek]

httr2 (via curl::curl_parse_url(), which in turn is inheriting this behavior from C) always uses a minimum of "/" for the path now. That's almost always ok, but it caused a token request to fail for me via normalize_redirect_uri(), when "http://127.0.0.1:8888" (a path specified for that client) was normalized to "http://127.0.0.1:8888/" (which the client did not recognize). For this particular use-case I was able to patch in a fix by using sub("/$", "", url_build(parsed)) as the uri, but that feels hacky.

I can't think of a situation other than redirect_uri where this really matters. I also ran into it yesterday in a test (the req$url now always has the trailing / for httr2 1.1+ and previously lacked it, which caused a macos CI test to fail when it wasn't caught up with the other OSs yet), but that didn't feel like a big deal. The redirect one prevented me from refreshing a token until I patched it, though.

This kind of should be fixed at the curl level, but I wouldn't always consider this "broken", so I think normalize_redirect_uri is probably the best place.

[jonthegeek]

For the YouTube Data API case, adding /authorize/ to my redirect_uri s worked. So this no longer impacts my particular case, but it's still a potential source of issues and confusion since APIs tend to be strict about trailing / matching for redirect URIs.

[jonthegeek]

Do you have a plan for how to "fix" this? I feel like the best solution might be to treat redirect_uri like text (don't try to normalize it) since this is often a parameter you define in the client setup, and many APIs are strict about what it can be. If it's weird, that should be on the user to deal with.

I can PR something, I just want to make sure I'm fixing the right thing. It looks like normalize_redirect_uri() mostly only matters when it includes deprecated parameters in the all, plus a check for localhost, so maybe it shouldn't change anything unless those deprecated arguments are present?

[hadley]

It looks like most of normalize_redirect_uri() is now deprecated and not used in most cases. I don't think we can get rid of that code yet, but I think we could do something like this:

  old <- parsed <- url_parse(redirect_uri)
  
  ...

   uri = if (identical(old, parsed)) redirect_uri else url_build(parsed),

[hadley]

@jonthegeek do you want to do a PR? I'm aiming for release fairly soon, so if you don't have time in the next few days, I'm happy to do it.

[jonthegeek]

@jonthegeek do you want to do a PR? I'm aiming for release fairly soon, so if you don't have time in the next few days, I'm happy to do it.

I'll knock it out this afternoon. Your plan is pretty much what I as thinking (except cleaner/simpler), so it shouldn't take long.