Skip to content

fix(http): keep proxy credentials out of HTTP crawler errors - #2361

Merged
ntohidi merged 1 commit into
unclecode:developfrom
alkaz-nodemaven:fix/redact-proxy-credentials-in-http-errors
Oct 12, 2026
Merged

ntohidi merged 1 commit into
unclecode:developfrom
alkaz-nodemaven:fix/redact-proxy-credentials-in-http-errors

Conversation

@alkaz-nodemaven

Copy link
Copy Markdown
Contributor

Summary

Refs #2326: the separate step mentioned there, hiding credentials in the wrapped error text. It does not touch _format_proxy_url, so it is independent of #2327 (checked: the two merge cleanly and both test files pass together).

_handle_http re-raised aiohttp errors as HTTPCrawlerError / ConnectionError / ConnectionTimeoutError with str(e) in the message. InvalidURL's message is the whole proxy URL, so the password ended up in the error text, and also in the traceback through the chained original exception.

Now every re-raise goes through _wrap_error, which masks user:pass@ in the message (the exact proxy userinfo first, so a password with / or # is caught whole, then any scheme://user:pass@ pattern). When the original message carried credentials, its context is suppressed so the traceback does not print it under "During handling of the above exception". Errors without credentials keep their context as before.

List of files changed and why

  • crawl4ai/async_crawler_strategy.py: _redact_proxy_credentials and _wrap_error; the five re-raises in _handle_http use _wrap_error.
  • tests/test_http_proxy_error_redaction_2326.py: unit tests for both helpers, plus _handle_http against a closed local proxy port with passwords s3cret, s3cret#pw, pa/ss, asserting the password is in neither the message nor the formatted traceback.

How Has This Been Tested?

  • New tests: 14 passed. tests/test_http_timeout_unit_1894.py and tests/general/test_http_crawler_strategy.py still pass.
  • On develop without the change, the same _handle_http calls leak s3cret#pw and pa/ss in both message and traceback; with it, neither does.
  • Merged locally with Percent-encode proxy credentials in the HTTP crawler proxy URL #2327: its test and these pass together.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - no documented behaviour changes
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

🤖 Generated with Claude Code

Errors raised from AsyncHTTPCrawlerStrategy._handle_http quoted the
underlying aiohttp message, and InvalidURL's message is the whole proxy
URL with the password in it. Mask user:pass@ in the wrapped message, and
when the original carried credentials keep it out of the traceback.

Refs unclecode#2326

@ntohidi ntohidi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alkaz-nodemaven for this, and for checking it together with #2327.

What I verified

  • Tests: merged with current develop (which now includes #2327), test_http_proxy_error_redaction_2326.py, tests/proxy/test_http_proxy_url_encoding.py and test_http_timeout_unit_1894.py pass together (29).
  • No regressions: tests/unit, tests/proxy and tests/general/test_http_crawler_strategy.py show no new failures against develop.
  • Still needed after #2327: with #2327, the # and / passwords no longer cause InvalidURL, so _handle_http no longer leaks them on develop. But a malformed proxy server (bad host or port) still raises InvalidURL carrying the full URL with the password, and this PR masks it. It's good defense in depth.
  • proxy_url is set before the try, so every handler can use it safely.

Small note (not blocking)

The test file would fit better in tests/proxy/, next to the #2327 test.

Approving. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants