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

Enable autoescape #3562

Closed
wants to merge 1 commit into from
Closed

Enable autoescape #3562

wants to merge 1 commit into from

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Jan 31, 2024

@oprypin oprypin force-pushed the escape branch 4 times, most recently from cb4dc0c to e96fd3c Compare February 1, 2024 00:23
@pawamoy
Copy link
Sponsor Contributor

pawamoy commented Feb 1, 2024

I have tested this branch locally. The only thing that seems to break is the typeset plugin from @squidfunk's Material for MkDocs:

typeset
typeset2

If I disable the plugin, the code tags are removed, leaving the title unstyled in the ToC. The span tag is also removed, and in that particular case, following Javascript code cannot insert text into it, therefore the ToC title ends up as Funding.

typeset3
typeset4

Code tags in navigation through gen-files + literate-nav continue to work, as well as code tags in table of contents through mkdocstrings' heading Jinja filter making use of data-toc-label. Search looks good too. I didn't test with emojis.

@squidfunk
Copy link
Sponsor Contributor

This is a pretty significant change that has the potential to break a lot of plugins. I'm not sure the consequences of this can be fully understood a priori. If this gets merged, please release it as MkDocs 2.0. I would not recommend doing it, as it is a departure from what we discussed in #3357.

@oprypin
Copy link
Contributor Author

oprypin commented Feb 1, 2024

@squidfunk the change matches what is now being discussed in #3357 :)

Yes I'll consider naming the release 2.0

Regarding plugins in general, I am checking my "regressions" workflow and I'll be further expanding it, to warn authors in advance or avoid changes in the first place.

Regarding the typeset plugin, I have no access to its source code, but assuming it sets nav_item.title = 'foo', it should simply set nav_item.title = markupsafe.Markup('foo') (which can be applied already, as it's forwards-compatible or whatever it's called). Or maybe with further edits that I'll soon make, even that might not be necessary.

@oprypin
Copy link
Contributor Author

oprypin commented Feb 4, 2024

So, the idea of autoescaping can work and it's nice, but reckless to try to push through considering the long historic context. Leaving the PR in a working state for posterity.

@oprypin oprypin closed this Feb 4, 2024
@mondeja
Copy link
Contributor

mondeja commented Mar 27, 2024

I see that this should be a must. Whatever page trying to add Mkdocs in a untrusted input context is totally vulnerable to XSS. Are you sure that this shouldn't be enabled by default in a breaking change release?

@squidfunk
Copy link
Sponsor Contributor

squidfunk commented Mar 27, 2024

@mondeja I kindly disagree. MkDocs pages are not subject to XSS attacks because all content is built statically. There is no threat vector that carries untrusted user input. Adding auto-escape for the sake of mitigating XSS is fixing something that is not broken. Happy to be proven wrong.

@mondeja
Copy link
Contributor

mondeja commented Mar 28, 2024

There is no threat vector that carries untrusted user input. Adding auto-escape for the sake of mitigating XSS is fixing something that is not broken. Happy to be proven wrong.

Your theme builds a feature which includes untrusted input that has been created with third party integrations, probably these kinds of limitations influenced that decision. Additionally, untrusted input could come from a server too or in the form of third party plugins building the static content from third party sources. What is Mkdocs really depends on the usage, the "is a static generator" is not true and some of the features of your theme prove that.

I'm seeing that your project has eaten up the main one. In this toxic state of the ecosystem I don't think it will contribute anything again.

@squidfunk
Copy link
Sponsor Contributor

Your theme builds a feature which includes untrusted input that has been created with third party integrations, probably these kinds of limitations influenced that decision.

I think the distinction between user (viewer) and author (writer) is important here. Only authors are able to change the content or add plugins that do. The only thing the typeset plugin effectively does is to allow to render parts of the page's content in the navigation and table of contents, nothing more. Are you maybe able to provide a demo that shows how XSS can be pulled off in a way that the author is unaware, that does only happen when the typeset plugin is active?

@squidfunk
Copy link
Sponsor Contributor

squidfunk commented Mar 28, 2024

I'm seeing that your project has eaten up the main one. In this toxic state of the ecosystem I don't think it will contribute anything again.

I'm sorry, what? Yes, Material for MkDocs is one of the biggest dependents on MkDocs, and I'm not sure I should feel sorry for building something people love to use, but I'm not seeing how this is toxic. Could you please elaborate?

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

4 participants