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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option for disabling escaping HTML reserved characters #74

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

Taitava
Copy link
Contributor

@Taitava Taitava commented Mar 19, 2023

Hi,

I'd like to propose creating an option named escape_html that defaults to true, but can be turned off.

Background: I'm adopting this library for Shell commands plugin for Obsidian. Obsidian.md is a knowdge base/note taking application, and my plugin provides an ability to execute shell commands via hotkeys within Obsidian. One feature is that output received from shell commands can be utilized in Obsidian, for example by inserting the output to a currently open note.

As Obsidian uses the Markdown format for notes, it can display HTML content really well, so I'd like to combine being able to convert ANSI code to HTML, and also retain any possible HTML that might be outputted by user-specified shell commands.

Thank you for considering this! 馃檪 This is just a proposal, and I'm willing to make changes if needed.

P.S. There are four commits in this PR, but they can be squashed to one. I was first thinking about making the href="" attribute not to be affected by this option for hyperlinks (i.e. it would always escape href`` value even if the option is false). But I changed my mind, as I came to think about that &should not be escaped in urls as they might be part of a query string. So now settingescape_html` to false prevents all escaping. I'm not 100% sure about this decision, so please let me know if you have any opinions. 馃檪

@drudru
Copy link
Owner

drudru commented Mar 20, 2023

Thank you. I will review this today.

@drudru
Copy link
Owner

drudru commented Mar 21, 2023

Ok, Thanks for your PR.

I actually had escape_html as a boolean for a long time.
However, I introduced a security vulnerability in version 4.

This is covered in CVE-2021-3377.

At the time, I thought I would just make things simple and always escape the HTML for safety.

This was done in the v5 release of ansi_up.

After skimming over your code, I think your change is probably safe.
You do not have to change anything at this point.

I just want to revisit the old code, the security fix, and your changes before merging.

@Taitava
Copy link
Contributor Author

Taitava commented Mar 21, 2023

Thanks! 馃檪 Take your time. 馃檪

@drudru drudru merged commit 4177b16 into drudru:master Mar 31, 2023
@drudru
Copy link
Owner

drudru commented Mar 31, 2023

Thank you. A release with these changes will come soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants