abhinav-phi commented on PR #2126: URL: https://github.com/apache/stormcrawler/pull/2126#issuecomment-5574003021
Thanks for the review — pushed a5d7ba15 addressing all four points: - **Cleartext HTTP:** the send-credentials decision is now made per request (`credentialsAllowed(url)`): a server is only considered authenticated when the connection is https:// *and* the certificate chain validates. Cleartext http:// requests no longer receive the Authorization header, credential headers or cookies. Two new WireMock tests cover the withheld and opt-in paths over http://. - **Denylist:** the header names are now configurable via `http.credentials.headers` (default unchanged: `authorization, proxy-authorization, cookie, x-api-key`), so an operator can add site-specific names like `x-auth-token` — kept the denylist approach as the smaller change, per your second option. - **Cookie warning:** moved below the emptiness check, so it only fires when a cookie was actually going to be sent. - **Proxy:** comment added to the proxy authenticator saying its credentials are deliberately unaffected — the proxy is not the crawled server. - **Keystores:** the keytool commands that generated them are recorded in a comment in the test class. Also switched the TLS test server to WireMock (JDK's `com.sun.net.httpserver` was rejected by the forbiddenapis check on CI) — that was the red rat job, not formatting. On the upgrade note: I'll add the combined release-notes paragraph for the six default-changing PRs once this batch is approved — happy to carry it in #2124 (file scheme) since that one is the most breaking, unless you prefer a dedicated wiki/doc page. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
