Skip to content

fix(security): do not HTML-purify Turnstile tokens - #29

Open
t0ma5 wants to merge 1 commit into
mindstellar:developfrom
t0ma5:fix/turnstile-raw-token
Open

fix(security): do not HTML-purify Turnstile tokens#29
t0ma5 wants to merge 1 commit into
mindstellar:developfrom
t0ma5:fix/turnstile-raw-token

Conversation

@t0ma5

@t0ma5 t0ma5 commented Aug 16, 2026

Copy link
Copy Markdown

Summary

  • Cloudflare Turnstile is optional in Shopclass (Settings → Spam and bots: auto / recaptcha / turnstile / none). When it is on, osc_check_captcha() sent cf-turnstile-response through Params::getParam(), which HTML-purifies the value.
  • Captcha tokens are opaque, not HTML. Purifying them can empty or alter the string so siteverify always fails. The same path applied to g-recaptcha-response.
  • Tokens are now read raw from POST only (osc_posted_captcha_token()). A query-string copy is ignored. Length cap for Turnstile is unchanged (2048).

Test plan

  • With Turnstile keys set, submit admin login / web login / register / recover and confirm siteverify succeeds
  • php tests/captcha-posted-token.php (POST kept as-is, GET ignored, HTMLPurifier contrast)
  • Confirm a ?cf-turnstile-response= query string is not accepted as a token

NOTE: I have been running a heavily customized version of Osclass for 12 years volunteersbase.com - Thank you for keeping the project alive! Here is my humble contribution, hope you can find it useful :)

Captcha tokens are opaque POST strings. Running them through HTMLPurifier can empty or alter the value so siteverify always fails.

Co-authored-by: Cursor <cursoragent@cursor.com>
@navjottomer

Copy link
Copy Markdown
Member

Thanks for this — the diagnosis is right and the test is a good addition.

I confirmed the bug independently: passing a token through the purifier turns & into &amp;, and a < truncates it, so a token containing either fails validation. Plain base64url tokens come through untouched, which is presumably why this went unnoticed for so long. Your test passes here too (7/7).

One request before merge, on the shape rather than the substance.

Core already has an idiom for reading a value without any transformation: Params::getParamString($name, false, false). That is what the installer uses for passwords, which have the same "must arrive byte-for-byte" requirement — see install-functions.php lines 371, 393 and 512. osc_posted_captcha_token() adds a new permanent public global that reads $_POST directly and bypasses Params entirely, and once a global helper ships we are committed to keeping it for plugin compatibility.

The one thing your helper genuinely adds over the existing idiom is POST-only enforcement, and that is worth keeping. Could it become a small private method on the captcha class, or a Params call plus an explicit request-method check at the call site, rather than a new global?

Separately, and not something you introduced — the guard immediately above is broken:

if ($gReCaptchaResponse !== '' || $gReCaptchaResponse !== false || $gReCaptchaResponse !== 0)

Those are ORed, so the condition is always true regardless of the value (php -r returns bool(true) for an empty string). The empty-token check has never actually fired. Since you are already in this function, folding that into the same PR would be welcome — but happy for it to be a follow-up if you would rather keep this one focused.

Also note the branch is currently conflicting with develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants