OF-2970: Implement HTTP Cache-Control headers in the Admin Console#3254
OF-2970: Implement HTTP Cache-Control headers in the Admin Console#3254MilanTyagi2004 wants to merge 1 commit intoigniterealtime:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements centralized HTTP caching policy for the Openfire Admin Console by introducing a servlet filter that applies Cache-Control headers based on requested resource type (static vs dynamic), aiming to improve performance while preventing caching of sensitive admin pages.
Changes:
- Register a new
CacheControlFilterearly in the admin console filter chain (web.xml) and map it to all requests. - Add
org.jivesoftware.admin.CacheControlFilterto apply aggressive caching to static assets and no-store/no-cache headers to dynamic content.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| xmppserver/src/main/webapp/WEB-INF/web.xml | Registers and maps CacheControlFilter for all admin console requests. |
| xmppserver/src/main/java/org/jivesoftware/admin/CacheControlFilter.java | Implements extension-based caching rules for static vs dynamic resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Aggressive caching for static assets to improve performance and reduce server load. | ||
| // 'immutable' is used as UI assets are typically stable between version upgrades. | ||
| httpResponse.setHeader("Cache-Control", "public, max-age=31536000, immutable"); |
There was a problem hiding this comment.
Static resources are marked as max-age=31536000, immutable, but the admin console references assets with stable URLs (e.g. /style/global.css, /js/cookies.js in decorators/main.jsp) without any cache-busting. After an upgrade, browsers can keep serving stale CSS/JS for up to a year, potentially breaking the UI. Consider either adding explicit cache-busting to asset URLs (eg, append an Openfire version/build query param) or reducing the caching policy (shorter max-age and/or drop immutable).
| // Aggressive caching for static assets to improve performance and reduce server load. | |
| // 'immutable' is used as UI assets are typically stable between version upgrades. | |
| httpResponse.setHeader("Cache-Control", "public, max-age=31536000, immutable"); | |
| // Cache static assets briefly to improve performance, but avoid long-lived immutable | |
| // caching because admin console asset URLs are stable across upgrades. | |
| httpResponse.setHeader("Cache-Control", "public, max-age=3600"); |
| // Only set headers if not already present (preserving specific servlet/filter overrides) | ||
| if (!httpResponse.containsHeader("Cache-Control")) { | ||
| if (isStaticResource(uri)) { | ||
| // Aggressive caching for static assets to improve performance and reduce server load. | ||
| // 'immutable' is used as UI assets are typically stable between version upgrades. | ||
| httpResponse.setHeader("Cache-Control", "public, max-age=31536000, immutable"); | ||
| } else { | ||
| // Disable caching for sensitive administrative and dynamic content to ensure | ||
| // that data is never stored in browser history or proxy caches. | ||
| httpResponse.setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); | ||
| httpResponse.setHeader("Pragma", "no-cache"); | ||
| httpResponse.setDateHeader("Expires", 0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| chain.doFilter(request, response); | ||
| } |
There was a problem hiding this comment.
This sets cache headers before chain.doFilter(...), without knowing the eventual response status. If a static resource request results in a 404/500, the current logic can still attach a 1-year cache policy to that error response, causing clients/proxies to cache failures for a long time. Consider applying the caching headers after chain.doFilter(...) and only when the response status is cacheable (eg, 200/304) and the response is not committed, or use a response wrapper to capture status reliably.
| String uri = httpRequest.getRequestURI(); | ||
|
|
||
| // Strip query parameters for extension checking | ||
| int queryIndex = uri.indexOf('?'); | ||
| if (queryIndex != -1) { | ||
| uri = uri.substring(0, queryIndex); | ||
| } |
There was a problem hiding this comment.
The code attempts to strip query parameters from getRequestURI(), but the standard Servlet API exposes the query string separately (getQueryString()), and getRequestURI() typically does not contain ?…. As written, this logic is likely redundant and can be misleading. Consider removing it, or (if you really need query-aware classification) use getQueryString() / getRequestURL() explicitly and update the method javadoc accordingly.
| @Override | ||
| public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) | ||
| throws IOException, ServletException { | ||
|
|
||
| if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { | ||
| HttpServletRequest httpRequest = (HttpServletRequest) request; | ||
| HttpServletResponse httpResponse = (HttpServletResponse) response; | ||
|
|
||
| String uri = httpRequest.getRequestURI(); | ||
|
|
||
| // Strip query parameters for extension checking | ||
| int queryIndex = uri.indexOf('?'); | ||
| if (queryIndex != -1) { | ||
| uri = uri.substring(0, queryIndex); | ||
| } | ||
|
|
||
| // Only set headers if not already present (preserving specific servlet/filter overrides) | ||
| if (!httpResponse.containsHeader("Cache-Control")) { | ||
| if (isStaticResource(uri)) { | ||
| // Aggressive caching for static assets to improve performance and reduce server load. | ||
| // 'immutable' is used as UI assets are typically stable between version upgrades. | ||
| httpResponse.setHeader("Cache-Control", "public, max-age=31536000, immutable"); | ||
| } else { | ||
| // Disable caching for sensitive administrative and dynamic content to ensure | ||
| // that data is never stored in browser history or proxy caches. | ||
| httpResponse.setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); | ||
| httpResponse.setHeader("Pragma", "no-cache"); | ||
| httpResponse.setDateHeader("Expires", 0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| chain.doFilter(request, response); | ||
| } |
There was a problem hiding this comment.
New caching behavior is introduced here, but there are no unit tests to validate the classification (static vs dynamic) and header behavior (eg, respects pre-existing Cache-Control, adds Pragma/Expires for dynamic). There are existing tests for other admin filters (see AuthCheckFilterTest), so adding targeted tests for this filter would help prevent regressions.
guusdk
left a comment
guusdk
left a comment
There was a problem hiding this comment.
Hi @MilanTyagi2004 - thanks for another contribution! This looks pretty good already, but I think it can be improved a bit further.
As I've also commented on a few other of your PRs: please make sure that the commit message(s) contain a good description of why the commit was added. The text of a commit message becomes part of the history of the project. Pull Request descriptions are not.
Please review the review comments made by Copilot. That contains good hints.
I don't think it's quite necessary to have a 'cache busting' mechanism if the cache duration is not 'forever'. Consider creating a default cache timeout of an hour or so. That will give a good balance between 'most requests will be answered from cache' and 'faulty cached objects linger forever'.
Please introduce two SystemProperties, to make the behavior of this new implementation configurable:
- A property that defines the set of file extensions to which caching is applied.
- Another property that is used to define the Duration of the cache
|
Sure Guus, |
Jira Issue: OF-2970
Problem Statement
The Openfire Admin Console currently lacks consistent HTTP Cache-Control headers. This leads to:
Performance Degradation: Browsers frequently re-request stable static assets (JS, CSS, images), increasing latency and server load.
Security Risks: Sensitive administrative pages and session-specific layouts can be stored in browser history or intermediary proxy caches.
Solution Overview
Key Improvements
Robust Implementation:
Verification Results
Dynamic Response (index.jsp)
Cache-Control: no-cache, no-store, must-revalidate
Pragma: no-cache
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Static Resource (logo.png)
Cache-Control: public, max-age=31536000, immutable