Copilot commented on code in PR #3400:
URL: https://github.com/apache/tika/pull/3400#discussion_r4351364499
##########
tika-app/src/main/java/org/apache/tika/gui/TikaGUI.java:
##########
@@ -527,14 +530,10 @@ public void startElement(String uri, String localName,
String name, Attributes a
if (XHTMLContentHandler.XHTML.equals(uri)) {
uri = null;
}
- if (!"head".equals(localName)) {
+ if (!HTML_VIEW_DROPPED.contains(localName)) {
if ("img".equals(localName)) {
- AttributesImpl newAttrs;
- if (atts instanceof AttributesImpl) {
- newAttrs = (AttributesImpl) atts;
- } else {
- newAttrs = new AttributesImpl(atts);
- }
+ // a copy: the other tee'd handlers see the same
attributes
+ AttributesImpl newAttrs = new AttributesImpl(atts);
for (int i = 0; i < newAttrs.getLength(); i++) {
if ("src".equals(newAttrs.getLocalName(i))) {
Review Comment:
`localName` (and `getLocalName(i)`) can be an empty string when SAX
namespace processing is disabled; in that case the dropped-element filter and
the `<img src>` stripping won’t trigger, potentially reintroducing the unsafe
behavior this PR is meant to prevent. Consider falling back to
`name`/`getQName(i)` when `localName` is empty (and optionally normalizing
case) before checking `HTML_VIEW_DROPPED`, `"img"`, and `"src"`.
##########
tika-app/src/main/java/org/apache/tika/gui/TikaGUI.java:
##########
@@ -105,6 +105,9 @@ public class TikaGUI extends JFrame implements
ActionListener, HyperlinkListener
//maximum length to allow for mark for reparse to get JSON
private static final int MAX_MARK = 20 * 1024 * 1024;//20MB
+ // Swing's html view instantiates the class an <object> names and fetches
<link> stylesheets
Review Comment:
The comment has a small grammatical issue (“the class an `<object>` names”)
and doesn’t mention the remote `<img src>` fetch behavior that this change also
mitigates. Consider rewording to clearly capture both effects (component
instantiation via `<object classid>`/`<param>` and network fetches via `<link>`
and remote `<img src>`).
##########
tika-app/src/main/java/org/apache/tika/gui/TikaGUI.java:
##########
@@ -551,6 +550,10 @@ public void startElement(String uri, String localName,
String name, Attributes a
System.err.println("Error creating
temp image file " + filename);
// The html viewer will show a broken
image too to alert them
}
+ } else {
+ // the view would fetch it on display
+ newAttrs.removeAttribute(i);
+ i--;
Review Comment:
This introduces security-sensitive sanitization behavior
(dropping/selectively rewriting elements/attributes) but adds no regression
coverage. Even without a GUI harness, this logic should be unit-testable by
feeding SAX events (or parsing a small XHTML snippet) through the handler and
asserting that `<object>/<param>/<link>/<head>` are omitted and that `<img
src>` is preserved only for `embedded:` while other `src` values are removed.
--
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]
