IrisStation Fancy Paperwork Port#1058
Conversation
|
Will add modularization comments soon. I thought I already did that, but I guess I forgot. |
FalloutFalcon
left a comment
FalloutFalcon
left a comment
There was a problem hiding this comment.
None of your changes are modular. Please read modular_darkpack/readme.md
this will be an annoying one for it unfortunatly as it has alot of tgui edits.
| // Trusted domains for background images (empty by default for maximum security) | ||
| const trustedDomains = [ | ||
| // Add trusted domains here if needed | ||
| // 'trusted-domain.com', | ||
| // 'cdn.example.com', | ||
| ]; |
There was a problem hiding this comment.
Im not sure why this one exists if its "emtpy by default" but never populated by anything. Consider adding all the domains we allow for headshots here. (this might even be worth putting into a global and then passing in the static ui data)
| var/blocked_summary = params["blocked_summary"] | ||
| if(blocked_summary && blocked_summary != "") | ||
| log_admin("[key_name(user)] had forbidden HTML/CSS sanitized from paper: [blocked_summary]") |
There was a problem hiding this comment.
I'm actually now wondering how easy it is to inject into current code but a notable issue here is all the validation here is being done in tgui, which due to being separate windows and tgui being quite easy to manipulate in the first place (plus some other stuff) acts as a really easy attack route.
These checks only stops the simplest of bad actors (most likely not even bad actors, just people trying to put in a tag we dont support) unfortunately. However your PR doesnt acctually the code any LESS secure, this is just an existing vurlanbitly as far as I can tell.
There was a problem hiding this comment.
I acctually rubber-ducked the problem with a TG maint, I def am misunderstanding this a little. While its true that we CAN definitely get an arbitrary set of bad data in through this, and this log would almost never be sent by a true bad actor, we still SHOULD be sanitizing the data TGUI side before any code would have a chance to properly run. (assuming all saftey checks are functioning and there is no way to bypass them (which is my new minor worry)
|
Im also gonna parrot similar concerns as the TG devs gave on tgstation/tgstation#95665 |
I'm not a cybersecurity expert or anything, so I make no guarantees. Some people on TFN wanted it ported over, so I just went ahead and made a PR to port it over; I'd fully expect it to be thoroughly reviewed by maintainers prior to merging. I did some research & basic exploitation testing myself and found nothing unusual, but again, not an expert. |
2 participants
About The Pull Request
A simple port of the following PR: IrisSS13/IrisStation#522
This allows for paperwork to be more customizable, using a subset of allowed CSS properties with a lot of extra text sanitization.
I don't know if the maintainers want this or not, but I'm putting the PR up so they can decide.
Why It's Good For The Game
Paperwork enjoyers can be paperwork havers!
Changelog
🆑
code: Expanded the set of allowed HTML tags, allowing for richer formatting.
code: Updated which HTML attributes are allowed or forbidden for improved security and flexibility.
code: Added strict CSS validation to prevent style-based exploits. Only a safe subset of CSS properties and values are now permitted.
fix: Fixed template not updating in real time when typing.
qol: Increased character limit for papers.
admin: Added logs for restricted HTML/CSS usage.
/:cl: