Fix XML Reporters not Escaping Characters #969

fabb · 2016-12-12T09:53:02Z

Fixes #968.

…tains characters '<' and '>' which are not allowed in xml or html

codecov-io · 2016-12-12T10:11:11Z

Current coverage is 81.16% (diff: 95.16%)

Merging #969 into master will increase coverage by 0.20%

@@             master       #969   diff @@
==========================================
  Files           123        124     +1   
  Lines          5774       5825    +51   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4675       4728    +53   
+ Misses         1099       1097     -2   
  Partials          0          0

Powered by Codecov. Last update 288a3ec...402d55c

marcelofabri · 2016-12-12T14:50:59Z

Source/SwiftLintFramework/Reporters/String+XML.swift

@@ -0,0 +1,25 @@
+//
+//  CheckstyleReporter.swift


wrong filename

good catch :-)

marcelofabri · 2016-12-12T14:51:56Z

Source/SwiftLintFramework/Reporters/String+XML.swift

@@ -0,0 +1,25 @@
+//
+//  CheckstyleReporter.swift


Also, there's an "Extensions" group/folder, this file probably belong there

ok, moved it

marcelofabri · 2016-12-12T17:13:37Z

Thanks for this! Could you add a changelog entry? Check https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

jpsim

Missing a changelog entry.

jpsim · 2016-12-12T17:41:04Z

Source/SwiftLintFramework/Reporters/CheckstyleReporter.swift

