snoopdave commented on PR #154:
URL: https://github.com/apache/roller/pull/154#issuecomment-5973478728
🐞Claude Issue: **PR-Review: General Issues**
The following issues were found but cannot be attached to a specific line in
the diff:
- **Blocking: why checks fail.** CodeQL reports 4 new high alerts. The XSS
one is covered inline. The other three are `java/path-injection` at
`MediaCollection.java:122`, `:141` and `:185`. That code is unchanged, but the
forked servlet now exposes the request source to CodeQL. The `Slug` header
becomes the `File.createTempFile` prefix. The JDK strips directory parts from
the prefix (I checked `"../../evil"` on JDK 17), so this is not exploitable.
Still, the prefix has no reason to carry request data: use
`File.createTempFile("roller-atom-", ".tmp")`, which removes all three alerts
without dismissals.
- **Blocking:** The PR conflicts with `master` (`mergeStateStatus: DIRTY`)
and needs another merge.
- **Blocking:** `CHANGES.md` is not updated. This PR removes OAuth 1.0a for
AtomPub, the consumer key UI and OpenID 2.0 login. It also raises the baseline
to Java 17 and Tomcat 11 / Servlet 6.1. Each of these needs an upgrade note
linked to [ROL-2183](https://issues.apache.org/jira/browse/ROL-2183).
- **Scope note:** At 226 files, this review covered CI, `security.xml`, and
the AtomPub fork. The Struts 7, `@StrutsParameter` and JPA namespace changes
were not reviewed line by line.
--
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]
