Re: [PR] [MINOR UPDATE]: Potential fix for code scanning alert no. 43: Server-side request forgery (drill)

["pjfanning (via GitHub)" <gi...@apache.org>]

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: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org