junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces #175

tsmock · 2022-01-21T15:56:59Z

This occurs when the stacktrace message contains ]]>, which is the CDATA
end code. There is no escape, so it must be replaced with ]] + ]]> +
<![CDATA[ + >, which means that the CDATA section is split.

azotcsit · 2022-01-21T18:59:58Z

src/main/org/apache/tools/ant/taskdefs/optional/junitlauncher/LegacyXmlResultFormatter.java

-                writer.writeCData(StringUtils.getStackTrace(t));
+                // write out the stacktrace, replacing any CDATA endings by splitting them into multiple CDATA segments
+                // See https://bz.apache.org/bugzilla/show_bug.cgi?id=65833
+                writer.writeCData(StringUtils.getStackTrace(t).replaceAll("]]>", "]]]]><![CDATA[>"));


Currently there is encode method which seems to be intended for handling special character. So this logic probably would fit there better.

PS:
I'm not a project maintainer and ant expert, just sharing my ideas since I've been recently exploring this code.

The encode method currently only handles special characters (generic XML). This handles a special sequence for a specific data section. ]]>.

From [1], """Within a CDATA section, only the CDEnd string is recognized as markup, so that left angle brackets and ampersands may occur in their literal form; they need not (and cannot) be escaped using " < " and " & ". CDATA sections cannot nest.""" (CDEnd === ]]>)

This means that replacing > with > would not work, as CDATA parsers shouldn't treat > === >. They should treat it as a literal > (since the & is literal), which can break many usages of the stacktrace. Sure, people could have regexes to fix the stack trace, but that is duplicate work for many people.

With all that said, writeCData should properly handle text with ]]> in it, but since people have had to work around it, it may no longer be possible to fix the issue in the actual implementing libraries without a lot of work. How would it know if you were working around this issue, and not escape the cdata sequence again? (]]> -> ]]]]><!CDATA[> -> ]]]]]]><!CDATA[><!CDATA[>).
Example pitfall: A test failure outputs XML with a CDATA section, which is then written to CDATA in LegacyXmlResultFormatter. We want to keep the XML output as is, which means escaping the interior CDATA endings. If this then happens again in the writeCData method, then the xml output shown to the user will most likely be wrong. From the above example, instead of ]]>, the user would see ]]]]><!CDATA[> unless the CDATA parser recursively parses the data (unlikely IMO).

[1] https://www.w3.org/TR/REC-xml/#dt-cdsection

Ok, thanks for the details! Originally I was not clear why splitting CDATA section is better than escaping. But looks like it is a common practice for nesting CDATA sections [1]. Moreover, junit team fixed such an issue in the same way [2]. The fix looks good to me!

[1] https://en.wikipedia.org/wiki/CDATA#Nesting
[2] junit-team/junit5#1225

src/main/org/apache/tools/ant/taskdefs/optional/junitlauncher/LegacyXmlResultFormatter.java

jaikiran · 2022-01-26T04:30:01Z

This PR looks fine to me. Thank you for fixing this. Given that we have had odd issues in this area with XML content, do you think it's possible to add a test case to reproduce this issue and verify the fix? If so, would you be willing to add one in this PR?

tsmock · 2022-01-26T14:03:40Z

This PR looks fine to me. Thank you for fixing this. Given that we have had odd issues in this area with XML content, do you think it's possible to add a test case to reproduce this issue and verify the fix? If so, would you be willing to add one in this PR?

I was looking into this yesterday. I'll say this much, figuring out how the tests were run and verified took me a bit of time, and the output from ./build.sh clean test does not have any mention of junitlauncher on my system. As in, it appears that those tests do not run with the suggested test command in CONTRIBUTING.md. I don't know if it is just my system or not (Mac OS X 12.1), so I was going to try it on a Raspberry Pi sometime today.

EDIT: The junitlauncher tests did not run on the Pi either. I looked for ant CI and found https://builds.apache.org/job/Ant/job/Ant_Nightly/lastBuild/consoleText -- it looks like junitlauncher tests are not running there either.

Bugzilla Report 65833 This occurs when the stacktrace message contains ]]>, which is the CDATA end code. There is no escape, so it must be replaced with `]]` + `]]>` + `<![CDATA[` + `>`, which means that the CDATA section is split. Signed-off-by: Taylor Smock <tsmock@fb.com>

Non-regression test for Bugzilla Report 65833 This also updates JUnit 4 to 4.13.2, JUnit 5 to 5.8.2, and JUnit Platform to 1.8.2. This also adds junit-platform-testkit as a test dependency. Note: At time of this commit, tests for junitlauncher must be run from the pom.xml for junitlauncher. Attempting to modify build.xml to run the tests will result in a failure due to the presence of JUnit 3.8.2 on the classpath. Signed-off-by: Taylor Smock <tsmock@fb.com>

