Update view_finding.html

[37b]

Description

View Finding fields are all wrapped in <pre> tags, breaking markdown formatting. This PR just removes those tags and allows markdown formatting to be parsed correctly.

Test results

No changes to tests

Documentation

None

[dryrunsecurity[bot]]

DryRun Security Summary

The pull request focuses on improving the formatting and readability of the finding details in the view_finding.html template of the Dojo application by removing the <pre> tags from various sections and continuing to use the markdown_render filter to render the content stored in Markdown format.

Expand for full summary

Summary:

The changes made in this pull request are focused on improving the formatting and readability of the finding details in the view_finding.html template of the Dojo application. The <pre> tags have been removed from various sections of the template, including the "Description", "Mitigation", "Request / Response Pairs", "Impact", "Steps To Reproduce", "Severity Justification", and "References". This change will likely enhance the user experience by making the content more visually appealing and easier to read.

Additionally, the markdown_render filter is still being used to render the content, which suggests that the content is being stored in a Markdown format and needs to be rendered for proper display. From an application security perspective, the changes made in this pull request do not appear to introduce any security vulnerabilities. However, it's important to note that the overall security of the application depends on various factors, such as input validation, access control, and secure coding practices throughout the entire codebase. A comprehensive security review of the application would be necessary to identify and address any potential security issues.

Files Changed:

• dojo/templates/dojo/view_finding.html: The changes in this file are focused on improving the formatting and readability of the finding details. The <pre> tags have been removed from various sections of the template, including the "Description", "Mitigation", "Request / Response Pairs", "Impact", "Steps To Reproduce", "Severity Justification", and "References". The markdown_render filter is still being used to render the content, which suggests that the content is being stored in a Markdown format and needs to be rendered for proper display.

Code Analysis

We ran 9 analyzers against 1 file and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer	Findings
Configured Codepaths Analyzer	1 finding

Riskiness

:red_circle: Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

[Maffooch]

@37b I'm curious if this could be a browser issue. Markdown appears to be rendering fine on version 2.38.1 of dojo

Chrome:

Safari:

[37b]

@37b I'm curious if this could be a browser issue. Markdown appears to be rendering fine on version 2.38.1 of dojo

Chrome:

Safari:

@Maffooch

Did you notice those are all monospaced fonts? It should render like this, but the <pre> tags are colliding with the markdown renderer.

Instead it renders like this...which is ugly and mangles some markdown formatting.

[Jon-the-2nd]

It seems this is a CSS issue rather than a markdown to HTML issue. The HTML code that is generated reflects the initial markdown but then the CSS makes it all look the same again.

I found that tweaking the dojo.css file a little bit goes a long way in making the fields on the 'finding' page more readable. Here's what I added at the bottom of dojo.css:

.panel-body {
      background-color: #eaf2f5;
  }
  
  .panel-body h1,h2,h3,h4,h5,h6,p,pre {
      font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
  }
  
  .panel-body hr {
      border: 1px solid #bcdce8;
  }
  
  .panel-body blockquote {
      margin: 0;
      border-left: 5px solid #bcdce8;
      padding: 0 10px;
  }
  
  .panel-body blockquote>p {
      font-size: 13px;
  
  }
  
  .panel-body pre {
      line-height: 1;
  }
  
  .panel-body p {
      margin: 0;
  }
  
  .codehilite {
      background-color: #ccd0d3 !important;
      border: 1px solid #8f8686 !important;
      padding: 10px !important;
      margin: 0 !important;
      color: #d96966;
  }
  
  .panel-body code {
      color: #d96966;
      line-height: 1.2;
  }

Now this may have an impact on other pages in the application, but I haven't gotten around to that yet.

Additional issues

I've run into two additional but related problems:

1. The 'Request' and 'Response' fields don't seem to support markdown at all, yet they have the same WYSIWYG editor. This is quite confusing when manually adding a finding as you can spend some time adding markdown to the text input, then when you save it it doesn't render as expected.
2. The behaviour when creating a new finding (Add New Finding from the Findings menu) is slightly different. In the editor the 'Code' button is missing and a 'Preview' button is added instead. Now it seems that both buttons have keyboard shortcuts which can still be used even if the button isn't displayed, but for instance in the 'Edit Finding' page when typing text in a field and hitting Ctrl-P will display the 'preview', however if you go to another field and hit Ctrl-P again it open a 'print' dialog rather than disabling the preview.
3. The 'preview' mode has different styles applied than what is actually rendered when saving the finding.

