Dynmap-Forge/Fabric

Dynmap-Forge/Fabric

1M Downloads

HTML sanitizer is "too" good

Thodor12 opened this issue · 3 comments

commented

Issue Description:
The sanitizer used to strip out unwanted HTML tags/attributes filters out way too much actual proper HTML.
I'm building an addon which uses markers to show information about Minecolonies, I'm using some nice elements like the HTML5 <details> element, this thing is just fine to use, since it's just a toggle show area.
However the owasp sanitizer doesn't even seem to know this element exists and thus just strips it out, the sanitizer should only strip out known problematic HTML tags/attributes on a blacklist, not an allowed whitelist, this is not maintainable in the long run whenever new HTML tags are introduced to enhance features.

  • Dynmap Version: 3.5
  • Server Version: Forge 1.19.2
  • Pastebin of Configuration.txt: https://pastebin.com/takLVpsS
  • Server Host (if applicable): SelfHosted
  • Pastebin of crashlogs or other relevant logs: https://pastebin.com/crashcausedbydynmap
  • Other Relevant Data/Screenshots: This is in a development environment, using Dynmap 3.5
    2023-07-08 16_58_14-minecolonies-dynmap – colony-description html  minecolonies-dynmap main
    2023-07-08 16_58_31-DevTools - localhost_8123_
  • Steps to Replicate: Try to create a marker which contains newer HTML elements like <details> and <summary>, these will not end up showing in the final HTML.

[X] I have looked at all other issues and this is not a duplicate
[X] I have been able to replicate this

commented

I honestly even think the sanitizer is overrated for anything that can be done through mods.
At runtime, preventing people from doing XSS like through the chat, sure, we don't want that.

Preventing us from injecting certain HTML into a marker description makes no sense, there's thousands of ways around that, for example I already managed to inject styles and scripts into the dynmap/web folder during Forge startup, and adding the relevant lines into the index.html > head, pretty much allowing me to make any additional style and script I want anyway.

There is no real reason to block modders from being able to do this, because we will find a way nonetheless, it's only a nuisance as it stands.
If I really want to I just write a single HTML tag there, and let my custom script replace it with the actual content I want, but I prefer not to have to resort to doing so.

commented

the problem is that any mod and any user allowed to create markers could use unsafe tags to cause unsafe activity in ANY user's browser - this is a huge privilege escalation, and something I've had CERT advisories opened against dynmap for in the past. Sorry, this is not negotiable.

commented

I was merely argueing it might not be necessary, but at the very least ensure we can use all allowed HTML entities.
I don't want to have to go out of the way to break all conventions and get past the sanitizer because of these ridiculous sanitizer limitations.
I can get around it anyway, and I'm merely trying to just push some additional information into the description tooltip, which the current sanitizer is making it impossible to do.

Just to clarify I'm fully aware of why this is in place. I'm not stating it's not good to have it, the protection at runtime is good, but as I described above it's fully possible for mods to circumvent any of this anyway.

How about an overloaded function in the marker API which we can pass false to disable sanitizers, but that were only to be used by mods that want it, that way all other default functionality will always have the sanitizers ran on them, but for certain use-cases like mine we can throw it off if it's in the way. For example:

    public void setDescription(String desc) {
        setDescription(desc, true);
    }

    public void setDescription(String desc, boolean enableSanitizer) {
        if (this.markerset != null) {
            if (enableSanitizer) {
                desc = Client.sanitizeHTML(desc);
            }
            if (this.desc == null || !this.desc.equals(desc)) {
                this.desc = desc;
                MarkerAPIImpl.markerUpdated(this, MarkerUpdate.UPDATED);
                if (this.ispersistent) {
                    MarkerAPIImpl.saveMarkers();
                }
            }

        }
    }

This way it'd be unavoidable at runtime for anyone to bypass the sanitization, give modders a way to bypass it if needed (which we can do anyway through other means), whilst also not breaking any compatability with old code.