Limit number of HTTP redirects to 10 #1420
Conversation
Zee314159
force-pushed
the
Fix/ota-3729/Limit-number-of-HTTP-redirects
branch
from
93525bc
to
0a6b164
Compare
src/libaktualizr/http/httpclient.cc
Outdated
| @@ -53,6 +53,9 @@ HttpClient::HttpClient(const std::vector<std::string>* extra_headers) { | |||
|
|
|||
| curlEasySetoptWrapper(curl, CURLOPT_VERBOSE, get_curlopt_verbose()); | |||
|
|
|||
| curlEasySetoptWrapper(curl, CURLOPT_FOLLOWLOCATION, 1L); | |||
| curlEasySetoptWrapper(curl, CURLOPT_MAXREDIRS, 10L); | |||
There was a problem hiding this comment.
This curl param should be set in HttpClient::downloadAsync(). But won't work anyway because aktualizr/package_manager tries to download it three times.
| install_result = director.wait_for_install() | ||
| return not install_result | ||
|
|
||
|
|
There was a problem hiding this comment.
The test should look like this
"""
Verifies if aktualizr rejects redirects over 10 times - update fails after redirect
"""
@with_uptane_backend(start_generic_server=True)
@with_customrepo(handlers=[
RedirectHandler(number_of_redirects=(11 * 3 + 1), url='/primary-image.img')
])
@with_imagerepo()
@with_director()
@with_aktualizr(start=False, run_mode='once', output_logs=True)
def test_backend_failure_sanity_customrepo_unsuccessful_update_redirect(aktualizr, uptane_repo,
custom_repo, director, **kwargs):
update_hash = uptane_repo.add_image(aktualizr.id, 'primary-image.img',
custom_url=custom_repo.base_url + '/' + 'primary-image.img')
with aktualizr:
aktualizr.wait_for_completion()
return not director.get_install_result()
A number of requests should be equal to (CURL_MAX_REDIRECTS + 1) * (RETRY_NUMB) + 1, which is in this specific case is (10+1)*3 + 1 (An initial request to the mock repo server is not considered as a redirect request by curl so it starts counting only from the second request, hence +1).
There was a problem hiding this comment.
Thanks, @mike-sul . I'll have a try.
There was a problem hiding this comment.
@mike-sul , the reason of using aktualizr.wait_for_completion() instand of director.wait_for_install() is "limitation of a redirect number should be aktualizr-wide and applied to each outgoing request.", right?
There was a problem hiding this comment.
This is a negative test, an installation of a new update is not supposed to happen since a number of redirects exceeds the maxim allowed. Therefore, director.wait_for_install() will never succeed (installation should not happen) and exit with a timeout error. So, the test just simply wait until Aktualizr completes an uptane cycle.
Zee314159
force-pushed
the
Fix/ota-3729/Limit-number-of-HTTP-redirects
branch
from
0a6b164
to
5484a45
Compare
Codecov Report
@@ Coverage Diff @@
## master #1420 +/- ##
==========================================
+ Coverage 80.4% 80.42% +0.02%
==========================================
Files 181 181
Lines 10648 10649 +1
==========================================
+ Hits 8561 8564 +3
+ Misses 2087 2085 -2
Continue to review full report at Codecov.
|
src/libaktualizr/http/httpclient.cc
Outdated
| @@ -241,6 +241,8 @@ std::future<HttpResponse> HttpClient::downloadAsync(const std::string& url, curl | |||
| curlEasySetoptWrapper(curl_download, CURLOPT_LOW_SPEED_TIME, speed_limit_time_interval_); | |||
| curlEasySetoptWrapper(curl_download, CURLOPT_LOW_SPEED_LIMIT, speed_limit_bytes_per_sec_); | |||
| curlEasySetoptWrapper(curl_download, CURLOPT_RESUME_FROM_LARGE, from); | |||
| curlEasySetoptWrapper(curl_download, CURLOPT_FOLLOWLOCATION, 1L); | |||
There was a problem hiding this comment.
It's redundant, see line # 232.
|
Just a nitpick, but could you change the commit message to "Limit number of HTTP redirects to 10". Just saves someone from having to read the PR so closely. |
Signed-off-by: Zee314159 <252806294@qq.com>
Zee314159
force-pushed
the
Fix/ota-3729/Limit-number-of-HTTP-redirects
branch
from
5484a45
to
7cf5b04
Compare
Zee314159
changed the title
Draft code