Warren noticed this the other day

I checked in a change so that js's original test case no longer causes problems. Now the "Recent" link on the menu bar omits the lime background color. But my change from last night is insufficient from the general case. There are still many ways to sneak anti-social CSS into a Markdown-formatted wiki page or forum post.

I looked at the Commonmark spec for embedded HTML today. Wow - what a mess. I thought the whole point of Markdown was that it was suppose to be simpler than things like raw HTML. Not so. Take a look at the rules for HTML in Commonmark. Sometimes inline markup within an HTML block is rendered, and other times not. There are 7 distinct cases that you have to remember and understand.

In any event, Natacha Porté's Markdown implementation used by Fossil appears to be based on John Gruber’s original Markdown.pl implementation and hence does not implement all of the byzantine rules of Commonmark. And I'm not sure that is worth fixing.

Contrast the commonmark HTML rules with the rules for including HTML in Fossil-wiki. Fossil-wiki has a white-list of supported ("safe") HTML markup. The HTML can occur anywhere. The formatter automatically takes care of adding omitted close elements.

What to do about Markdown

I think there are basically two options for fixing this:

1. Major brain surgery on the existing Markdown formatter, so that it can enforce limitations on embedded HTML, and add missing close-elements.

2. Leave the Markdown formatter unchanged (perhaps even undoing my change from last night) and add a second pass that goes over the generated HTML to remove problematic elements and insert missing close tags.

I'm not sure which approach is better here.

I'm not sure which approach is better here.

i had an idea about this last night which might resolve our forum-side markdown tag pain without too much grief/effort:

When saving a post, count the number of openers and closers for each tag type. Don't check that they're properly nested or make semantic sense, just ensure that for each <FOO ...> we have a </FOO> (ignoring case, of course, and not even caring whether FOO is a genuine HTML tag name or not). If those counts are not equal, refuse to save, pointing out that there's a tag count mismatch. There would necessarily be some exceptions for self-closing tags like BR, HR, IMG, INPUT (not that INPUT should be allowed at all in markdown, though).

That doesn't solve the <STYLE> tag case (which we could arguably elide altogether in forum output, or convert <style>...</style> to <!--style>...</style-->), but it would eliminate the troubling missing </TEXTAREA> tag and similar cases.

There's one case i can see this breaking: weird constructs like 1<4>3, where this hypothetical checker would confirm that there's a </4> tag. That seems like a negligible risk, though, with an easy workaround for users (add a space before the '4').

For robustness, I think it needs to do a second pass over the output, with full HTML parsing. No compromises or short-cuts. Attackers will exploit compromises and short-cuts. The second pass should:

1. Omit any markup that is not on the white-list. Perhaps the white-list should be expanded to include some of the newer HTML5 elements that have been added since the original formatter was written.

2. Omit any HTML element attributes not specifically allowed by the white-list. Without this step, an attacker would be able to add id= tags that could interfere with the Javascript at the bottom of the page.

3. Omit any extra end-tags. For example, we probably do not want allow </body> in a forum post. Or, for that matter, an extra </div> that would close the division that contains the post early.

4. Insert all missing end-tags so that the tags are strictly nested.

For robustness, I think it needs to do a second pass over the output

Could that not be done in the first pass, before any generated markup is added? If done in the second pass, the amount of effort for the parser (as opposed to development effort) jumps considerably as we've already emitted lots of new markup. So: POST ==> tidy-up filter ==> parser (ummodified)

This sounds like a good idea. But should 4 really be done, or is it better to refuse the input? I think in 99% of the cases, someone is just forgetting to close a tag and not someone malicious and they would appreciate the hint.

How should markdown that is coming from the command line tool be handled?

How should markdown that is coming from the command line tool be handled?

Markdown via the wiki and embedded docs (which are all handled via the same underlying routines) should, IMO, not be affected by this. We've had markdown since 2012 or 13 or so and it's only proven to be remotely problematic since the introduction of the forum, where random people can drop random content into it. (Even then, somewhat surprisingly, it actually took about a year and a half for it to ever even get recognized as a genuine problem, through a few posts which were innocently missing closing tags of one sort or another.)

I think those ideas are good one, both in forum and in wiki (unless the administrator has enabled use of arbitrary HTML in wiki). However, additionally I think that:

• Inline CSS should be disabled.

• If a CSS class is specified, it must be one of the whitelisted classes (allowing the administrator to set the list of whitelisted CSS classes) or else it is disallowed. However, any CSS class whose name begins with "language-" and only consists of whitelisted characters should be permitted, even if the CSS class name is not whitelisted.