Behaviour seems to be the same on DefectDojo versions 2.37.0 and 2.38.1

[37b]

It seems this is a CSS issue rather than a markdown to HTML issue. The HTML code that is generated reflects the initial markdown but then the CSS makes it all look the same again.

I found that tweaking the dojo.css file a little bit goes a long way in making the fields on the 'finding' page more readable. Here's what I added at the bottom of dojo.css:

.panel-body {
      background-color: #eaf2f5;
  }
  
  .panel-body h1,h2,h3,h4,h5,h6,p,pre {
      font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
  }
  
  .panel-body hr {
      border: 1px solid #bcdce8;
  }
  
  .panel-body blockquote {
      margin: 0;
      border-left: 5px solid #bcdce8;
      padding: 0 10px;
  }
  
  .panel-body blockquote>p {
      font-size: 13px;
  
  }
  
  .panel-body pre {
      line-height: 1;
  }
  
  .panel-body p {
      margin: 0;
  }
  
  .codehilite {
      background-color: #ccd0d3 !important;
      border: 1px solid #8f8686 !important;
      padding: 10px !important;
      margin: 0 !important;
      color: #d96966;
  }
  
  .panel-body code {
      color: #d96966;
      line-height: 1.2;
  }

Now this may have an impact on other pages in the application, but I haven't gotten around to that yet.

Additional issues

I've run into two additional but related problems:

1. The 'Request' and 'Response' fields don't seem to support markdown at all, yet they have the same WYSIWYG editor. This is quite confusing when manually adding a finding as you can spend some time adding markdown to the text input, then when you save it it doesn't render as expected.
2. The behaviour when creating a new finding (Add New Finding from the Findings menu) is slightly different. In the editor the 'Code' button is missing and a 'Preview' button is added instead. Now it seems that both buttons have keyboard shortcuts which can still be used even if the button isn't displayed, but for instance in the 'Edit Finding' page when typing text in a field and hitting Ctrl-P will display the 'preview', however if you go to another field and hit Ctrl-P again it open a 'print' dialog rather than disabling the preview.
3. The 'preview' mode has different styles applied than what is actually rendered when saving the finding.

Behaviour seems to be the same on DefectDojo versions 2.37.0 and 2.38.1

All I did was remove the preformatted text tags and all looks fine now. You can try it in dev tools to see it without rebuilding. Even if there are alternate methods to improve the visuals, it doesn't make sense to wrap everything in <pre> tags.

[Jon-the-2nd]

All I did was remove the preformatted text tags and all looks fine now. You can try it in dev tools to see it without rebuilding. Even if there are alternate methods to improve the visuals, it doesn't make sense to wrap everything in \<pre\> tags.

It does look better indeed without the pre tags. I think it's still a bit confusing that there's a difference between the way it is rendered by the 'preview' button and the way it does when the finding is saved though.

What about the request/response fields, aren't these supposed to support markdown too? The thing is if you remove the pre tags there, everything just gets wrapped and become unreadable.

[37b]

All I did was remove the preformatted text tags and all looks fine now. You can try it in dev tools to see it without rebuilding. Even if there are alternate methods to improve the visuals, it doesn't make sense to wrap everything in \<pre\> tags.

It does look better indeed without the pre tags. I think it's still a bit confusing that there's a difference between the way it is rendered by the 'preview' button and the way it does when the finding is saved though.

What about the request/response fields, aren't these supposed to support markdown too? The thing is if you remove the pre tags there, everything just gets wrapped and become unreadable.

@Jon-the-2nd

I'm not following. What is the preview button you're referring to?

This pr removes <pre> from all the relevant fields in the dom so markdown formatting works as intended everywhere.

If the request/response pairs should be rendered in monospace fonts, then we should use either markdown for that, or just leave the <pre> tags on these fields, but that would mean if there were markdown it may fail to render correctly. Given these fields are strictly "code" I think that's a fair trade off.

