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

fix: shouldn't htmlEntities and htmlNumbers pass ? #130

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

signavio-fghedina
Copy link

@signavio-fghedina signavio-fghedina commented Feb 2, 2022

Just by chance I looked at this pr when was opened
and I asked myself whether or not the escapeHtml should ignore htmlEntities and htmlNumbers

this would mean no transformation for those occurrences when using markdown: true, which makes a lot of sense to my eyes

in case it actually should behave like that, this pr fixes it accordingly, changing the bare minimum.
actually that function could also be reduced to

export function escapeHtml(unsafe = ''){
  const htmlEscapes = {
    	'&': '&',
    	'<': '&lt;',
    	'>': '&gt;',
    	'"': '&quot;',
    	"'": '&#039;',
  	}
  const rxt = /&(?![\w\#]+;)|[<>"']/g
  const rx = new RegExp(rxt);
  return rx.test(unsafe)
    ? unsafe.replace(rxt, e => htmlEscapes[e])
  	: unsafe
}

please let me know your thoughts on that

@signavio-fghedina signavio-fghedina changed the title Shouldn't htmlEntities and htmlNumbers pass ? fix: shouldn't htmlEntities and htmlNumbers pass ? Feb 14, 2022
@marpme
Copy link
Contributor

marpme commented Sep 8, 2022

@signavio-fghedina is this PR still valid and needed or may it be closed?

@signavio-fghedina
Copy link
Author

signavio-fghedina commented Sep 9, 2022

@signavio-fghedina is this PR still valid and needed or may it be closed?

@marpme I think it is still valid implementation wise. Up to you, I wont mind if u close it 🤷‍♂️

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.

2 participants