feat: improve behavior of HTTP redirects #182
Conversation
https://docs.travis-ci.com/user/caching/#pip-cache Signed-off-by: Norbert Biczo <[email protected]>
This commit modifies the Python core so that it will include "safe" headers when performing a cross-site redirect where both the original and redirected hosts are within IBM's "cloud.ibm.com" domain. Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/support-redirect
branch
from
dc1d65d
to
4d221e7
Compare
Signed-off-by: Norbert Biczo <[email protected]>
|
I've converted this PR to to draft, to prevent merging it before the necessary CISO approvals. |
ibm_cloud_sdk_core/base_service.py
Outdated
|
|
||
| # If both the original and the redirected URL are under the `.cloud.ibm.com` domain, | ||
| # copy the safe headers that are used for authentication purposes, | ||
| if self.service_url.endswith('.cloud.ibm.com') and urlparse(next_request.url).netloc.endswith( |
There was a problem hiding this comment.
self.service_url.endswith('.cloud.ibm.com') will most definitely not work in the general case. To see why, consider a service url like https://myhost.ibm.com/api or https://myhost.ibm.com:8080. I think you'll need to parse self.service_url and extract the hostname from it.
There was a problem hiding this comment.
Yeah, that's a good point and I already parse the url in the second part of the expression... :)
ibm_cloud_sdk_core/base_service.py
Outdated
|
|
||
| # If we reached the max number of redirects and the last response is still a redirect | ||
| # stop processing the response and return an error to the user. | ||
| if redirects_count == MAX_REDIRECTS and response.is_redirect: |
There was a problem hiding this comment.
Why not do the limit check immediately after incrementing redirect_count above? I think it could be cleaner as you would just need to check:
if redirects_count >= MAX_REDIRECTS:
There was a problem hiding this comment.
Makes sense! I need to excuse myself so: initially I implemented the error handling in a different way then I forgot to refactor that check.. 😄
test/test_base_service.py
Outdated
| @@ -788,6 +788,166 @@ def test_retry_config_external(): | |||
| assert retry_err.value.reason == error | |||
|
|
|||
|
|
|||
| @responses.activate | |||
There was a problem hiding this comment.
Need to add some additional tests:
- redirect to same host (both inside cloud.ibm.com)
- redirect to same host (both outside cloud.ibm.com)
Also, because you are completely overriding the default "follow redirects" behavior, I think you'll need to add some tests for redirect scenarios involving POST and a representative sample of redirect status codes. There are some subtle gotcha's when implementing redirects and we need to make sure that we are providing equivalent function as compared to the default "follow redirects" behavior.
I had to do this in the Java implementation as well. For example, for 301/302 status codes, a POST should be re-written as a GET with no body (it may fail in the redirected server, but that's what the HTTP semantics should be). On the other hand, 307/308 status codes should NOT cause a POST to be re-written as a GET.
You should study the code that you are overriding and try to match it as closely as possible other than adding support for the *.cloud.ibm.com stuff.
There was a problem hiding this comment.
Tests are already there those scenarios. Copied from the tests
def test_redirect_ibm_to_ibm_success():
url_from = 'http://region1.cloud.ibm.com/'
url_to = 'http://region2.cloud.ibm.com/'
...and
def test_redirect_not_ibm_to_not_ibm_fail():
url_from = 'http://region1.notcloud.ibm.com/'
url_to = 'http://region2.notcloud.ibm.com/'
...Are you talking about something else?
There was a problem hiding this comment.
Also, because you are completely overriding the default "follow redirects" behavior...
That's not the case. Still the requests package handles the redirections, the "allow_redirect": False" option only prevents to fire the next request automatically, but it's still being processed and resolved appropriately. From the session.py in the requests package:
# If redirects aren't being followed, store the response on the Request for Response.next().
if not allow_redirects:
try:
r._next = next(
self.resolve_redirects(r, request, yield_requests=True, **kwargs)
)
except StopIteration:
passLet's touch base later, to make sure we are on the same page! :)
There was a problem hiding this comment.
Are you talking about something else?
-
redirect to same host (both inside cloud.ibm.com):
from: region1.cloud.ibm.com
to: region1.cloud.ibm.com -
redirect to same host (both outside cloud.ibm.com):
from: region1.notcloud.ibm.com
to: region1.notcloud.ibm.com
Redirecting to the same host SHOULD allow the safe headers to be included in the redirected request, regardless of whether the host is inside or outside the cloud.ibm.com domain.
There was a problem hiding this comment.
Let's touch base later, to make sure we are on the same page!
Yes, let's discuss tomorrow.
There was a problem hiding this comment.
Redirecting to the same host SHOULD allow the safe headers to be included in the redirected request, regardless of whether the host is inside or outside the cloud.ibm.com domain.
Ok it's my bad, I used the domain and host words interchangeably(which is incorrect, I know :))...
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/support-redirect
branch
from
325e577
to
885dbfb
Compare
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/support-redirect
branch
from
b149648
to
ef1f6b5
Compare
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/support-redirect
branch
from
a3536b2
to
56242fe
Compare
|
@padamstx I've made the tests more robust, but it may be overkill...:) let me know what you think! |
This commit modifies the Python core so that it will include "safe" headers
when performing a cross-site redirect where both the original and redirected
hosts are within IBM's "cloud.ibm.com" domain.