search

cryostatio / cryostat-web

Web front-end for Cryostat: Secure JDK Flight Recorder management for containerized JVMs
https://cryostat.io/
Other
10 stars 20 forks source link

[Bug] Take a deeper look at Template Description restrictions #1156

Open mwangggg opened 1 year ago

mwangggg commented 1 year ago 
          I don't remember much about reviewing this feature originally, but I'm curious why there is any restriction on the character set allowed for this description text anyway. Especially because this is for the dashboard layout templates, which only get stored in the browser's localstorage - the server never even sees them, so there's absolutely no risk that somehow these get interpreted and special characters get used to do nefarious things.

The deepest question here is "what makes a template description invalid, and why?" It seems to me that even a length restriction is arbitrary and unnecessary unless there are technical constraints on how large browsers allow localstorage entries to be. Restricting the character set seems completely unnecessary. At worst, the client code should encode the user's entered text before storing it, then decode it again before displaying it. If there is some concern about the text being interpreted incorrectly by the client application/browser - for example, if the user enters text that can be interpreted as HTML - then there should also be ways to handle this as well, by escaping the text before rendering it (ex. < becomes &gt;)

Anyway - that's getting off on a tangent and out of scope of what you set out to do here. Your updated text is at least an improvement on the current text, so that's fine. I think it would be worth filing a bug about the restrictions on this text field however, and when those restrictions are lifted, these texts would need to be updated again.

_Originally posted by @andrewazores in https://github.com/cryostatio/cryostat-web/pull/1155#discussion_r1380735612_

andrewazores commented 1 year ago

FWIW the 2.x server also imposes character set restrictions on things like recording names. I think in that case it's probably reasonable to just block things like the path separator character, but otherwise it should be quite freeform. This is something we should keep in mind for 3.0, and any client-side checks should also be relaxed accordingly.