azotcsit · 2022-01-27T08:59:41Z

@tsmock
I experienced the same problem and @jaikiran suggested to fetch necessary dependencies by running ant -f fetch.xml -Ddest=optional [1]. It actually helped me.

[1] https://lists.apache.org/thread/2v8l95fwh0sgz0vlgqcf1rpd88swf3pv

tsmock · 2022-01-27T13:52:47Z

@tsmock I experienced the same problem and @jaikiran suggested to fetch necessary dependencies by running ant -f fetch.xml -Ddest=optional [1]. It actually helped me.

[1] https://lists.apache.org/thread/2v8l95fwh0sgz0vlgqcf1rpd88swf3pv

It did not help me. See https://github.com/apache/ant/blob/master/build.xml#L1989 . Specifically, when I commented that line out, the tests failed due to having JUnit 3.8.2 on the class path (see https://github.com/apache/ant/tree/master/lib/optional ). I'm actually surprised that JUnit 3 is still supported in ant -- it looks like the last release for it was in 2006 or 2007. Junit 4.0 was released around that time.)

I was, however, able to run the tests with maven (I checked to make certain that all the tests will fail if the expected output was not there, as I changed a significant part of the logic in the test).

tsmock · 2022-02-09T15:18:08Z

Do I need to do anything else for this PR?

jaikiran · 2022-02-10T03:06:16Z

Hello Taylor, I just merged the initial commit that is part of this PR. I left out the tests for now since it even had changed some junit versions. I'll look into the testing part separately once I get some free time. As for testing this locally, what Aleksei pointed to should have worked (that's how we test locally), but I haven't been able to look more into your failures.

Thank you for the patience on this one. As part of the merge, I've added your name to the list of contributors to this project.

tsmock · 2022-02-10T13:42:42Z

I ended up updating the JUnit5 versions to get some of their newer testing methods for testing JUnit extensions. In any case, from what I know, there shouldn't be any effect on ant users by increasing the JUnit5 version, as the junitlauncher task requires the user to provide JUnit5 libraries on the path.

JUnit4 4.13.1 -> 4.13.2 should have no effect, minus the bug fixes
JUnit5 5.2.0 -> 5.8.2 is a bit bigger:

Added Java 12, 13, 14, 15, 16, 17, 18 in their JRE enum
@Suite support
EngineTestKit had some deprecated APIs removed in 5.7.0 (APIs were deprecated in 5.6.0)
Script-based APIs have been deprecated (5.5.0) and removed (5.6.0)
Many, many bug fixes

Also, thank you for letting me know that you didn't take everything from the branch -- I usually delete my branches once they get merged, but I won't delete this one for awhile. I will go ahead and rebase onto HEAD though.

tsmock changed the title ~~Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces~~ junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces Jan 21, 2022

azotcsit reviewed Jan 21, 2022

View reviewed changes

tsmock force-pushed the fixup-bugzilla-65833-invalid-cdata branch from 787ece4 to b1b5dbf Compare January 25, 2022 14:10

jaikiran reviewed Jan 26, 2022

View reviewed changes

src/main/org/apache/tools/ant/taskdefs/optional/junitlauncher/LegacyXmlResultFormatter.java Show resolved Hide resolved

tsmock force-pushed the fixup-bugzilla-65833-invalid-cdata branch from b1b5dbf to 3af5a01 Compare January 26, 2022 14:06

asfgit closed this in aa47924 Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces #175

junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces #175

tsmock commented Jan 21, 2022 •

edited

azotcsit Jan 21, 2022

tsmock Jan 24, 2022 •

edited

azotcsit Jan 25, 2022

jaikiran commented Jan 26, 2022

tsmock commented Jan 26, 2022 •

edited

azotcsit commented Jan 27, 2022

tsmock commented Jan 27, 2022

tsmock commented Feb 9, 2022

jaikiran commented Feb 10, 2022

tsmock commented Feb 10, 2022

junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces #175

junitlauncher - Fix an issue in LegacyXmlResultFormatter with ]]> in stacktraces #175

Conversation

tsmock commented Jan 21, 2022 • edited

azotcsit Jan 21, 2022

Choose a reason for hiding this comment

tsmock Jan 24, 2022 • edited

Choose a reason for hiding this comment

azotcsit Jan 25, 2022

Choose a reason for hiding this comment

jaikiran commented Jan 26, 2022

tsmock commented Jan 26, 2022 • edited

azotcsit commented Jan 27, 2022

tsmock commented Jan 27, 2022

tsmock commented Feb 9, 2022

jaikiran commented Feb 10, 2022

tsmock commented Feb 10, 2022

tsmock commented Jan 21, 2022 •

edited

tsmock Jan 24, 2022 •

edited

tsmock commented Jan 26, 2022 •

edited