pjfanning commented on PR #3090:
URL: https://github.com/apache/drill/pull/3090#issuecomment-5963844118
I think this change is redundant. `WebUtils.getDrillbitURL` already
validates the hostname against the registered Drillbits before it builds any
URL:
```java
int drillbitPort = work.getContext().getAvailableBits().stream()
.filter(db -> db.getAddress().equals(hostname))
.findAny()
.map(DrillbitEndpoint::getHttpPort)
.orElseThrow(() -> new RuntimeException("No such drillbit: " +
hostname));
```
An unknown hostname throws before any request is sent, and the port comes
from the endpoint record rather than from user input. The new block runs the
same allowlist match a second time. Other notes:
- The null/empty check can't trigger. `{hostname}` is a path segment, so an
empty value won't route to this method.
- The only behaviour change is the exception type (`RuntimeException`
becomes `IllegalArgumentException`). Both return a 500.
- The endpoint is already `@RolesAllowed(ADMIN_ROLE)`.
I'd suggest closing this and dismissing code-scanning alert 43 as a false
positive, pointing to the allowlist lookup in `getDrillbitURL`. That lookup
also covers the other callers of `getDrillbitURL`. If we want to make that
clear to future readers or scanners, a short comment in `getDrillbitURL` would
be better than duplicating the check at each call site.
--
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]
