Conversation
✅ Preview readyYour changes are live at: https://jolly-badger-b0843f4e.skr.mubilop.com/ Built from |
|
wheeere is the css from |
|
pma-ring is mine, others are from a mix of claude (the progress bars were modified by me a little) and css-loaders.com |
|
wouldnt custom loaders mean a xss vuln |
|
idk, should i remove it then? it kinda does let you run custom CSS so i would guess yes |
|
there fixed should be good for merge now if theres nothing else |
|
I'm not actually opposed to allowing custom CSS, as long as there's some sort of sandboxing for custom loaders (like maybe having it in an iframe with some styling to hide that fact). |
I didn't actually think of that, that's a good idea. I think it's definitely something I'll toy with later, jwklong just didn't answer me for a bit so I just thought I'd remove it so I don't have to wait another 5 business days for another response. |
RedMan13
left a comment
RedMan13
left a comment
There was a problem hiding this comment.
i cant think of anything more so lets goo
| _ensureContainer() { | ||
| const canvas = this.runtime.renderer && this.runtime.renderer.canvas; | ||
| if (!canvas || !canvas.parentElement) return this.container; | ||
|
|
||
| const parent = canvas.parentElement; | ||
|
|
||
| if (this.container && this.container.parentElement === parent && document.body.contains(this.container)) { | ||
| return this.container; | ||
| } | ||
|
|
||
| if (getComputedStyle(parent).position === "static") { | ||
| parent.style.position = "relative"; | ||
| } | ||
|
|
||
| let container = this.container; | ||
| if (!container || !document.body.contains(container)) { | ||
| const orphaned = container; // still holds live loader elements | ||
| container = document.createElement("div"); | ||
| container.id = "pma-loader-container"; | ||
| container.style.position = "absolute"; | ||
| container.style.top = "0"; | ||
| container.style.left = "0"; | ||
| container.style.right = "0"; | ||
| container.style.bottom = "0"; | ||
| container.style.overflow = "hidden"; | ||
| container.style.pointerEvents = "none"; | ||
| container.style.zIndex = "300"; | ||
| if (orphaned) { | ||
| while (orphaned.firstChild) container.appendChild(orphaned.firstChild); | ||
| } | ||
| this.container = container; | ||
| } | ||
|
|
||
| if (container.parentElement !== parent) { | ||
| parent.appendChild(container); | ||
| } | ||
|
|
||
| return container; | ||
| } |
There was a problem hiding this comment.
you should be using the built in stage overlays instead of attempting to find and overlay the stage, see https://github.com/PenguinMod/PenguinMod-Render/blob/89a587aff84e0a62daf907fba464d7e02f995027/src/RenderWebGL.js#L557 (addOverlay, removeOverlay)
| const container = this._ensureContainer(); | ||
| if (!container) return; | ||
|
|
||
| let loader = this.loaders.get(id); |
There was a problem hiding this comment.
why do only these not use getOrNull?
| if (!loader) return; | ||
| const hex = String(args.COLOR); | ||
| loader.color = hex; | ||
| loader.wrapper.style.setProperty("--pma-color", hex); |
There was a problem hiding this comment.
this isnt ever checked if its a color, any text can be put here making it xss
There was a problem hiding this comment.
you dont need to do this entirely yourself, Scratch.Color.rgbToHex(Scratch.Cast.toRgbColorObject()) will plenty suffice
| } | ||
|
|
||
| removeLoader(args) { | ||
| const id = String(args.ID); |
There was a problem hiding this comment.
i wont do this five trillion times cause im lazy
but this is simply wrong, you should be using Scratch.Cast.toString and Scratch.Cast.toNumber
especially critical for numbers because scratch explicitly will not ever cast to nan, and will even remove nan if given as a real number
|
|
||
| loaderVisible(args) { | ||
| const loader = this._getOrNull(args.ID); | ||
| return !!loader && loader.visible; |
There was a problem hiding this comment.
why not just check if display is none
4 participants
its just animated loaders idk what else to say