-
Improvement
-
Resolution: Fixed
-
Minor
-
None
-
DocFX 2.28.2
As requested in JENKINS-48669, I'm filing a sample DocFX log so you can make Violations Lib support it. The format is a series of JSON objects with newlines in between.
The log file is written by C# code shown here: https://github.com/dotnet/docfx/blob/v2.28.2/src/Microsoft.DocAsCode.Common/Loggers/ReportLogListener.cs
Notes on the properties of the JSON objects:
- "message": The most important text.
- "source": What the program was doing when it generated the log entry.
- "file": The value in is bizarrely HTML-encoded. This property is not always present.
- "line": This property is not always present.
- "date_time": Always UTC.
- "message_severity": Lower case.
- "code": Seems to be an error code for grouping. This property is not always present.
- "correlation_id": Consists of a GUID (random by default) and a hierarchical identifier. Does not seem useful to include in violation comments.
[JENKINS-48670] Support DocFX JSON log in Violations Lib
The file attribute is mandatory for the lib. So I will just ignore the entries that has no line. And that leaves only 3 violations parsed from the file that you supply:
Is that what you would expect?
Yes, I think it is OK not to generate violation comments from the DocFX log entries that lack a "file" property. My conversion script likewise discards them. (It might be best to feed them to the Warnings Plug-in, though.)
It looks like DocFX log entries with severity "info", "verbose", or "diagnostic" are generally not useful to include in violation comments, even if they have "file" and "line" properties. This can be handled with the minSeverity: 'WARN' option of ViolationsToBitbucketServerRecorder and so does not need special handling in DocFXParser. I don't think it is necessary to support specifying a separate minSeverity for each parser.
Kalle Niemitalo
added a comment - - edited Yes, I think it is OK not to generate violation comments from the DocFX log entries that lack a "file" property. My conversion script likewise discards them. (It might be best to feed them to the Warnings Plug-in, though.)
It looks like DocFX log entries with severity "info", "verbose", or "diagnostic" are generally not useful to include in violation comments, even if they have "file" and "line" properties. This can be handled with the minSeverity: 'WARN' option of ViolationsToBitbucketServerRecorder and so does not need special handling in DocFXParser. I don't think it is necessary to support specifying a separate minSeverity for each parser. Kalle Niemitalo
added a comment - Is there any security risk with your use of ScriptEngine.eval? Could malicious JSON create instances of unexpected Java classes and bypass a trust boundary? I'm not sure there is a trust boundary between the Jenkins agent and the tools it runs.
Kalle Niemitalo
added a comment - Is there any security risk with your use of ScriptEngine.eval ? Could malicious JSON create instances of unexpected Java classes and bypass a trust boundary? I'm not sure there is a trust boundary between the Jenkins agent and the tools it runs. I'm pretty sure it is possible to create a custom file with javascript and that script will be executed by the plugin.
But then you have to have access to Jenkins in order to create that file. I would not worry as long as the regexp, that makes the plugin recognize the file as a DocFX file, points to something that is only generated by DocFX.
Byt I'm not the paranoid kind =)
The reason for doing it like this is because I don't want to add third party libraries to the classpath. I want to avoid classpath collisions when people use the lib.
All the plugins should be released now!
Kalle Niemitalo
added a comment - Can you please get the JSON parsing reviewed by the Jenkins security team? I suspect there may be a vulnerability in a Docker scenario, where users would expect the container to restrict what the tools can do to the Jenkins agent.
The Jenkins core depends on a patched json-lib, but I presume you don't want to use that in violations-lib, which does not otherwise depend on Jenkins. Would it be feasible to make the Jenkins plugins provide json-lib's JSON parser to violations-lib as a service somehow, and use ScriptEngine.eval only if the service is not provided?
Kalle Niemitalo
added a comment - Can you please get the JSON parsing reviewed by the Jenkins security team? I suspect there may be a vulnerability in a Docker scenario, where users would expect the container to restrict what the tools can do to the Jenkins agent.
The Jenkins core depends on a patched json-lib, but I presume you don't want to use that in violations-lib, which does not otherwise depend on Jenkins. Would it be feasible to make the Jenkins plugins provide json-lib's JSON parser to violations-lib as a service somehow, and use ScriptEngine.eval only if the service is not provided? I replaced ScriptEngine with Gson now:
https://github.com/tomasbjerre/violations-lib/commit/524c39c2ff47d40f2ad2622b22d69f6f2550ba41
Also released the plugins.
I've changed it to ignore casing when checking for equality.
I've added a DocFX parser.
The DocFX report file is always UTF-8 without Byte Order Mark.