Skip to content

Remove custom strikethrough in preparation of po5's markdown security fix - #30

Open
xEobardThawne wants to merge 2 commits into
liamw1:masterfrom
xEobardThawne:master
Open

Remove custom strikethrough in preparation of po5's markdown security fix#30
xEobardThawne wants to merge 2 commits into
liamw1:masterfrom
xEobardThawne:master

Conversation

@xEobardThawne

@xEobardThawne xEobardThawne commented Aug 15, 2025

Copy link
Copy Markdown

Back when I first coded descriptions, before they were fully implemented (thanks for adding and improving them btw), I was adding descriptions to my posts, which sometimes included the tilde (~) key. I quickly realized that a custom markdown feature was added using the tilde to make strikethroughs. This would be fine, if it wasn't so buggy. Normally you'd expect a feature like that to be cancel-able by adding a backslash (\) key before the tilde. Well I tried doing that, to find out that while it worked, it didn't actually remove the backslashes for some reason? Today, I looked into it further, and found out that the previous developer had added a piece of code to stop backslashes from being removed. Turns out when you remove that code, it just stops the backslashes from doing anything at all, and it deletes them. So basically, the backslashes either work and stay in the text, or they do literally nothing and get removed from the text regardless.

This mess of a feature has led me to the conclusion that it would be best if it was removed all together, especially when there's something else that does the exact same thing. it would just be readded in a better way in future versions.

Also check out @po5's comment down below.

@xEobardThawne

Copy link
Copy Markdown
Author

I should probably add that I use Firefox, not Google Chrome. I don't think there would be much of a difference, but just in case it might be useful to know.

@po5

po5 commented Aug 21, 2025

Copy link
Copy Markdown

I explain why this was added (not supported in marked.js at the time) and why it's no longer necessary (feature is now built into marked.js) in this commit message po5@5f7346e
And note that you should not use <del>. You're not meant to be able to use arbitrary HTML tags, it's an oversight in szurubooru and possible security issue. This was fixed in https://github.com/rr-/szurubooru/pull/733 and will stop working when oxibooru ports the fix over, use tildes like you normally would instead. They can be escaped as you'd expect after removing this now-unnecessary code.

TagPermalinkFixWrapper is also unnecessary and is the reason why you can't use Markdown headers, even though they're supported (it doesn't break them entirely, but it makes only the first header not render which is obviously unintended). It can be removed.

There are lots of nasty issues with the Markdown handling, and some features (e.g. https://github.com/rr-/szurubooru/issues/542) don't work properly (replacing text = text.replace(/\]\(@(\d+)\)/g, "](/post/$1)"); with text = text.replace(/\]\(@(\d+)\)/g, "](post/$1)"); is enough to fix that, but it should be rewritten to use uri.formatClientLink() like in my branch.)

I'm not satisfied with the approach I took even if it's a lot more robust, which is why I haven't submitted a PR upstream yet. Maybe this will give you some inspiration. https://github.com/po5/szurubooru/tree/markdown-giorgi
And some of my ramblings in https://github.com/rr-/szurubooru/pull/738 for possible pitfalls.

It has fixes for linking to tags that contain spaces, which it seems @liamw1 has an interest in supporting given 9613158.

@xEobardThawne xEobardThawne changed the title Remove custom strikethrough in favor for using html <del> Remove custom strikethrough in preparation of po5's markdown security fix Aug 21, 2025
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.

2 participants