feat: HTML report #130
feat: HTML report #130
Conversation
|
Hi @tcdsv, I'm not reviewing it because it is a draft. Please let me know if you want me to review it. |
There was a problem hiding this comment.
Good work, of course, more work is needed here.
- @jossef, do we need to ask a designer to look at the HTML output?
- Regarding reports from multiple sources, it is not supported right now. Of course, it might be supported in the future, but you shouldn't implement it right now.
It's not easy to include the source in the summary, as the first item in the map of
Secretsneeds to be retrieved in order to check itsSourceattribute.
I don't understand.
The design of the
Reportstruct, which organizes secrets based on filenames, makes it nontrivial to generate a report with multiple sources where secrets are grouped by source type. This suggests that for supporting reports with multiple sources, a possible mapping order inReportstruct could besources->filenames->secrets.
I agree that the structure is nontrivial, but please consider we are not supporting multiple sources, are you still see a problem with this structure?
The
Reportstruct lacks information such as version, time started, and time ended, which cannot be included in the summary. theConfigstruct has attributes likeNameandVersion, which are closely related to theReportstruct. It makes sense to have them all together in the Report struct itself.
I see why the version and the time and so should be part of the report. But we can do it outside this PR to keep this PR focuses.
You're welcome to open an issue with more fields you think are needed, and we will review it.
There was a problem hiding this comment.
We need to break the line (overflow-wrap: anywhere)
Also, I think putting it inside a code block will help.
| var buffer bytes.Buffer | ||
| err := tmpl.Execute(&buffer, report) | ||
| if err != nil { | ||
| log.Fatalf("failed to create HTML report with error: %v", err) |
There was a problem hiding this comment.
I think maybe use log.Errorf, because Fatalf is calling os.Exit, and maybe there are more outputs formats that still can be written.
| func getFuncMap() template.FuncMap { | ||
| return template.FuncMap{ | ||
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | ||
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, |
There was a problem hiding this comment.
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, | |
| "includeGithubSVG": func() template.HTML { return template.HTML(githubSVG) }, |
| func getFuncMap() template.FuncMap { | ||
| return template.FuncMap{ | ||
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | ||
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, | ||
| "includeLogo": func() template.HTML { return template.HTML(checkmarxLogo) }, | ||
| } | ||
| } |
There was a problem hiding this comment.
It can be declared as var instead of function:
| func getFuncMap() template.FuncMap { | |
| return template.FuncMap{ | |
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | |
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, | |
| "includeLogo": func() template.HTML { return template.HTML(checkmarxLogo) }, | |
| } | |
| } | |
| var funcMap = template.FuncMap{ | |
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | |
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, | |
| "includeLogo": func() template.HTML { return template.HTML(checkmarxLogo) }, | |
| } |
| ) | ||
|
|
||
| func writeHtml(report Report) string { | ||
| tmpl := template.Must(template.New("report").Funcs(getFuncMap()).Parse(htmlTemplate)) |
There was a problem hiding this comment.
It is new for me.
When the template.New("report").Funcs(funcMap).Parse(htmlTemplate) returns (*template.Template, error), the template.Must that accept *template.Template, error will get the multi-var return value?
Good to know!
|
|
||
| <div class="secret-info-details"> | ||
| <span><strong>ID:</strong> {{ .ID }}</span> | ||
| <span><strong>Source:</strong> {{ .Source }}</span> |
There was a problem hiding this comment.
Most of the times, the Source will be a link, but not for Git, for example.
Do you think we can make it a link when it is?
What I meant is that as you mentioned it is out of the scope of this PR |
|
@tcdsv We are working with our Product Manager to define the requirements here. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return template.FuncMap{ | ||
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | ||
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, | ||
| "includeLogo": func() template.HTML { return template.HTML(checkmarxLogo) }, |
Check failure
Code scanning / gosec
The used method does not auto-escape HTML. This can potentially lead to 'Cross-site Scripting' vulnerabilities, in case the attacker controls the input.
| func getFuncMap() template.FuncMap { | ||
| return template.FuncMap{ | ||
| "includeCSS": func() template.CSS { return template.CSS(cssTemplate) }, | ||
| "includeSVG": func() template.HTML { return template.HTML(githubSVG) }, |
Check failure
Code scanning / gosec
The used method does not auto-escape HTML. This can potentially lead to 'Cross-site Scripting' vulnerabilities, in case the attacker controls the input.
|
Hi @tcdsv, just want to update you that we are still working on the HTML output definition, and soon we will return back to you. Now when a product manager starts to look closely at this product, the HTML output exposes more out-of-scope questions, and I hope that when we finish to define the requirements for the HTML, we will get more things to improve. |
|
Status:
|
|
Sorry, closing it, I see it is not important for us now :-( |
Closes #126
first version of HTML report.
main.gohtml.goreport file and related templatesthe output of the report: https://gist.github.com/tcdsv/764dd9db5d03ca08087694cfce0a214a
The report still needs some work on how it looks and what should be in it.
Limitations associated with the Report struct
It's not easy to include the source in the summary, as the first item in the map of
Secretsneeds to be retrieved in order to check itsSourceattribute.The design of the
Reportstruct, which organizes secrets based on filenames, makes it nontrivial to generate a report with multiple sources where secrets are grouped by source type. This suggests that for supporting reports with multiple sources, a possible mapping order inReportstruct could besources->filenames->secrets.What I'm trying to clarify is whether the concept of the "Report" supports multiple sources or if it is designed to have only one source.
The
Reportstruct lacks information such as version, time started, and time ended, which cannot be included in the summary. theConfigstruct has attributes likeNameandVersion, which are closely related to theReportstruct. It makes sense to have them all together in the Report struct itself.I submit this contribution under the Apache-2.0 license.