• There are some commands which may be useful in wiki but possibly should not be allowed in forums, such as <NAV> (a forum post isn't a set of navigation links) and <ARTICLE> (the forum post is already a article; it shouldn't be necessary to make a nested article).

If a CSS class is specified, it must be one of the whitelisted classes (allowing the administrator to set the list of whitelisted CSS classes) or else it is disallowed.

FWIW, that's a level of micro-management which i would really hate to see fossil try to impose. It's completely onerous to those who develop docs in fossil: if they're an admin, they have to go whitelist every class they try to use and if they're not an admit they have to go ask an admin to do so.

The policy could not be versionable because that would allow any user to bypass it locally and whitelist what they want, then push those changes back to the server (keeping in mind that they can edit docs, which means they have commit access). Thus only an admin on the central-most copy of the repo would be able to whitelist anything. That would be an utter tragedy in terms of usability/flexibility, IMO.

FWIW, that's a level of micro-management which i would really hate to see fossil try to impose. It's completely onerous to those who develop docs in fossil: if they're an admin, they have to go whitelist every class they try to use and if they're not an admit they have to go ask an admin to do so.

You could have an option to disable that restriction if wanted, I suppose. However, I would find it useful, since I want to avoid having too much CSS in general (actually, if it was up to me, I would have no CSS at all); the user can specify their own CSS if wanted and ignore that on the server. The end user's CSS code should be able to distinguish what context something is, too (although this could also be done by writing more complicated selectors, I suppose, too).

Code to fix this (and hopefully also the unclosed tag issue previously identified) should now be on trunk, and on the main Fossil website. Please try to break it. Report any success or failure here. Thanks.

Looks like this will "eat" the blacklisted tags.

Maybe it should escape them and show as part of text instead (either preformatted or html-escape the angle brackets, after all it's user input?

FWIW GitHub seems to just print such tags as text.

OK. It is a simple modification to show the elided tags a plain text. But I took this one step further: I enclose the elided tags within an error <span>, so that they are more readily apparent. I don't know if this is a good idea or not, but it seems like something worth trying out.

Example #1: js's original test case. <style>body{background-color:lime;}</style>

Example #2: Surplus end-tags </div></body>

Another possibility is to only use the error <span> in the Preview and just show the elided tags as plain text if they do get committed. That will involve an extra "flags" parameter that will need to get passed down through the formatter routines, to let the HTML sanitizer know whether or not it is suppose to generate the error <span>s. So it is more work, but quite doable.

Yet another possibility: Use the error <span> on Preview but also refuse to commit a forum post or wiki edit that has any HTML errors. Force the user to fix the error in order for their post to go in.

... but also refuse to commit a forum post or wiki edit that has any HTML errors

Harsh but fair.

We could maybe make use of these warnings in the wiki/fileedit markdown preview, too, but without a cannot-commit restriction imposed on it. Once you're done i'll look at adding that (presumably a flag needs to be added to the relevant calls to pass on that preference).

But I took this one step further:...

That is a thing of beauty. Whether or not to show those only in preview mode is a tough call. It would arguably be better for arbitrary forum-goers if some broken markup outside of their control is not trying to steal their attention with bright colors, though, which argues for only showing the error tags in preview mode.

If Preview renders differently from the default display, is it still really "Preview"?

If Preview renders differently from the default display, is it still really "Preview"?

LOL! i'm not foolish enough to enter into a battle of philosophy against you ;). That's a fair question, though.

<script>body{background-color:lime;}</script>

PS: that should be "style" instead of script.

I'd prefer this to be shown verbatim, no coloring.

It's Markdown, not HTML validator. Anything allowed is rendered in HTML, the rest is just text, or ?code.

This way the Preview is just as what would be commited.

The new formatter is not working quite right. See the four error tokens in markdown.md.

The new formatter is not working quite right. See the four error tokens in markdown.md.

But it really should only be enabled for the forum, shouldn't it? For embedded docs and wiki pages this whole thing is a non-problem.

Some Fossil repos allow public wiki additions, in the spirit of Wikipedia.

Some Fossil repos allow public wiki additions, in the spirit of Wikipedia.

Mis-markup in a wiki or embedded doc page doesn't really break anything except that page (and anyone allowing public edits deserves what they get - i've had fossil repos gets spammed by bots that way when i quite inadvertently allowed anonymous users to edit their wikis). The forum content is another matter because each unit is no longer atomic, but can have side effects on a whole thread.

IMO trying to police the contents of wiki pages and embedded docs for well-formedness is a dark, slippery path.