Are there other fields that should only ever be monospaced?

@Maffooch thoughts?

[Maffooch]

Ahh okay I see. It looks the issue is that there is not a differentiation between

• code snippets
• not code snippets

That is is the biggest difference I can see between the font

The monospace font used here has been around since I got involved in the project 😅 It's just normal dojo to me

[37b]

It seems this is a CSS issue rather than a markdown to HTML issue. The HTML code that is generated reflects the initial markdown but then the CSS makes it all look the same again.

I found that tweaking the dojo.css file a little bit goes a long way in making the fields on the 'finding' page more readable. Here's what I added at the bottom of dojo.css:

.panel-body {
      background-color: #eaf2f5;
  }
  
  .panel-body h1,h2,h3,h4,h5,h6,p,pre {
      font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
  }
  
  .panel-body hr {
      border: 1px solid #bcdce8;
  }
  
  .panel-body blockquote {
      margin: 0;
      border-left: 5px solid #bcdce8;
      padding: 0 10px;
  }
  
  .panel-body blockquote>p {
      font-size: 13px;
  
  }
  
  .panel-body pre {
      line-height: 1;
  }
  
  .panel-body p {
      margin: 0;
  }
  
  .codehilite {
      background-color: #ccd0d3 !important;
      border: 1px solid #8f8686 !important;
      padding: 10px !important;
      margin: 0 !important;
      color: #d96966;
  }
  
  .panel-body code {
      color: #d96966;
      line-height: 1.2;
  }

Now this may have an impact on other pages in the application, but I haven't gotten around to that yet.

Additional issues

I've run into two additional but related problems:

1. The 'Request' and 'Response' fields don't seem to support markdown at all, yet they have the same WYSIWYG editor. This is quite confusing when manually adding a finding as you can spend some time adding markdown to the text input, then when you save it it doesn't render as expected.
2. The behaviour when creating a new finding (Add New Finding from the Findings menu) is slightly different. In the editor the 'Code' button is missing and a 'Preview' button is added instead. Now it seems that both buttons have keyboard shortcuts which can still be used even if the button isn't displayed, but for instance in the 'Edit Finding' page when typing text in a field and hitting Ctrl-P will display the 'preview', however if you go to another field and hit Ctrl-P again it open a 'print' dialog rather than disabling the preview.
3. The 'preview' mode has different styles applied than what is actually rendered when saving the finding.

Behaviour seems to be the same on DefectDojo versions 2.37.0 and 2.38.1

Sorry, I am rereading this and getting a better understanding. I think adding markdown support in the request/response is easy enough, but personally I don't think it should be used in these sections as they should be purely for request/response pairs and nothing else. Instead, the form should remove the editor buttons or have some other indicator that these fields do not support formatting.

I can't remember the last time I created a finding manually, but I suspect the reason the preview looks different to the saved finding is because of the <pre> tags, which are not considered when the JS fires. This PR should fix that bit at least.

[Jon-the-2nd]

@37b, thanks for clarifying. One question though about the request/response pairs: that notion of pairs and the fact that when viewing it says Request #1 and Response #1 makes it appear like it's possible to add multiple such pairs, but I can't seem to find a way to do that. Would you happen to know how that works?

[mtesauro]

@Jon-the-2nd Generally tool parsers are the ones that create multiple request/response pairs. Burpsuite is an example of this.

Typically, manual testers who are manually adding findings only include a single request/response pair which is why the UI was created with only a single pair available.

I believe but haven't checked that additional pairs can be added via the API.

[37b]

@Jon-the-2nd Generally tool parsers are the ones that create multiple request/response pairs. Burpsuite is an example of this.

Typically, manual testers who are manually adding findings only include a single request/response pair which is why the UI was created with only a single pair available.

I believe but haven't checked that additional pairs can be added via the API.

@mtesauro Do you have an opinion on improving the typography and/or making the distinction between code samples or pre-formatted text and standard text?

[Maffooch]

Something that we have not considered is that the pre tags are used for maintaining spacing. The fear is that tool output in DefectDojo could appear radically different following this change. We don't know what assumptions are made by parsers, and checking all 180+ of them is a big lift