-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow customizing html escape when export note #1935
Conversation
Did you try to export a note containing an attachment without the escaped html? I've not tried it but i would guess it is not working anymore because my attachment management code is not able to figure out which links are attachment links. |
@ZeroX-DG to be honest, with my refactoring i broke your fix again.. You could simply insert another key then the preview should work as expected .. :) i will include a fix for that again :) |
conserning your fix. I would vote against giving the option to disable the html escaping since it is a very very important security feature. input: expected: acutal: as you see the problem is that the it think we escape the html two times instead of the needed one time.. but i don't know where we do that at the moment.. |
@ehhc Oh, I see the problem there! thank you. Hmm...I also think that allow unescaped html is pretty dangerous, but I want to keep the option there to increase the "customizability" of boostnote, beside, the html is escaped by default, if the user choose to turn that feature of, he/she probably knows what he/she is doing (I hope so 😄 ). Anyway, I'll try to investigate the "double-escaped-html" issue and submit a fix right here. |
Even though i understand your argument about customization, i think giving users the possibility to do potential harmful things seems not to be a good idea in my opinion.. |
and i think we should ask @Rokt33r as well |
browser/lib/utils.js
Outdated
return lastIndex !== index | ||
? html + str.substring(lastIndex, index) | ||
: html | ||
return str |
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.
Have you tested the speed between the current and new version of the function?
From my point of view, 6 replaces is the slower solution than previous. Instead of searching the whole text (which can be big) only once it is done 6 times in a row.
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.
Yes, I've noticed that and discovered that your version seem to work fine and not the source of the problem as when I revert to your version and change the escape for the &
char to '&ssssss;'
The html exported still use &
which is quite wierd and I'll re-investigate the problem.
@nlopin @Rokt33r @ehhc , The problem is more complicated than I thought.
Sorry for the delay, I almost forgot about this PR 😄 |
After reading the issue: #1728, I think the option for customizing whether the html should be escape or not is redundant so I won't add it into the code. |
When exporting note to html they are escaped before export, if user doesn't want to escape it (#1893) Then they can customize it in export tab in setting.