Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update reqwest to 0.12 #1985

Closed
legoktm opened this issue May 2, 2024 · 3 comments · Fixed by #2015
Closed

Update reqwest to 0.12 #1985

legoktm opened this issue May 2, 2024 · 3 comments · Fixed by #2015
Labels

Comments

@legoktm
Copy link
Member

legoktm commented May 2, 2024

The current proxy v2 branch (#1718) has reqwest 0.11, we should update to 0.12 before releasing it. I don't want to do it in that branch since it's already so big.

@legoktm
Copy link
Member Author

legoktm commented May 17, 2024

When I upgrade to reqwest 0.12:

============================= test session starts ==============================
platform linux -- Python 3.11.9, pytest-7.4.4, pluggy-1.4.0
rootdir: /home/user/github/freedomofpress/securedrop-client/proxy
plugins: httpbin-2.0.0
collected 16 items                                                             

tests/test_errors.py .....F                                              [ 37%]
tests/test_proxy.py .....F....                                           [100%]

=================================== FAILURES ===================================
_____________________________ test_cannot_connect ______________________________

proxy_request = <function proxy_request.<locals>.proxy_ at 0x7a8b6f488720>

    def test_cannot_connect(proxy_request):
        test_input = {
            "method": "GET",
            "path_query": "/",
            "stream": False,
        }
        # .test is a reserved TLD, so it should never resolve
        result = proxy_request(input=test_input, origin="http://missing.test/foo")
        assert result.returncode == 1
>       assert (
            result.stderr.decode().strip()
            == '{"error":"error sending request for url (http://missing.test/): '
            + "error trying to connect: dns error: failed to lookup address information: "
            + 'Name or service not known"}'
        )
E       assert '{"error":"er...sing.test/)"}' == '{"error":"er...e not known"}'
E         Skipping 52 identical leading characters in diff, use -v to show
E         - ing.test/): error trying to connect: dns error: failed to lookup address information: Name or service not known"}
E         + ing.test/)"}

tests/test_errors.py:67: AssertionError
_________________________________ test_timeout _________________________________

proxy_request = <function proxy_request.<locals>.proxy_ at 0x7a8b6f489760>
httpbin = <pytest_httpbin.serve.Server object at 0x7a8b6f45b590>

    def test_timeout(proxy_request, httpbin):
        start = time.time()
        test_input = {"method": "GET", "path_query": "/delay/10", "stream": False, "timeout": 1}
        result = proxy_request(input=test_input)
        assert result.returncode == 1
>       assert (
            result.stderr.decode().strip()
            == '{"error":"error sending request for url (http://127.0.0.1:%s/delay/10): ' % httpbin.port
            + 'operation timed out"}'
        )
E       assert '{"error":"er...5/delay/10)"}' == '{"error":"er...n timed out"}'
E         Skipping 63 identical leading characters in diff, use -v to show
E         - /delay/10): operation timed out"}
E         + /delay/10)"}

tests/test_proxy.py:54: AssertionError
=========================== short test summary info ============================
FAILED tests/test_errors.py::test_cannot_connect - assert '{"error":"er...sing.test/)"}' == '{"error":"er...e not known"}'
FAILED tests/test_proxy.py::test_timeout - assert '{"error":"er...5/delay/10)"}' == '{"error":"er...n timed out"}'
======================== 2 failed, 14 passed in 17.60s =========================

This is fallout from seanmonstar/reqwest#2199 - it's unclear why the change was made, but we can probably keep the old behavior without too much difficulty:

diff --git a/proxy/src/main.rs b/proxy/src/main.rs
index d1cb6563..b6e04772 100644
--- a/proxy/src/main.rs
+++ b/proxy/src/main.rs
@@ -165,10 +165,12 @@ async fn main() -> ExitCode {
     match proxy().await {
         Ok(()) => ExitCode::SUCCESS,
         Err(err) => {
+            let mut error = err.to_string();
+            if let Some(source) = err.source() {
+                error = format!("{}: {}", error, source);
+            }
             // Try to serialize into our error format
-            let resp = ErrorResponse {
-                error: err.to_string(),
-            };
+            let resp = ErrorResponse { error };
             match serde_json::to_string(&resp) {
                 Ok(json) => {
                     // Print the error to stderr

legoktm added a commit that referenced this issue May 17, 2024
There's a small change in how reqwest errors are printed (see
<seanmonstar/reqwest#2199>), so handle that in
our code and update one test accordingly.

Import a number of audits and trust markers, a few audits will still be
needed.

Fixes #1985.
@legoktm legoktm mentioned this issue May 17, 2024
5 tasks
@legoktm
Copy link
Member Author

legoktm commented May 17, 2024

I'm not assigning this to myself because there's some Rust audits outstanding and those can be grabbed by others if there's interest (and happy to pair on them as well). Currently:

recommended audits for safe-to-run:
    Command                                    Publisher   Used By         Audit Size
    cargo vet diff atomic-waker 1.1.0 1.1.2    notgull     h2              5 files changed, 138 insertions(+), 3 deletions(-)
    cargo vet diff smallvec 1.11.1 1.13.2      mbrubeck    hyper           6 files changed, 108 insertions(+), 47 deletions(-)
      NOTE: mozilla trusts Matt Brubeck (mbrubeck) - consider cargo vet trust smallvec mbrubeck
    cargo vet diff tower-layer 0.3.1 0.3.2     davidpdrsn  tower           8 files changed, 390 insertions(+), 27 deletions(-)
    cargo vet diff rustls-pemfile 1.0.4 2.1.2  ctz         reqwest         13 files changed, 623 insertions(+), 241 deletions(-)
    cargo vet inspect rustls-pki-types 1.7.0   cpu         rustls-pemfile  3062 lines

estimated audit backlog: 4639 lines

@rocodes
Copy link
Contributor

rocodes commented Jun 3, 2024

Ah, thanks for offering up the chance to pair here, sorry to have missed it. Reviewing the version bump instead (and getting more familiar with cargo vet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants