spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.5k stars 5.78k forks source link

Regression Cache-control headers are being ignored #4307

Closed czubin closed 5 years ago

czubin commented 7 years ago

The cache control headers, if provided by the controller are being ignored. The original issue and fix: #2953 This regression seems to be caused by reversal: #3975

Now it's very unclear what a developer should do. Either the developers adds a workaround or disables the headerWriterFilter globally. Neither is preferable.

winterbe commented 7 years ago

I've encountered the same issue, using Spring Boot 1.5.2.RELEASE with Spring Security 4.2.2.

When I set Cache Control in ResponseEntity inside a controller, it gets overridden by CacheControlHeadersWriter from Spring Security. Disabling this feature solves the problem but then it's off for all controllers.

CacheControlHeadersWriter should only be active if no CacheControl was explicitly specified in user code.

zutnop commented 7 years ago

Same problem. Upgrading from 4.1.0 to 4.2.2 broke this for our app (running on tomcat as a normal war, spring framework 4.3.7).

For example, user profile controller returns dynamic user profile picture and sets .cacheControl(CacheControl.maxAge(24, TimeUnit.HOURS)) for the ResponseEntity, but this is now ignored by Spring Security and every user request re-downloads the user profile picture :(

nealeu commented 7 years ago

@rwinch Any chance of a comment on this? I do want security and do want caching.

Clearly this did work at some point for http://www.baeldung.com/spring-security-cache-control-headers, which suggests exactly as @zutnop is doing.

rwinch commented 7 years ago

The code was reverted because there are no official on response committed hooks with the Servlet Container and it was deemed to fragile to continue using this method.

I'm going to have to take some time and see how best to resolve this.

nealeu commented 7 years ago

Thanks for the update Rob. Bit of a nightmare, but important to try to resolve for a good out of the box experience. The workaround in #2953 is okay, but developer joy would be something like:

imgx64 commented 7 years ago

@rwinch Can you elaborate on why the OnCommittedResponseWrapper is fragile? In #3975, no one provided a test case which showed the issue, but the fix was reverted anyway. Is OnCommittedResponseWrapper not good enough for what it does? (If it isn't, I think there should be a documentation warning on it).

nealeu commented 6 years ago

@rwinch This is still causing significant pain. The behaviour is just non-obvious, and for expected behaviour we have to put a hack in. Is there any out of the box developer joy planned here?

nealeu commented 6 years ago

Rob. To add to that, I'm currently spending days migrating from XML config to Java Config in order to be able to apply the workaround (which in itself causes maintenance issues and testability issues). Forking and reverting is now crossing my mind. I actually want to be delivering customer value on a somewhat legacy project, but this is causing a nightmare.

czubin commented 6 years ago

@nealeu you van override the offending class. Just add the same class to your module with the fix. The classloader should pick it up.

That way you do not need tot migrate to javaconfig and apply the workaround.

Adding a test to check the cache headers is always a good idea

nealeu commented 6 years ago

Hi @czubin I've ended up with the workaround as per my comment on the earlier issue: https://github.com/spring-projects/spring-security/issues/2953#issuecomment-335443527.

I take it from your comment that you're suggesting replacing the default Spring bean. Perhaps you can give an example if it differs from the workarounds in #2953.

czubin commented 6 years ago

@nealeu it's a dirty trick. Thinking about it makes it seems like a bad idea but it's quick fix.

When you duplicate the same class and modify it to include the fix (see reversed code). For a webapp(e.g. Tomcat webapp) the classloader will look to your webapp classes before searching the spring jar's. Note: use this with extreme caution!

The cleaner solution would be to use your or above workaround.

nealeu commented 6 years ago

Yes, that is! There's plenty of things to fall foul of, including weird app server classloader situations such as Websphere allowing the order to be reversed (although that's between the server and the app, not within the app jars).

geeksven-dev commented 6 years ago

