pjfanning commented on PR #1456: URL: https://github.com/apache/poi/pull/1456#issuecomment-6349998688
Thanks - the fix looks right. `handleRedirect` copied every header, `Authorization` included, onto the redirect connection, so with `setFollowRedirects(true)` an https -> http redirect sent the Basic credentials in cleartext. The forwarding rule (same host with the same scheme and port, or an http -> https upgrade on default ports) is sound: in the upgrade case the credentials were already sent over plain http on the first request. Follow-up points: 1. **Subclasses that override `handleRedirect` no longer see the redirect hop.** Before, the redirect recursed into the protected, overridable `handleRedirect(String, MethodHandler, boolean)`. Now it goes to the new private 4-arg overload, so an override is bypassed for the redirect request. The class Javadoc says it is meant as a base for user code, so the 4-arg overload (the one with `sendAuth`) should be `protected`, keeping the 3-arg method delegating to it. 2. **Default ports aren't normalized (nit).** `https://h/` -> `https://h:443/` is treated as a different origin and the credentials are dropped. That fails safe, but mapping -1 to 80/443 by scheme before comparing would avoid the false negative. 3. **Relative `Location` headers fail (pre-existing, not from this PR).** A `Location` like `/tsp2` fails with an `IOException` in `new URI(url).toURL()`. Resolving it with `new URI(url).resolve(newUrl)` before following would fix that, and `isAuthForwardable` would then compare against the resolved URL. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
