jbonofre commented on code in PR #2676:
URL: https://github.com/apache/activemq/pull/2676#discussion_r4310569432
##########
activemq-web-console/src/test/java/org/apache/activemq/spring/jetty/JettyServerBeanTest.java:
##########
@@ -55,6 +56,13 @@ public void testJettyServerBean() throws Exception {
HttpResponse.BodyHandlers.ofString());
assertEquals("Web console must require authentication", 401,
response.statusCode());
+ HttpResponse<String> authenticatedResponse = client.send(
+ HttpRequest.newBuilder(URI.create("http://127.0.0.1:"; + port +
"/admin/"))
+ .header("Authorization", "Basic " +
java.util.Base64.getEncoder().encodeToString(
+
"admin:admin".getBytes(StandardCharsets.ISO_8859_1)))
+ .GET().build(),
+ HttpResponse.BodyHandlers.ofString());
+ assertEquals("SAMEORIGIN",
authenticatedResponse.headers().firstValue("X-Frame-Options").orElse(null));
Review Comment:
This request never actually authenticates. The test resource have no JAAS
configuration: there's no `login.config`, and neither
`java.security.auth.login.config` nor `activemq.conf` is set.
`JAASLoginService` can't load the `activemq` login module, so this request
gets a 401, just like the one above.
The assertion still passes because the `RewriteHandler` now runs before the
`SecurityHandler`, which puts `X-Frame-Options` on the 401 too. So the
authenticated path is not covered. The test would stay green even if headers
were dropped from 200 responses.
I suggest the following changes:
1. Add a test `login.config` with a `activemq` entry that uses
`PropertiesLoginModule`, plus `users.properties` and `groups.properties` that
put `admin` in `admins` (the role `/admin/*` requires). Point
`java.security.auth.login.config` at it, either through surefire
`systemPropertyVariables` or from the test itself.
2. Check the status before the header (the test webapp has an `index.html`,
so a 200 is expected):
```
assertEquals(200, authenticatedResponse.statusCode());
assertEquals("SAMEORIIGIN",
authenticatedResponse.headers().firstValue("X-Frame-Options").orElse(null));
```
3. Wrap up the test body in `try { ... } finally { context.close(); }`. If
this assertion fails, `context.stop()` is skipped and Jetty stays running into
the next test. Even when it isn't skipped, `stop()` does't call
`JettyServerBean.destroy()`. Only `close()` does.
Nit: `java.util.Base64` is written out in full inline, while
`StandardCharsets` gets an import.
##########
assembly/src/release/conf/jetty/jetty-webapps.xml:
##########
@@ -21,8 +21,9 @@
<Configure class="org.eclipse.jetty.server.handler.ContextHandlerCollection"
id="Contexts">
<!-- Rewrite Handler for Security Headers -->
- <Call name="addHandler">
- <Arg>
+ <Ref refid="Server">
+ <Call name="insertHandler">
Review Comment:
Moving the `RewriteHandler` to the server level is right.
But it means the CSP (line 68) is now actually sent, which has a side
effect. That policy sets `script-src-elem 'self'` and `default-src 'none'`,
with no `script-src` or `script-src-attr`. Inline event handlers fall back to
`default-src: 'none'`, so the browser blocks them.
The console still has three of them:
- `browse.jsp:64`: `onclick="return confirm('Are you sure you want to delete
this message?')"`
- `browse.jsp:85`: confirmation before retrying to the DLQ's target queue
- `message.jsp:154`: confirmation before Retry
When the `onclick` is blocked, the confirm dialog never appears and the link
goes straight to `deleteMessage.action` / `moveMessage.action` /
`retryMessage.action`.
So once this merges, Delete and Retry on a DLQ run without asking. Today
they prompt.
Could you move these three handlers to external JS, the same way
`js/message.js` already does for `actionDelete`?
Please don't loosen the CSP with `unsafe-inline` to get around it 😄
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
