abhinav-phi commented on code in PR #2128:
URL: https://github.com/apache/stormcrawler/pull/2128#discussion_r3996893282
##########
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:
Fixed — the hop loop now uses the same client that served the first request
(hoisted as an effectively-final `fetchClient`, HttpProtocol.java:712, used at
:724), so the configured dynamic proxy and its `proxyAuthenticator` apply on
every hop. (The branch has since been rebased onto current main and squashed to
the single commit ea452dda, which contains the fix.)
##########
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:
Fixed, and aligned with #2126 as requested: #2126 merged on 09-10 and the
branch was rebased onto it, so hops now use the merged
`http.credentials.headers` classification and `credentialsAllowed` transport
policy directly — one source, no duplication. On a host change or an
unauthenticated target every credential header is stripped before the next
request (ea452dda:785). `credentialsAreStrippedOnACrossOriginHop` now exercises
a true cross-origin chain and asserts neither `Authorization` nor `X-Api-Key`
reaches the second origin; `credentialsSurviveASameOriginHop` covers the
same-origin case.
##########
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:
Fixed — the warning now fires only when the hop budget is exhausted *and*
the last response is still a redirect. The budget itself is configurable via
`http.allow.redirects.max` (default 5).
--
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]
