Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect http response for a request using the Range header for pre-compressed resources #25976

Closed
180254 opened this issue Oct 25, 2020 · 3 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@180254
Copy link

180254 commented Oct 25, 2020

What happened:
I use serving pre-compressed resources (spring.resources.chain.compressed), index.html is also compressed.
I found that Facebook Sharing Debugger (https://developers.facebook.com/tools/debug/) doesn't work for my website.

The Facebook Crawler (https://developers.facebook.com/docs/sharing/webmasters/crawler) sends the following request:

"request" : {
  "method" : "GET",
  "uri" : "https://example.com",
  "headers" : {
    "host" : [ "example.com" ],
    "range" : [ "bytes=0-524287" ],
    "x-forwarded-for" : [ "0.0.0.0" ],
    "accept-encoding" : [ "deflate, gzip" ],
    "accept" : [ "*/*" ],
    "user-agent" : [ "facebookexternalhit/1.1 (+http://www.facebook.com/externalhit_uatext.php)" ]
  },
  "remoteAddress" : null
},

Spring boot serves pre-compressed file (index.html.gz), response has the following headers:

"response" : {
  "status" : 206,
  "headers" : {
    "Accept-Ranges" : [ "bytes" ],
    "Content-Range" : [ "bytes 0-177/178" ],
    "Connection" : [ "close" ],
    "Vary" : [ "Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers" ],
    "Last-Modified" : [ "Sun, 25 Oct 2020 20:36:37 GMT" ],
    "Content-Length" : [ "178" ],
    "Content-Language" : [ "en-US" ],
    "Date" : [ "Sun, 25 Oct 2020 20:36:46 GMT" ],
    "Content-Type" : [ "text/html;charset=UTF-8" ]
  }
},

In the above scenario server serves a pre-compressed resource, but does not inform about used compression.

What you expected to happen:
The following headers are included in the response:

Vary: Accept-Encoding
Content-Encoding: gzip

How to reproduce it:
Reproduction-steps code: https://github.com/180254/spring-boot-issue-23830

[n] means "terminal number n"
[1] $ mvn clean package
[1] $ java -jar target/demo-0.0.1-SNAPSHOT.jar
[2] $ curl -v -H "Accept-Encoding: gzip" -H "Range: bytes=0-1000" -H "Connection: close" -A "facebookexternalhit/1.1 (+http://www.facebook.com/externalhit_uatext.php)" "http://localhost:8080/" -o outputfile

Pay attention to the request&response headers in the curl log and/or visit http://localhost:8080/actuator/httptrace to see http trace.

Check if compression was requested, check if information about compression exist in response. Check if "outputfile" contains compressed or uncompressed data.

Reproducing initial problem: Use example above, run it at any public address and use Facebook Sharing Debugger. Facebook Sharing Debugger uses the compressed bytes as a final response. There is no info from server the data should be uncompressed beforehand.

Anything else we need to know?:
I tried to use the following embedded servers: Tomcat, Jetty, Undertow. There is the same problem for each of them.

Environment:
Spring Boot 2.3.4.RELEASE
Apache Tomcat/9.0.38
Apache Maven 3.6.3
Java version: 11.0.9, vendor: AdoptOpenJDK, runtime: /usr/lib/jvm/adoptopenjdk-11-hotspot-amd64

@wilkinsona
Copy link
Member

wilkinsona commented Oct 26, 2020

Thanks very much for the sample and detailed reproduction steps. I've reproduced the problem and, as far as I can tell, it's due to the following logic in Spring Framework's ResourceHttpRequestHandler:

if (request.getHeader(HttpHeaders.RANGE) == null) {
    Assert.state(this.resourceHttpMessageConverter != null, "Not initialized");
    setHeaders(response, resource, mediaType);
    this.resourceHttpMessageConverter.write(resource, mediaType, outputMessage);
}
else {
    Assert.state(this.resourceRegionHttpMessageConverter != null, "Not initialized");
    response.setHeader(HttpHeaders.ACCEPT_RANGES, "bytes");
    ServletServerHttpRequest inputMessage = new ServletServerHttpRequest(request);
    try {
        List<HttpRange> httpRanges = inputMessage.getHeaders().getRange();
        response.setStatus(HttpServletResponse.SC_PARTIAL_CONTENT);
        this.resourceRegionHttpMessageConverter.write(
                HttpRange.toResourceRegions(httpRanges, resource), mediaType, outputMessage);
    }
    catch (IllegalArgumentException ex) {
        response.setHeader("Content-Range", "bytes */" + resource.contentLength());
        response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
    }
}

It's the call to setHeaders that allows the resource that's being served to contribute to the response headers. As you can see, it's only called for requests with no Range header.

I'm not sure if this is intentional or an oversight. We'll transfer this to the Framework team so that they can take a look.

@philwebb philwebb transferred this issue from spring-projects/spring-boot Oct 26, 2020
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 26, 2020
@180254
Copy link
Author

180254 commented Oct 26, 2020

Thanks.

I looked into the class that you indicated. I think there is one more problem in this part of the code. Let me mention it here.

// Content phase
if (METHOD_HEAD.equals(request.getMethod())) {
	setHeaders(response, resource, mediaType);
	return;
}

ServletServerHttpResponse outputMessage = new ServletServerHttpResponse(response);
if (request.getHeader(HttpHeaders.RANGE) == null) {
	...
}
else {
	...
}

HEAD is handled too early. GET headers vary depending on whether the range was used or not. HEAD always sends headers for non-Range queries.

@rstoyanchev rstoyanchev self-assigned this Oct 27, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 27, 2020
@rstoyanchev rstoyanchev added this to the 5.2.10 milestone Oct 27, 2020
@rstoyanchev
Copy link
Contributor

I don't think there is a specific reason for the current behavior. It was that way from the start and it looks like an oversight. By comparison WebFlux always calls setHeaders but it does have the same issue with HTTP HEAD.

rstoyanchev added a commit that referenced this issue Oct 27, 2020
Allow the body to be written in order for all headers to be set
as they would be on HTTP GET. The body content is ignored as a
lower level.

See gh-25976
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x labels Oct 27, 2020
rstoyanchev added a commit that referenced this issue Oct 27, 2020
Allow the body to be written in order for all headers to be set
as they would be on HTTP GET. The body content is ignored as a
lower level.

See gh-25976
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
Allow the body to be written in order for all headers to be set
as they would be on HTTP GET. The body content is ignored as a
lower level.

See spring-projectsgh-25976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants