Polish

Author: Pijukatel

impit-python/src/cookies.rs

@@ -91,13 +91,7 @@ impl CookieStore for PythonCookieJar {
 
                 kwargs.set_item("rest", rest).unwrap_or_default();
 
-                // A hostile or buggy server can send a `Set-Cookie` that
-                // `http.cookiejar.Cookie` rejects (e.g. a bad domain/expiry), and a custom
-                // cookie jar's `set_cookie` may raise too. Skip the offending cookie instead
-                // of `.unwrap()`ing: a panic here would unwind across the FFI boundary and
-                // abort the host process rather than raise a catchable Python exception.
-                // Cookie parsing errors are ignored silently, matching the Node binding's
-                // `setCookie` handling in `index.wrapper.js`.
+                // Cookie parsing errors are ignored silently.
                 let py_cookie = match self.cookie_constructor.call(py, (), Some(&kwargs)) {
                     Ok(py_cookie) => py_cookie,
                     Err(_) => continue,

impit-python/test/async_client_test.py

@@ -193,25 +193,18 @@ async def test_complex_cookies(self, browser: Browser) -> None:
                 assert cookie.domain == '127.0.0.1'
 
     @pytest.mark.asyncio
-    async def test_malformed_cookie_is_skipped_not_crashing(self, browser: Browser) -> None:
-        """A cookie the jar rejects is skipped instead of aborting the process.
-
-        Regression test for https://github.com/apify/impit/issues/478: `set_cookies`
-        runs inside reqwest's cookie-store callback, so a raising `set_cookie` used to
-        `.unwrap()` into a Rust panic that unwound across the FFI boundary and aborted
-        the host process. It must now be caught and the offending cookie skipped, while
-        valid cookies in the same response are still stored.
-        """
+    async def test_rejected_cookie_is_skipped_not_crashing(self, browser: Browser) -> None:
+        """A cookie jar rejects are skipped instead of aborting the process."""
 
         class RejectingCookieJar(CookieJar):
             def set_cookie(self, cookie: Cookie) -> None:
                 if cookie.name == 'bad':
-                    raise ValueError('cookie jar rejects this cookie')
+                    raise ValueError('simulate parsing error')
                 super().set_cookie(cookie)
 
         cookies_jar = RejectingCookieJar()
 
-        impit = AsyncClient(browser=browser, cookie_jar=cookies_jar, follow_redirects=True)
+        impit = AsyncClient(browser=browser, cookie_jar=RejectingCookieJar(), follow_redirects=True)
 
         url = get_httpbin_url(
             '/response-headers',
@@ -223,13 +216,10 @@ def set_cookie(self, cookie: Cookie) -> None:
             },
         )
 
-        # The request must return normally rather than aborting the interpreter.
         response = await impit.get(url)
         assert response.status_code == 200
 
-        names = {cookie.name for cookie in cookies_jar}
-        assert 'bad' not in names  # the rejected cookie was skipped
-        assert 'good' in names  # a valid cookie in the same response is still stored
+        assert {'good'} == {cookie.name for cookie in cookies_jar}
 
     @pytest.mark.asyncio
     async def test_cookie_jar_works(self, browser: Browser) -> None:

impit-python/test/basic_client_test.py

@@ -203,25 +203,18 @@ def test_complex_cookies(self, browser: Browser) -> None:
                 # but it's ok - https://www.rfc-editor.org/rfc/rfc6265#section-4.1.2.3
                 assert cookie.domain == '127.0.0.1'
 
-    def test_malformed_cookie_is_skipped_not_crashing(self, browser: Browser) -> None:
-        """A cookie the jar rejects is skipped instead of aborting the process.
-
-        Regression test for https://github.com/apify/impit/issues/478: `set_cookies`
-        runs inside reqwest's cookie-store callback, so a raising `set_cookie` used to
-        `.unwrap()` into a Rust panic that unwound across the FFI boundary and aborted
-        the host process. It must now be caught and the offending cookie skipped, while
-        valid cookies in the same response are still stored.
-        """
+    def test_rejected_cookie_is_skipped_not_crashing(self, browser: Browser) -> None:
+        """A cookie jar rejects are skipped instead of aborting the process."""
 
         class RejectingCookieJar(CookieJar):
             def set_cookie(self, cookie: Cookie) -> None:
                 if cookie.name == 'bad':
-                    raise ValueError('cookie jar rejects this cookie')
+                    raise ValueError('simulate parsing error')
                 super().set_cookie(cookie)
 
         cookies_jar = RejectingCookieJar()
 
-        impit = Client(browser=browser, cookie_jar=cookies_jar, follow_redirects=True)
+        impit = Client(browser=browser, cookie_jar=RejectingCookieJar(), follow_redirects=True)
 
         url = get_httpbin_url(
             '/response-headers',
@@ -233,13 +226,10 @@ def set_cookie(self, cookie: Cookie) -> None:
             },
         )
 
-        # The request must return normally rather than aborting the interpreter.
         response = impit.get(url)
         assert response.status_code == 200
 
-        names = {cookie.name for cookie in cookies_jar}
-        assert 'bad' not in names  # the rejected cookie was skipped
-        assert 'good' in names  # a valid cookie in the same response is still stored
+        assert {'good'} == {cookie.name for cookie in cookies_jar}
 
     def test_cookie_jar_works(self, browser: Browser) -> None:
         cookies = Cookies({'preset-cookie': '123'})