I have the same issue Using Spring Boot 1.5.6 and Spring Security 4.2.3. I was wondering if I am just not smart enough to follow a simple tutorial. But it seems broken.

ResponseEntity.ok().cacheControl(CacheControl.maxAge(900, TimeUnit.SECONDS).cachePrivate().mustRevalidate()).lastModified(lastModified.getTime()).body(bootstrap);

For now I just disabled default headers with

http.headers().disable();

But I guess that's no solution for the future.

FrontierPsychiatrist commented 6 years ago

Encountered the same today - using the .cacheControl method on ResponseEntity is ignored by the CacheControlHeadersWriter. This is confusing and unexpected.

As a workaround, without disabling all headers from Spring Security I set the headers on the raw HttpServletResponse in the controller.

public ResponseEntity<String> handle(HttpServletResponse response) {
    response.setHeader("Cache-Control", "public, max-age=1800");
    return ResponseEntity.ok().body("foo");
}
pavelety commented 6 years ago

Spring Boot 1.5.9 (Spring Security 4.2.3) Chrome sends revalidation request with @FrontierPsychiatrist workaround. To make Chrome completely cache resource:

@GetMapping("foo")
public ResponseEntity<String> getAll(HttpServletResponse response) {
    response.setHeader(HttpHeaders.CACHE_CONTROL, "public, max-age=" + TimeUnit.DAYS.toSeconds(365));
    response.setHeader(HttpHeaders.PRAGMA, null);
    return ResponseEntity.ok().body("foo");
}
sbley commented 6 years ago

CacheControlHeadersWriter is actually not ignoring the cache-control header from ResponseEntity.cacheControl(), but it can not find it in the response:

// CacheControlHeadersWriter.java

@Override
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
    if (hasHeader(response, CACHE_CONTROL) || hasHeader(response, EXPIRES) 
            || hasHeader(response, PRAGMA)) {
        return; // should go here, but doesn't
    }
    this.delegate.writeHeaders(request, response);
}

When I debugged the workflow the CacheControlHeadersWriter seemed to be executed before .cacheControl(). Weird...

stefanocke commented 6 years ago

@sbley : Probably not so weird, since the code revert in #3975 (mentioned by @czubin and @rwinch ) has changed HeaderWriterFilter like this:

                for (HeaderWriter headerWriter : headerWriters) {
            headerWriter.writeHeaders(request, response);
        }
        filterChain.doFilter(request, response);

So IMHO the controller ist called after the headers are written...

stefanocke commented 6 years ago

@pavelety , the workaround response.setHeader(HttpHeaders.PRAGMA, null); does not work for me. It does not delete the already set pragma header. Looking at org.apache.catalina.connector.Response.setHeader(String, String) there is:

        if (name == null || name.length() == 0 || value == null) {
            return;
        }

I tried empty string instead of null. It works in Chrome and IE, but it is probably against HTTP spec and may fail with other browsers or other servlet containers than Tomcat (which I currently use).

stefanocke commented 6 years ago

Good news ? ... In Spring Security 5.0.2 it is fixed by reverting the code revert: #5004

rwinch commented 5 years ago

I'm closing this issue as no samples have been produced. Anyone experiencing an issue should create a new issue and put together a sample using the latest version of Spring Security. Without a sample it is difficult if not impossible to solve the issue (which with these header issues are often subtle).

Be aware of https://github.com/spring-projects/spring-security/pull/6509 which eagerly writes the headers.

jsm174 commented 5 years ago

@rwinch Apologizes for the comment. Forgot to update it.

I was using a system with zuul which was adding these headers back.

(https://github.com/spring-cloud/spring-cloud-netflix/issues/2799)

rwinch commented 5 years ago

@jsm174 I'm not sure I understand. It sounds as though Spring Cloud was what was adding the headers vs Spring Security?

If there is an actual issue, please open a new issue with all the details (including sample) to reproduce.

jsm174 commented 5 years ago

Correct. There is no issue @rwinch . Thanks.