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.

Sign up for GitHub

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

Jump to bottom

Get rid of unnecessary safeHTML usage #135

Merged
merged 36 commits into from
May 14, 2020
Merged

Get rid of unnecessary safeHTML usage #135

merged 36 commits into from
May 14, 2020

Conversation

palant
Copy link
Collaborator

@palant palant commented May 12, 2020 •
edited
Loading

This change fixes #134 for a single template - the only safeHTML call required here is moved into the new utils/icon.html partial. While at it, a few things are simplified:

  • It's easier to emit a delimiter before any non-first list item than after any non-last item.
  • Parsing urlized categories as URLs is unnecessary - these aren't turned into full URLs, merely non-letter characters are being replaced.

@palant palant changed the title Simplify post-meta.html template logic Get rid of unnecessary safeHTML usage May 12, 2020
@palant
Copy link
Collaborator Author

palant commented May 12, 2020 •
edited
Loading

Ok, let's morph this pull request into a generic "Get rid of unnecessary safeHTML usage" - I'll add more commits to it for the other templates. The newer commits rely on the icon.html template anyway.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

Note that the changes to feed generation get rid of CDATA - Hugo has no proper way to escape CDATA content, and the current approach is slightly broken. If the contents of some page happen to contain ]]> it will break the feed.

@reuixiy
Copy link
Owner

reuixiy commented May 13, 2020

Wow, thanks for the fix and reminder!

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

I removed emitting whitespace as delimiter in minimal-footer-about.html - this is unnecessary, there is a newline between the links which has the exact same effect already.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

Note that the markup generated by post-nav component was actually invalid - < and > (the arrows) needed to be escaped.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

Same issue was on home-posts.html - < and > weren't escaped. I also had to make larger changes to summary.html, so that it returns markup (which is automatically marked as safe HTML) rather than put a string into scratch.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

I had to change image autodetection regex in order to test the previous change properly - your regex doesn't allow for any additional attributes between <img and src= (I have a class attribute there). I can separate that change into another pull request if you prefer.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

I think I'm done here. Remaining safeHTML usage:

  • Feeds: it appears to be impossible to render an XML processing instruction without it.
  • partials/footer.html: busuanzi code unchanged because I cannot test it.
  • partials/components/post-gitinfo.html: unchanged because I cannot test it.
  • partials/utils/toc.html, partials/utils/markdownify.html, partials/utils/icon.html, partials/utils/content.html: it's actually necessary here.

@palant
Copy link
Collaborator Author

palant commented May 13, 2020

I had to change image autodetection regex in order to test the previous change properly - your regex doesn't allow for any additional attributes between <img and src= (I have a class attribute there). I can separate that change into another pull request if you prefer.

Actually, I reverted the change to the image autodetection regex - it's not worth it. The majority of users shouldn't have this issue, and I can change the order of attributes on my end.

@reuixiy
Copy link
Owner

reuixiy commented May 13, 2020

Awesome! 🎉 I’ll review this as soon as possible! Thank you for this great contribution!

reuixiy
reuixiy requested changes May 14, 2020
View reviewed changes
Copy link
Owner

@reuixiy reuixiy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned a lot! And here are some issues I want to mention.

layouts/index.sectionsatom.xml Show resolved Hide resolved
layouts/index.sectionsrss.xml Show resolved Hide resolved
layouts/partials/components/minimal-footer.html Outdated Show resolved Hide resolved
layouts/partials/components/minimal-footer.html Outdated Show resolved Hide resolved
layouts/partials/components/minimal-footer.html Outdated Show resolved Hide resolved
layouts/partials/components/minimal-footer.html Outdated Show resolved Hide resolved
layouts/partials/components/post-meta.html Outdated Show resolved Hide resolved
layouts/partials/components/post-tags.html Outdated Show resolved Hide resolved
@palant
Copy link
Collaborator Author

palant commented May 14, 2020

I checked the source code and anchorize isn't a good alternative - it follows a different logic which depends on the Markdown parser. So url.Parse() it is, I implemented the necessary changes.

Btw, it seems that partials/pages/tag-cloud.html and partials/pages/taxonomy.html do is incorrectly right now - these don't even call urlize and use the tag name directly, which has its own set of issues (e.g. whitespace in tag name).

@palant palant requested a review from reuixiy May 14, 2020 07:48
reuixiy
reuixiy reviewed May 14, 2020
View reviewed changes
Copy link
Owner

@reuixiy reuixiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@reuixiy
Copy link
Owner

reuixiy commented May 14, 2020 •
edited
Loading

Btw, it seems that partials/pages/tag-cloud.html and partials/pages/taxonomy.html do is incorrectly right now

Yes. I guess it may be because taxonomy terms in these two partials came from .Site.Taxonomies and .Data.Terms instead of .Params. The different behaviors between them does cause a lot of trouble.

@reuixiy
Copy link
Owner

reuixiy commented May 14, 2020 •
edited
Loading

Seems everything's good now. I'll complete the following two todos,

  • partials/footer.html: busuanzi code
  • partials/components/post-gitinfo.html

then I'll merge this PR.

@palant palant mentioned this pull request May 14, 2020
Merged
@reuixiy reuixiy merged commit 91e84a1 into reuixiy:master May 14, 2020
ulmefors pushed a commit to ulmefors/hugo-theme-meme that referenced this pull request Nov 22, 2020
@palant @reuixiy @ulmefors
fix: Get rid of unnecessary safeHTML usage (reuixiy#135)
9967bf8 
Fixes reuixiy#134.

* Simplify post-meta.html template logic

* Fixed indentation in previous commit

* Use icon.html template for zodiac icons in lists

* Remove unnecessary safeHTML usage in footer.html (busuanzi functionality unchanged, cannot test)

* Improved previous commit, no unnecessary markup for the copyright symbol

* Remove unnecessary safeHTML usage in style.html

* Use icon.html partial in the header

* Remove unnecesary safeHTML usage in script.html

* Removed unnecessary safeHTML usage in menu.html

* Remove unnecessary safeHTML use in header.html

* Use icon.html partial in back-to-top and port-share components

* Use icon.html partial in related-posts and socials components

* Remove unnecessary safeHTML use in feed generation

* Simplify minimal-footer-about component

* Removed safeHTML usage from post-updated-badge component

* Remove unnecessary safeHTML usage in post-copyright component

* Remove unnecessary safeHTML usage in post-nav component

* Simplify minimal-footer component and remove safeHTML usage

* Simplify post-tags component and remove safeHTML usage

* Made summary.html produce its result directly rather than via scratch, removed unnecessary safeHTML usage in home-posts.html

* Use icon.html partial on home-footage page

* Remove unnecessary safeHTML usage on post.html page

* Remove unnecessary safeHTML usage on home-poetry.html page

* Remove unnecessary safeHTML usage from OpenGraph data

* Remove unnecessary safeHTML usage from utils/list-item.html

* Improve image autodetection regex and remove unnecessary unsafeHTML usage in images.html

* Remove unnecessary safeHTML usage from relative-url.html

* Remove unnecessary safeHTML usage in content.html

* Revert image auto-detection changes, these aren't useful

* Delete the missing line in post-share

* Revert to using urls.Parse on tag and category names passed to .GetPage again

* Fix error caused by wrong context in list.html

* Replace hyphen with en dash

* Remove unnecessary safeHTML usage in post-gitinfo.html

* Remove unnecessary safeHTML usage of busuanzi in footer.html

Co-authored-by: reuixiy <reuixiy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reuixiy reuixiy reuixiy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

safeHTML use should be avoided
2 participants
@palant @reuixiy