rzo1 commented on code in PR #2128:
URL: https://github.com/apache/stormcrawler/pull/2128#discussion_r3944168015
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -466,9 +491,85 @@ public ProtocolResponse getProtocolOutput(String url,
final Metadata metadata)
final Request request = rb.build();
- final Call call = localClient.newCall(request);
+ /*
+ * Follow redirect responses manually: every target runs through the
+ * URL filters before the hop is taken, and a target which is rejected
+ * ends the chain - the redirect response is then returned as is and
+ * the caller handles it like it does when http.allow.redirects is
+ * off. The final URL is recorded in the response metadata so that
+ * callers can tell that the content is not from the URL they asked
+ * for.
+ */
+ Response lastResponse = null;
+ Call call = null;
+ Request currentRequest = request;
+ String currentUrl = url;
+
+ try {
+ for (int hops = 0; hops <= MAX_REDIRECT_HOPS; hops++) {
+ if (lastResponse != null) {
+ // release the connection before issuing the next request
+ lastResponse.close();
+ lastResponse = null;
+ }
+ call = client.newCall(currentRequest);
Review Comment:
This drops the proxy.
`main` uses `localClient` (line 380, rebuilt at 424 with the proxy and its
`proxyAuthenticator`). This loop calls the `client` field, so every fetch
bypasses the configured proxy.
```suggestion
call = localClient.newCall(currentRequest);
```
`localClient` will need hoisting or making effectively final; it is
currently reassigned in the proxy branch above.
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -466,9 +491,85 @@ public ProtocolResponse getProtocolOutput(String url,
final Metadata metadata)
final Request request = rb.build();
- final Call call = localClient.newCall(request);
+ /*
+ * Follow redirect responses manually: every target runs through the
+ * URL filters before the hop is taken, and a target which is rejected
+ * ends the chain - the redirect response is then returned as is and
+ * the caller handles it like it does when http.allow.redirects is
+ * off. The final URL is recorded in the response metadata so that
+ * callers can tell that the content is not from the URL they asked
+ * for.
+ */
+ Response lastResponse = null;
+ Call call = null;
+ Request currentRequest = request;
+ String currentUrl = url;
+
+ try {
+ for (int hops = 0; hops <= MAX_REDIRECT_HOPS; hops++) {
+ if (lastResponse != null) {
+ // release the connection before issuing the next request
+ lastResponse.close();
+ lastResponse = null;
+ }
+ call = client.newCall(currentRequest);
+ lastResponse = call.execute();
+
+ if (hops == MAX_REDIRECT_HOPS) {
+ LOG.warn("More than {} redirect hops for {}",
MAX_REDIRECT_HOPS, url);
+ break;
+ }
+ if (!followRedirects || !isRedirect(lastResponse)) {
+ break;
+ }
+
+ final String location =
lastResponse.header(HttpHeaders.LOCATION);
+ if (StringUtils.isBlank(location)) {
+ LOG.debug(
+ "Got redirect response {} for {} without location",
+ lastResponse.code(),
+ url);
+ break;
+ }
+
+ final HttpUrl target = currentRequest.url().resolve(location);
+ if (target == null) {
+ LOG.warn(
+ "Redirect target {} could not be resolved against
{}",
+ location,
+ currentUrl);
+ break;
+ }
- try (Response response = call.execute()) {
+ final Metadata sourceMetadata = metadata != null ? metadata :
new Metadata();
+ final String filtered =
+ urlFilters.filter(
+ currentRequest.url().url(), sourceMetadata,
target.toString());
+ if (filtered == null) {
+ LOG.info("Redirect target {} rejected by the URL filters",
target);
+ break;
+ }
+
+ final HttpUrl accepted = HttpUrl.parse(filtered);
+ if (accepted == null) {
+ LOG.warn("Filtered redirect target {} is not a URL",
filtered);
+ break;
+ }
+
+ final Request.Builder followBuilder =
currentRequest.newBuilder().url(accepted);
Review Comment:
`currentRequest.newBuilder().url(accepted)` carries every header, including
`Authorization`, `Cookie` and the custom credential headers, to whatever host
the redirect names.
okhttp's own follower strips `Authorization` on a host change; following
manually loses that. This is the credential-disclosure case named in the
security model, and it undoes what #2126 is establishing in the same release.
```suggestion
final Request.Builder followBuilder =
currentRequest.newBuilder().url(accepted);
final HttpUrl from = currentRequest.url();
if (!from.host().equals(accepted.host())
|| from.port() != accepted.port()
|| !from.scheme().equals(accepted.scheme())) {
// a redirect must not hand the credentials of one
origin to
// another, and must not carry them from https down to
http
followBuilder.removeHeader(HttpHeaders.AUTHORIZATION);
followBuilder.removeHeader("Cookie");
}
```
Related: nothing stops an `https:` to `http:` downgrade. okhttp has
`followSslRedirects` as a separate knob for exactly this. Refuse the hop, or at
least log it.
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -466,9 +491,85 @@ public ProtocolResponse getProtocolOutput(String url,
final Metadata metadata)
final Request request = rb.build();
- final Call call = localClient.newCall(request);
+ /*
+ * Follow redirect responses manually: every target runs through the
+ * URL filters before the hop is taken, and a target which is rejected
+ * ends the chain - the redirect response is then returned as is and
+ * the caller handles it like it does when http.allow.redirects is
+ * off. The final URL is recorded in the response metadata so that
+ * callers can tell that the content is not from the URL they asked
+ * for.
+ */
+ Response lastResponse = null;
+ Call call = null;
+ Request currentRequest = request;
+ String currentUrl = url;
+
+ try {
+ for (int hops = 0; hops <= MAX_REDIRECT_HOPS; hops++) {
+ if (lastResponse != null) {
+ // release the connection before issuing the next request
+ lastResponse.close();
+ lastResponse = null;
+ }
+ call = client.newCall(currentRequest);
+ lastResponse = call.execute();
+
+ if (hops == MAX_REDIRECT_HOPS) {
Review Comment:
This branch runs after `call.execute()`, so it logs "More than 5 redirect
hops" even when the sixth response is a plain 200 and the chain ended normally.
```suggestion
if (hops == MAX_REDIRECT_HOPS) {
if (followRedirects && isRedirect(lastResponse)) {
LOG.warn("More than {} redirect hops for {}",
MAX_REDIRECT_HOPS, url);
}
break;
}
```
--
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]