@@ -25,11 +25,11 @@ public struct CheckstyleReporter: Reporter {
    }

    private static func generateForSingleViolation(_ violation: StyleViolation) -> String {
-        let file: String = violation.location.file ?? "<nopath>"
+        let file: String = violation.location.file?.escapedForXml() ?? "<nopath>"


"<nopath>" needs to be escaped: (violation.location.file ?? "<nopath>").escapedForXml()

thanks, good catch! i added an example to the unit tests

jpsim · 2016-12-12T17:41:54Z

Source/SwiftLintFramework/Reporters/HTMLReporter.swift

@@ -120,7 +120,7 @@ public struct HTMLReporter: Reporter {
    private static func generateSingleRow(for violation: StyleViolation, at index: Int) -> String {
        let severity: String = violation.severity.rawValue.capitalized
        let location = violation.location
-        let file: String = location.file ?? ""
+        let file: String = location.file?.escapedForXml() ?? ""


we should probably use "<nopath>" here too for consistency with CheckstyleReporter.swift: (location.file ?? "<nopath>").escapedForXml()

jpsim · 2016-12-12T17:42:16Z

Source/SwiftLintFramework/Reporters/JUnitReporter.swift

@@ -19,11 +19,12 @@ public struct JUnitReporter: Reporter {
    public static func generateReport(_ violations: [StyleViolation]) -> String {
        return "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<testsuites><testsuite>" +
            violations.map({ violation in
-                let fileName = violation.location.file ?? "<nopath>"
+                let fileName = violation.location.file?.escapedForXml() ?? "<nopath>"


"<nopath>" needs to be escaped: (violation.location.file ?? "<nopath>").escapedForXml()

… too

fabb · 2016-12-13T07:48:29Z

Sorry for not reading CONRIBUTING.md 🤦

fabb · 2016-12-13T08:14:58Z

Tests/SwiftLintFrameworkTests/ReporterTests.swift

@@ -88,7 +127,11 @@ class ReporterTests: XCTestCase {
            CSVReporter.generateReport(generateViolations()),


are there newlines missing in the CSVReporter output? right now all is just a single line. (out of scope of this PR though)

jpsim · 2016-12-13T08:16:20Z

CHANGELOG.md

@@ -61,6 +61,10 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#934](https://github.com/realm/SwiftLint/issues/934)

+* Fix XML reporters not escaping characters


still missing trailing period, followed by two spaces: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

maybe we should lint that file in the unit tests ;-)

jpsim · 2016-12-13T08:16:54Z

Source/SwiftLintFramework/Extensions/String+XML.swift

+//
+
+extension String {
+    func escapedForXml() -> String {


would be nice if XML was capitalized: escapedForXML()

…ept html reporter output, as that includes parametrized stuff

# Conflicts: # CHANGELOG.md

fabb

changes as suggested

fabb · 2016-12-13T08:16:56Z

Tests/SwiftLintFrameworkTests/ReporterTests.swift

+                location: location,
+                reason: "Shorthand syntactic sugar should be used" +
+                ", i.e. [Int] instead of Array<Int>."),
+            StyleViolation(ruleDescription: ColonRule.description,


since i added the extra violations, xcode becomes unresponsive for 10-20 seconds every time i open this file. i guess Xcode/SourceKit cannot cope with that many string appendings. I moved the expectation strings to other files - hope that's allright

fabb · 2016-12-13T09:47:53Z

Source/SwiftLintFramework/Reporters/HTMLReporter.swift

+    }
+
+    // swiftlint:disable function_body_length
+    // swiftlint:disable line_length


i tried these 2 variants, but it didn't work:

// swiftlint:disable function_body_length line_length

// swiftlint:disable:next function_body_length // swiftlint:disable:next line_length

bug?

yeah, could you please file a new ticket for this? I don't think my quick implementation from last week is 100% correct.

sure, done: #976

Fixed in #978, thanks!

fabb · 2016-12-13T09:53:13Z

CHANGELOG.md

@@ -61,6 +61,10 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#934](https://github.com/realm/SwiftLint/issues/934)

+* Fix XML reporters not escaping characters


maybe we should lint that file in the unit tests ;-)

jpsim · 2016-12-13T20:50:41Z

Thanks for pushing this forward @fabb! I've made some small changes and addressed the merge conflicts so I'll be merging this as part of #979.

fabb · 2016-12-14T05:48:49Z

Awesome, thanks a lot!

fabb added 2 commits December 12, 2016 10:19

added failing unit tests for syntactic sugar rule, as the message con…

9b88c34

…tains characters '<' and '>' which are not allowed in xml or html

escape characters for xml reporters

3ad0474

marcelofabri reviewed Dec 12, 2016

View reviewed changes

moved String+XML.swift to Extensions folder and fixed header comment

402d55c

jpsim requested changes Dec 12, 2016

View reviewed changes

fabb added 3 commits December 12, 2016 19:35

added change to CHANGELOG.md

e18f90c

added failing unit tests for empty location

5bd9392

fixed <nopath> not being xml encoded + use <nopath> for html reporter…

fd74d68

… too

fabb commented Dec 13, 2016

View reviewed changes

jpsim requested changes Dec 13, 2016

View reviewed changes

fabb added 5 commits December 13, 2016 10:02

moved all expected reporter results for tests to external files - exc…

d40a557

…ept html reporter output, as that includes parametrized stuff

moved expected html reporter result for tests to external file too

51c3a01

uppercase XML

6ce164d

fixed changelog format

ca45082

Merge branch 'master' into fix_xml

4d75640

# Conflicts: # CHANGELOG.md

fabb commented Dec 13, 2016

View reviewed changes

Merge branch 'master' into fix_xml

907db63

jpsim mentioned this pull request Dec 13, 2016

Fix XML Reporters not Escaping Characters #979

Merged

jpsim closed this Dec 13, 2016

fabb deleted the fix_xml branch December 14, 2016 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix XML Reporters not Escaping Characters #969

Fix XML Reporters not Escaping Characters #969

fabb commented Dec 12, 2016

codecov-io commented Dec 12, 2016 •

edited

Loading

marcelofabri Dec 12, 2016

fabb Dec 12, 2016

marcelofabri Dec 12, 2016

fabb Dec 12, 2016

marcelofabri commented Dec 12, 2016

jpsim left a comment

jpsim Dec 12, 2016

fabb Dec 13, 2016

jpsim Dec 12, 2016

fabb Dec 13, 2016

jpsim Dec 12, 2016

fabb Dec 13, 2016

fabb commented Dec 13, 2016

fabb Dec 13, 2016

jpsim Dec 13, 2016

fabb Dec 13, 2016

jpsim Dec 13, 2016

fabb left a comment

fabb Dec 13, 2016

fabb Dec 13, 2016

jpsim Dec 13, 2016

fabb Dec 13, 2016

jpsim Dec 13, 2016

fabb Dec 13, 2016

jpsim commented Dec 13, 2016

fabb commented Dec 14, 2016

		@@ -88,7 +127,11 @@ class ReporterTests: XCTestCase {
		CSVReporter.generateReport(generateViolations()),

Fix XML Reporters not Escaping Characters #969

Fix XML Reporters not Escaping Characters #969

Conversation

fabb commented Dec 12, 2016

codecov-io commented Dec 12, 2016 • edited Loading

Current coverage is 81.16% (diff: 95.16%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Dec 12, 2016

jpsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabb commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Dec 13, 2016

fabb commented Dec 14, 2016

codecov-io commented Dec 12, 2016 •

edited

Loading