feat: set X-Frame-Options header by default#24525
Conversation
|
|
|
|
||
| String frameOptions = config.getFrameOptions(); | ||
| if (frameOptions != null && !frameOptions.isEmpty()) { | ||
| response.setHeader("X-Frame-Options", frameOptions); |
There was a problem hiding this comment.
IMHO this should only be done if no X-Frame-Options are already present
There was a problem hiding this comment.
Good point — updated so the header is only set when the response doesn't already contain an X-Frame-Options header (via response.containsHeader(...)), letting an app-set or filter-set value take precedence. Added a test covering the already-present case. Thanks!
There was a problem hiding this comment.
Should log a warning if a non-default setting is ignored due to an existing header
There was a problem hiding this comment.
Done — when the X-Frame-Options header is already present and an explicit frameOptions value was configured, the handler now logs a warning that the configured value is being ignored (the default, unconfigured case stays silent).
There was a problem hiding this comment.
This logic should go only after service.modifyIndexHtmlResponse(indexHtmlResponse) so that the application has a chance of setting their own custom header before we check if there's already a header.
There was a problem hiding this comment.
Though on the other hand, the whole point of modifyIndexHtmlResponse is that the application can override whatever the framework has set. So maybe it's still better to have it before modifyIndexHtmlResponse.
There was a problem hiding this comment.
Agreed — and that's already how it's placed: the header is set here (before modifyIndexHtmlResponse), so an IndexHtmlRequestListener can still override it via getVaadinResponse().setHeader("X-Frame-Options", ...) (which replaces the framework default). The containsHeader guard only defers to a value set even earlier, e.g. by a servlet filter. So the framework sets a sensible default, the app can override, and an external pre-set header is respected. Happy to move it after modifyIndexHtmlResponse instead if you'd prefer listener-set headers to win over the configured value without a warning — let me know.
There was a problem hiding this comment.
Should log a warning if a non-default setting is ignored due to an existing header
While I understand the intention.. this might result in a lot of noise.. some tools do things like "This is logged once at WARN and afterwards at DEBUG" so that the log is not poluted
There was a problem hiding this comment.
The point is that it's logging only if you have set the configuration property and also set a custom header. You get rid of the annoying logging by clearing the configuration property back to the default value.
|
|
||
| String frameOptions = config.getFrameOptions(); | ||
| if (frameOptions != null && !frameOptions.isEmpty()) { | ||
| response.setHeader("X-Frame-Options", frameOptions); |
There was a problem hiding this comment.
Should log a warning if a non-default setting is ignored due to an existing header
|
|
||
| String frameOptions = config.getFrameOptions(); | ||
| if (frameOptions != null && !frameOptions.isEmpty()) { | ||
| response.setHeader("X-Frame-Options", frameOptions); |
There was a problem hiding this comment.
This logic should go only after service.modifyIndexHtmlResponse(indexHtmlResponse) so that the application has a chance of setting their own custom header before we check if there's already a header.
Send the X-Frame-Options HTTP response header with a default value of SAMEORIGIN for the application page so browsers opt in to clickjacking protection out of the box. The value is configurable through the new frameOptions init parameter (DeploymentConfiguration#getFrameOptions); an empty value disables the header for applications meant to be embedded in a frame. Fixes #23415
Artur-
force-pushed
the
feat/default-x-frame-options
branch
from
e52b919 to
94f5671
Compare
|
2 participants
Send the X-Frame-Options HTTP response header with a default value of
SAMEORIGIN for the application page so browsers opt in to clickjacking
protection out of the box. The value is configurable through the new
frameOptions init parameter (DeploymentConfiguration#getFrameOptions);
an empty value disables the header for applications meant to be embedded
in a frame.
Fixes #23415