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

Allow customizing html escape when export note #1935

Merged
merged 12 commits into from
Jul 17, 2018

Conversation

ZeroX-DG
Copy link
Member

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.

image

@ehhc
Copy link
Contributor

ehhc commented May 20, 2018

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.
Could you try it?

@ZeroX-DG
Copy link
Member Author

ZeroX-DG commented May 20, 2018

@ehhc Well, good news ! it's still works.
image

However, the old bug "image-not-show-when-drop" occurred again (#1822). And of course the image displayed again when I toggle the mode.

@ehhc
Copy link
Contributor

ehhc commented May 20, 2018

@ZeroX-DG to be honest, with my refactoring i broke your fix again..
i also opened a PR for it but it was closed "since it is already fixed".. -.-

You could simply insert another key then the preview should work as expected .. :)

i will include a fix for that again :)

@ehhc
Copy link
Contributor

ehhc commented May 20, 2018

conserning your fix. I would vote against giving the option to disable the html escaping since it is a very very important security feature.
Btw: the problem of #1893 is not that the html get escaped but that it is down in a wrong way.

input:
"}, "2":{"

expected:
<pre><code>&quot;}, &quot;2&quot;:{&quot; </code></pre>

acutal:
<pre><code>&amp;quot;}, &amp;quot;2&amp;quot;:{&amp;quot;</code></pre>

as you see the problem is that the & is wrongly escaped as well.. that's the problem.. not the escaping itself

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..

@ZeroX-DG
Copy link
Member Author

@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.

@ehhc
Copy link
Contributor

ehhc commented May 20, 2018

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..

@ZeroX-DG
Copy link
Member Author

@ehhc I fixed the double escaped html. We gonna need to vote whether the option should be there or not 😄 @kazup01 What do you think?

@ehhc
Copy link
Contributor

ehhc commented May 20, 2018

and i think we should ask @Rokt33r as well

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label May 21, 2018
@sosukesuzuki sosukesuzuki self-requested a review May 21, 2018 06:13
return lastIndex !== index
? html + str.substring(lastIndex, index)
: html
return str
Copy link
Contributor

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.

Copy link
Member Author

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 '&ampssssss;' The html exported still use &amp; which is quite wierd and I'll re-investigate the problem.

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jun 4, 2018
@ZeroX-DG
Copy link
Member Author

ZeroX-DG commented Jul 4, 2018

@nlopin @Rokt33r @ehhc , The problem is more complicated than I thought.

Sorry for the delay, I almost forgot about this PR 😄

@ZeroX-DG
Copy link
Member Author

ZeroX-DG commented Jul 4, 2018

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.

@Rokt33r
Copy link
Member

Rokt33r commented Jul 6, 2018

@Rokt33r After merging, a sanitizing method for code fences must be replaced with this escape method for better performance. #2191
(this is a reminder for me)

@Rokt33r Rokt33r removed the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Jul 17, 2018
@Rokt33r Rokt33r merged commit b18a09e into BoostIO:master Jul 17, 2018
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

5 participants