TIKA-4744 - fix apple, rtf, odf, ooxml xhtml tag balancing

Author: tballison

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-apple-module/src/main/java/org/apache/tika/parser/iwork/KeynoteContentHandler.java

@@ -142,6 +142,12 @@ public void endElement(String uri, String localName, String qName) throws SAXExc
         } else if (inMetadata && "key:authors".equals(qName)) {
             inMetaDataAuthors = false;
         } else if (inSlide && "sf:tabular-model".equals(qName)) {
+            // If the final row has fewer cells than numberOfColumns,
+            // parseTableData never reaches its </tr> emit. Close any open
+            // row before </table> so the SAX stream stays balanced.
+            if (currentColumn != null && currentColumn != 0) {
+                xhtml.endElement("tr");
+            }
             xhtml.endElement("table");
             tableId = null;
             numberOfColumns = null;

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-apple-module/src/test/java/org/apache/tika/parser/iwork/KeynoteContentHandlerTest.java

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tika.parser.iwork;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+import org.xml.sax.helpers.AttributesImpl;
+
+import org.apache.tika.metadata.Metadata;
+import org.apache.tika.parser.ParseContext;
+import org.apache.tika.sax.StrictXHTMLValidator;
+import org.apache.tika.sax.ToXMLContentHandler;
+import org.apache.tika.sax.XHTMLContentHandler;
+
+public class KeynoteContentHandlerTest {
+
+    /**
+     * TIKA-4744: Keynote tables whose final row contains fewer cells than
+     * numberOfColumns left {@code <tr>} open when {@code </sf:tabular-model>}
+     * fired {@code </table>}. Drive the handler with a synthetic 3-column
+     * table whose final row has only 2 cells and assert the XHTML stays
+     * balanced through endDocument.
+     */
+    @Test
+    public void testIncompleteFinalRowClosesTr() throws Exception {
+        Metadata md = new Metadata();
+        ToXMLContentHandler xml = new ToXMLContentHandler();
+        XHTMLContentHandler xhtml = new XHTMLContentHandler(
+                new StrictXHTMLValidator(xml), md, new ParseContext());
+        KeynoteContentHandler h = new KeynoteContentHandler(xhtml, md);
+
+        xhtml.startDocument();
+
+        // <key:slide>
+        h.startElement("", "slide", "key:slide", new AttributesImpl());
+        // <sf:tabular-model sfa:ID="t1">
+        AttributesImpl tabAttrs = new AttributesImpl();
+        tabAttrs.addAttribute("", "sfa:ID", "sfa:ID", "CDATA", "t1");
+        h.startElement("", "tabular-model", "sf:tabular-model", tabAttrs);
+        // <sf:columns sf:count="3">
+        AttributesImpl colAttrs = new AttributesImpl();
+        colAttrs.addAttribute("", "sf:count", "sf:count", "CDATA", "3");
+        h.startElement("", "columns", "sf:columns", colAttrs);
+        h.endElement("", "columns", "sf:columns");
+        // Two cells (less than 3) -> incomplete final row.
+        AttributesImpl cell = new AttributesImpl();
+        cell.addAttribute("", "sfa:s", "sfa:s", "CDATA", "A");
+        h.startElement("", "ct", "sf:ct", cell);
+        h.endElement("", "ct", "sf:ct");
+        cell = new AttributesImpl();
+        cell.addAttribute("", "sfa:s", "sfa:s", "CDATA", "B");
+        h.startElement("", "ct", "sf:ct", cell);
+        h.endElement("", "ct", "sf:ct");
+        // </sf:tabular-model> -- without the fix this emits </table> while
+        // <tr> is still topmost and StrictXHTMLValidator throws.
+        h.endElement("", "tabular-model", "sf:tabular-model");
+        // </key:slide>
+        h.endElement("", "slide", "key:slide");
+
+        xhtml.endDocument();
+
+        String out = xml.toString();
+        // Sanity: the cells emitted, the row closed, and the table closed.
+        assertTrue(out.contains("<td>A</td>"), "expected td A; got: " + out);
+        assertTrue(out.contains("<td>B</td>"), "expected td B; got: " + out);
+        assertTrue(out.contains("</tr>"), "expected </tr>; got: " + out);
+        assertTrue(out.contains("</table>"), "expected </table>; got: " + out);
+    }
+}

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/FormattingTagManager.java

@@ -114,14 +114,16 @@ void applyFormatting(RunProperties runProperties) throws SAXException {
         }
 
         if (runProperties.isBold() != isBold) {
-            if (isStrikeThrough) {
-                xhtml.endElement("s");
-                isStrikeThrough = false;
-            }
+            // Close inner tags before flipping <b>. Nesting is <b><i><s><u>
+            // (outermost to innermost), so close innermost first: u, s, i.
             if (isUnderline) {
                 xhtml.endElement("u");
                 isUnderline = false;
             }
+            if (isStrikeThrough) {
+                xhtml.endElement("s");
+                isStrikeThrough = false;
+            }
             if (isItalics) {
                 xhtml.endElement("i");
                 isItalics = false;
@@ -135,14 +137,15 @@ void applyFormatting(RunProperties runProperties) throws SAXException {
         }
 
         if (runProperties.isItalics() != isItalics) {
-            if (isStrikeThrough) {
-                xhtml.endElement("s");
-                isStrikeThrough = false;
-            }
+            // Close inner tags before flipping <i>: u then s (u is innermost).
             if (isUnderline) {
                 xhtml.endElement("u");
                 isUnderline = false;
             }
+            if (isStrikeThrough) {
+                xhtml.endElement("s");
+                isStrikeThrough = false;
+            }
             if (runProperties.isItalics()) {
                 xhtml.startElement("i");
             } else {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/OOXMLTikaBodyPartHandler.java

@@ -265,7 +265,16 @@ private void popExpected(String tag) {
 
     @Override
     public void startTable() throws SAXException {
-
+        // A <w:tbl> can appear nested inside an outer <w:p> -- corrupt-ish but
+        // present in the corpus (e.g., <w:p><w:r>...<wps:txbx><w:txbxContent>
+        // <w:tbl>...). At that point a run-level <b>/<i>/<u>/<s>/<a> may be on
+        // the SAX stack just above where the <table> is about to land. When a
+        // later paragraph inside a cell ends, formattingTags.closeAll() tries
+        // to emit </b> for the outer-paragraph state, but <td> is topmost --
+        // strict validator rejects the mismatch. Close pending formatting now
+        // so the table opens at a clean layer and the outer style is forgotten.
+        // Mirrors startSDT()'s same-shape guard.
+        formattingTags.closeAll();
         xhtml.startElement("table");
         openStructuralTags.push("table");
         tableDepth++;

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/rtf/RTFEmbObjHandler.java

@@ -35,6 +35,7 @@
 import org.apache.tika.metadata.TikaCoreProperties;
 import org.apache.tika.parser.ParseContext;
 import org.apache.tika.sax.EmbeddedContentHandler;
+import org.apache.tika.sax.XHTMLBalancingHandler;
 
 /**
  * This class buffers data from embedded objects and pictures.
@@ -235,12 +236,24 @@ private void extractObj(byte[] bytes, ContentHandler handler, Metadata metadata)
                     }
                     metadata.set(TikaCoreProperties.RESOURCE_NAME_EXTENSION_INFERRED, true);
                 }
+                // Wrap the outer handler in a balancing handler so that if
+                // the embedded parser aborts mid-element (leaving <p>/<b>/etc
+                // open on the wire), we can drain those before propagating.
+                // Without this, the outer RTF parser's subsequent
+                // </b>/</p>/</body> sequence trips StrictXHTMLValidator on
+                // unbalanced nesting and surfaces as a misleading error at
+                // endDocument. Same shape as the SXWPF / Epub catch arms.
+                XHTMLBalancingHandler balancer = new XHTMLBalancingHandler(handler);
                 try {
                     embeddedDocumentUtil
-                            .parseEmbedded(tis, new EmbeddedContentHandler(handler), metadata,
+                            .parseEmbedded(tis, new EmbeddedContentHandler(balancer), metadata,
                                     true);
                 } catch (IOException e) {
+                    balancer.drainOpenElements();
                     EmbeddedDocumentUtil.recordEmbeddedStreamException(e, metadata);
+                } catch (SAXException e) {
+                    balancer.drainOpenElements();
+                    EmbeddedDocumentUtil.recordException(e, metadata);
                 }
             }
         }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/rtf/RTFParser.java

@@ -35,6 +35,7 @@
 import org.apache.tika.mime.MediaType;
 import org.apache.tika.parser.ParseContext;
 import org.apache.tika.parser.Parser;
+import org.apache.tika.sax.XHTMLBalancingHandler;
 import org.apache.tika.sax.XHTMLContentHandler;
 
 /**
@@ -90,11 +91,22 @@ public void parse(TikaInputStream tis, ContentHandler handler, Metadata metadata
         TaggedInputStream tagged = new TaggedInputStream(tis);
         XHTMLContentHandler xhtml = new XHTMLContentHandler(handler, metadata, context);
         xhtml.startDocument();
+        // Wrap xhtml in a balancing handler so the finally's endDocument
+        // doesn't fire on an unbalanced stack if the RTF state machine
+        // emits inconsistent SAX events (e.g., </b> while <p> is topmost
+        // due to state-vs-stack drift on certain corpus files). Without
+        // this, StrictXHTMLValidator's </body> vs <p>/<b> at endDocument
+        // masks the real well-formedness error from inside extract().
+        XHTMLBalancingHandler balancer = new XHTMLBalancingHandler(xhtml);
         try {
-            parseInline(tis, xhtml, metadata, context);
+            parseInline(tis, balancer, metadata, context);
         } catch (IOException e) {
             tagged.throwIfCauseOf(e);
+            balancer.drainOpenElements();
             throw new TikaException("Error parsing an RTF document", e);
+        } catch (SAXException e) {
+            balancer.drainOpenElements();
+            throw e;
         } finally {
             xhtml.endDocument();
         }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/rtf/TextExtractor.java

@@ -473,7 +473,6 @@ public void extract(InputStream in) throws IOException, SAXException, TikaExcept
     }
 
     private void extract(PushbackInputStream in) throws IOException, SAXException, TikaException {
-
         while (true) {
             final int b = in.read();
             if (b == -1) {
@@ -648,6 +647,16 @@ private void endParagraph(boolean preserveStyles)
             lazyStartParagraph();
         }
         if (inParagraph || paragraphStack.size() > 0) {
+            // A HYPERLINK's <a> can be open mid-fldrslt when a \par is
+            // encountered. Closing </p> while <a> is topmost on the SAX
+            // stack trips StrictXHTMLValidator -- close </a> first and
+            // forget the hyperlink state (the paragraph break terminates
+            // the link visually anyway).
+            if (hyperlinkAnchorDepth >= 0) {
+                end("a");
+                fieldState = 0;
+                hyperlinkAnchorDepth = -1;
+            }
             if (groupState.italic) {
                 end("i");
                 groupState.italic = preserveStyles;
@@ -1385,6 +1394,13 @@ private void processControlWord() throws IOException, SAXException, TikaExceptio
             }
         } else if (equals("fldrslt") && fieldState == 2) {
             assert pendingURL != null;
+            // Temporarily clear fieldState so lazyStartParagraph's endStyles
+            // actually flushes any pending <b>/<i>. endStyles short-circuits
+            // when fieldState != 0 to keep style flips inside an already-open
+            // <a> from leaking; but here we're about to OPEN the <a>, so any
+            // outer <b>/<i> still on the SAX stack must close first or <p>
+            // would land on top of <b> and the link's </b> later mismatches.
+            fieldState = 0;
             lazyStartParagraph();
             AttributesImpl attrs = new AttributesImpl();
             attrs.addAttribute(XHTML, "href", "href", "CDATA", pendingURL);
@@ -1485,6 +1501,12 @@ private void processGroupEnd() throws IOException, SAXException, TikaException {
                 embObjHandler.handleCompletedObject();
             } catch (TikaException | IOException e) {
                 EmbeddedDocumentUtil.recordException(e, metadata);
+            } catch (RuntimeException e) {
+                // POI dispatch on a zero-byte embedded object throws
+                // EmptyFileException; other malformed embedded payloads
+                // can surface as a variety of runtime exceptions. Record
+                // and continue rather than aborting the outer RTF parse.
+                EmbeddedDocumentUtil.recordException(e, metadata);
             }
             groupState.objdata = false;
         } else if (groupState.pictDepth > 0) {
@@ -1511,6 +1533,18 @@ private void processGroupEnd() throws IOException, SAXException, TikaException {
         if (groupStates.size() > 0) {
             // Restore group state:
             final GroupState outerGroupState = groupStates.removeLast();
+            // If we're leaving the fldrslt group that owns the current <a>,
+            // close </a> BEFORE the style restore runs. Otherwise the
+            // restore's start("b") would land on top of <a>, and the next
+            // </a>/<b> would mismatch. Doing this here also flips fieldState
+            // to 0, which un-gates the style compare below so the outer
+            // bold/italic context is actually re-emitted.
+            if (fieldState == 3 && hyperlinkAnchorDepth >= 0
+                    && outerGroupState.depth < hyperlinkAnchorDepth) {
+                end("a");
+                fieldState = 0;
+                hyperlinkAnchorDepth = -1;
+            }
             //only modify styles if we're not in a hyperlink
             if (fieldState == 0) {
                 // Close italic, if outer does not have italic or

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/parser/microsoft/ooxml/OOXMLDocxSAXTest.java

@@ -282,6 +282,34 @@ public void testEmptyParagraphInTextbox() throws Exception {
         assertContains("Vedný odbor:", r.xml);
     }
 
+    @Test //TIKA-4744
+    public void testTableInRunInsideParagraph() throws Exception {
+        // A <w:tbl> nested deeply inside an outer <w:p>'s run (via
+        // <mc:AlternateContent>/<w:drawing>/<wps:txbx>/<w:txbxContent>) used
+        // to leave the run's <b>/<i>/etc on the SAX stack below the new
+        // <table>. A subsequent paragraph end inside a cell then tried to
+        // closeFormattingTags() for the outer state, emitting </b> while
+        // <td> was topmost. startTable now closes pending formatting first,
+        // mirroring startSDT.
+        XMLResult r = getXML("testWORD_tableInRunInsideParagraph.docx");
+        assertContains("<table>", r.xml);
+    }
+
+    @Test //TIKA-4744
+    public void testFormattingFlipPreservesNestingOrder() throws Exception {
+        // FormattingTagManager.applyFormatting used to close </s> before </u>
+        // in the bold-flip and italic-flip branches, but XHTML nesting is
+        // <b><i><s><u> (u is innermost). A run with bold+strike+underline
+        // followed by a non-bold run trips the wrong-order close: </s>
+        // emitted while <u> is topmost, then the catch arm's closeAnyPending
+        // tries </u> against <s> -- the visible </u> vs <s> the validator
+        // flagged on three corpus files.
+        XMLResult r = getXML("testWORD_formattingFlipUnderlineOrder.docx");
+        // Just confirm something extracted -- the real check is no validator
+        // throw, which getXML enforces via StrictXHTMLValidator.
+        assertContains("<body>", r.xml);
+    }
+
     @Test
     public void testDOCXOverrideParagraphNumbering() throws Exception {
         String xml = getXML("testWORD_override_list_numbering.docx").xml;

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/parser/microsoft/rtf/RTFParserTest.java

@@ -60,6 +60,22 @@ public void testBasicExtraction() throws Exception {
         assertContains("indexation Word", content);
     }
 
+    @Test //TIKA-4744
+    public void testParInsideHyperlink() throws Exception {
+        // A \par inside a HYPERLINK's \fldrslt used to leave <a> on the SAX
+        // stack while endParagraph emitted </p>, tripping the strict
+        // validator with </p> vs <a>. The exception was masked by the
+        // finally's xhtml.endDocument() throwing </body> vs <p>; visible
+        // end-state was just "<p> open at endDocument". Closing pending <a>
+        // before </p> drops the link span at the paragraph break, which is
+        // the right XHTML rendering.
+        XMLResult r = getXML("testRTF_parInsideHyperlink.rtf");
+        // First paragraph contains the link text, properly closed.
+        assertContains("<a href=\"#target\">line1</a>", r.xml);
+        // Second paragraph picks up the post-\par text without an open <a>.
+        assertContains("line2", r.xml);
+    }
+
     @Test //TIKA-4744
     public void testNestedHyperlinkPageRef() throws Exception {
         // A HYPERLINK field with a PAGEREF \field nested inside its \fldrslt

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/resources/test-documents/testRTF_parInsideHyperlink.rtf

@@ -0,0 +1,9 @@
+{\rtf1\ansi
+{\fonttbl{\f0 Arial;}}
+\pard {\f0 Before }
+{\field {\*\fldinst HYPERLINK "#target" }{\fldrslt 
+{\f0 line1\par line2}
+}}
+{\f0  after.}
+\par
+}