Fix incorrect escaping of site_name, remove some title attributes #1117
Conversation
| @@ -30,7 +30,7 @@ | |||
| <!-- Link to home --> | |||
| <div class="md-flex__cell md-flex__cell--shrink"> | |||
| <a href="{{ config.site_url | default(nav.homepage.url, true) | url }}" | |||
| title="{{ config.site_name }}" | |||
| title="{{ config.site_name | e }}" | |||
There was a problem hiding this comment.
Doesn't this lead to the same problem, i.e. if the site name contains special characters like & and " that they get (double) escaped? Have you tested this?
There was a problem hiding this comment.
I've tested this, and site_name isn't escaped at all, it's just copied from the config file directly. This appears to be the same for all config variables.
I also found that the copyright config variable isn't escaped, but I left it as you may want to enter HTML there - it isn't escaped in any of the default themes, so I assume it is supposed to be HTML.
There was a problem hiding this comment.
Ok, good. Is config.site_name escaped in the any of the default themes? Are any variables escaped in default themes? Are there maybe other variables to consider escaping? Just want to "catch 'em all".
There was a problem hiding this comment.
I had another look through all of mkdocs-material, and found a couple more areas where escaping is needed. I'll go through the default themes and see if they have any issues as well.
| {%- endif -%} | ||
|
|
||
| <!-- Redirect --> | ||
| {%- if page and page.meta and page.meta.redirect -%} | ||
| <script> | ||
| var anchor = window.location.hash.substr(1) | ||
| location.href = '{{ page.meta.redirect }}' + | ||
| location.href = {{ page.meta.redirect | tojson | safe }} + |
There was a problem hiding this comment.
What exactly is happening here? I've been testing your changes but couldn't come up with a case to test drive this.
There was a problem hiding this comment.
I was going through the mkdocs website and saw the tojson filter: https://www.mkdocs.org/user-guide/custom-themes/#tojson and decided to use it. It probably isn't much of an issue, as you wouldn't want to use single quotes in a URL anyway, but it might fix some edge cases.
src/base.html
Outdated
| @@ -306,7 +306,7 @@ | |||
| as the main headline. | |||
| --> | |||
| {%- if not "\x3ch1" in page.content -%} | |||
| <h1>{{ page.title | default(config.site_name, true)}}</h1> | |||
| <h1>{{ page.title | default(e(config.site_name), true)}}</h1> | |||
There was a problem hiding this comment.
Doesn't work for me, I tried and think it should be {{ page.title | default(config.site_name | e, true) }}
There was a problem hiding this comment.
You are correct, I forgot to test that.
|
Regarding the removal of the |
|
I'm not sure how to proceed here. I did some research and the default themes don't escape the variables at all, see for example: Of course, escaping is necessary in attributes because of quotes and in text nodes because of Furthermore, I would want a opinion of the MkDocs team and a clear strategy before proceeding with this PR. We need to tackle those issues systematically. Do you want to take the lead on this and open an issue on the MkDocs tracker for discussion? |
|
Apologies for the delay. I think I didn't escape the references that were in I'll open an issue on the MkDocs tracker shortly as you have suggested, to discuss these issues with the MkDocs team. |
|
I followed the discussion on the MkDocs issue tracker and came to the following conclusion: I think escaping of configuration attributes should be left to the user. If we start escaping template variables, we must escape all variables to be consistent. However, if we do, it won't be possible to add HTML to configuration options like some people do with the Metadata extension, e.g. for custom heros (which I didn't know was possible before I've seen it). I think this is pretty cool and don't want to take this away from the user. Users often ask for more configuration options, but better than providing options is giving them degrees of freedom like for example adding your own hero HTML. This PR would reduce this kind of freedom. Nevertheless, we should definitely add a block on escaping to the Getting Started guide. Happy to merge PRs. |
|
Closing for the reasons stated. I want to thank you again for taking the time to investigate this issue and facilitating the discussion with the guys over at MkDocs, but I think it doesn't make any sense progressing further down this way. |
This ensures that config.site_name is escaped correctly in all places, while also removing the title attributes discussed in #1100.