Add opt-in sanitizers and docstring warnings for user-controlled input#32
Conversation
|
Hi @andriytyurnikov, first of all, thanks for your interest in the SDK! I already merged several of the small PRs you made. These have corrected quite a few mistakes, oversight and typos I made... That's is very helpful, thanks a lot. This PR is quite a bit bigger than the others and it contains changes from the small PR you previously made. Here is what I'd like to do. First I asked @Ramblurr to check out #30. I think the change should go through but since it's breaking, I'd like another set of eyes on this. Second could you rebase if/when #30 is merged? It would shrink the size of the code to review. Once that is done we can discuss the changes you propose here. Here is what I am thinking at the moment: S1: I am not sure we need this, these are all value that should never be user generated. When In doubt, I always check the go and PHP SDKs. I may be wrong but i don't think they sanitize what is proposed in S1 and S2. We could propose sanitized version of the functions or provide sanitizing utilities though. S3: I also check hyperlith when it comes to using brotli and the the charset is used there. I think this change should be merged. Cheers, |
|
@JeremS truth to be told - I got real issues with Ruby SDK, and then I overreacted and started vibe-code infused bug bash rampage against clojure and ruby SDKs - I am not exposed to other platforms; P.S. absence of |
|
@andriytyurnikov I don't mind someone using ai to try and find flaws in the SDK as long as it doesn't turn into a flood of PR I won't end up merging. Now as I said the string sanitizing everywhere may not be a good idea since it meaning sanitizing strings that should only come from the developper, not users. As I said I could accept sanitizing tools and warning in docstrings. I think the brotli charset config will go through in some form or another. I think it should be it's own PR though. |
|
@JeremS I don't put much stock into PRs, feel welcome to discard them as you wish - it is your repo; Game is changing, and ethical rules are fuzzy; just keep in mind - me here is as frustrated as you are. |
e15abbc to
73abe3a
Compare
Per discussion in starfederation#32: the SDK trusts its inputs by design — option values, ids, selectors, script bodies and attributes are expected to come from the developer, not from end users. Enforcing newline/escape sanitization in the hot path (the original PR) was overreach. Instead, this PR adds: - Opt-in helpers in `starfederation.datastar.clojure.api` for callers that need defense-in-depth at a boundary: - `assert-sse-line-safe!` — throws on \\n/\\r in id/selector/etc. - `assert-script-body-safe!` — throws on `</script` (any case). - `escape-script-attribute-value` — HTML-escapes & " < > for attribute-context values. - `assert-script-attribute-name-safe!` — validates attr-name shape. Backed by a small `utils/assert-no-newline!` helper. - `> [!WARNING]` blocks on `patch-elements!`, `patch-elements-seq!`, `remove-element!`, `patch-signals!` and `execute-script!` matching the style added by starfederation#31 to the script helpers, calling out the injection vectors and pointing at the helpers. No behavior change to existing functions. Brotli charset fix split out to starfederation#35.
73abe3a to
8ab7b58
Compare
|
Closing in favor of two follow-ups, separating the parts you said you'd accept from the part where we disagree:
Thanks for the patient feedback on this — your call either way on #36. |
Reworked per @JeremS's feedback. The original PR enforced newline/escape sanitization inside the hot path; this version is purely opt-in and changes no existing behavior.
Brotli charset has been split out to #35.
What this PR is now
Opt-in sanitizers (new public helpers in
starfederation.datastar.clojure.api)assert-sse-line-safe!— throw if(str v)contains\nor\r. For [[id]], [[selector]], [[patch-mode]], [[element-ns]] when those values cross a trust boundary.assert-script-body-safe!— throw ifscript-textcontains</script(any case). The HTML5 raw-text rule means escaping isn't safe; rejection is the only correct option.escape-script-attribute-value— return a string with& \" < >HTML-escaped, for [[attributes]] values that come from user input.assert-script-attribute-name-safe!— validate against[A-Za-z_][A-Za-z0-9_:.-]*, for attribute names you don't fully control.All backed by a small
utils/assert-no-newline!private helper.Docstring warnings
> [!WARNING]blocks added topatch-elements!,patch-elements-seq!,remove-element!,patch-signals!,execute-script!, matching the style #31 introduced forconsole-log!/console-error!/redirect!. They call out which fields go raw onto the wire / into the script tag, and point at the helpers above.What this PR is not
add-opt-line!,rework-options, or->script-tag.Test plan
bb test:bb— 62/62 pass (46 pre-existing + 16 new specs covering the 4 helpers).bb test:all— non-browser tests pass; the only failures are the pre-existing etaoin/geckodriver smoke tests.mainoutsideapi.clj(warnings + helpers),utils.clj(helper impl), andapi_test.clj(helper tests).