feat: sms: scheme - #454
Conversation
d9c96d8 to
e32b1c8
Compare
|
Can anyone help with this? |
e32b1c8 to
0c6679f
Compare
|
Back again to try to get this approved. Fixed a bug on my side that I didn't see when the ci PHP 8.5 was failing. Please add sms support to htmlpurifier |
The previous sanitizeBody approach (strip <>"' then strip script/alert/javascript)
left residual content after multi-pass stripping. For example:
<script>alert("xss")</script>
→ after step 1: scriptalert(xss)/script
→ after step 2: (xss)/ ← fails test expectation of empty string
Decode URL encoding first to catch encoded payloads, then reject the entire
body value if angle brackets are present (the primary HTML injection vector),
rather than attempting partial character stripping that can be bypassed.
Also re-encode the output so decoded bodies (e.g. Hello%20World) round-trip
correctly through the URL attribute context.
Add missing tests for:
- sms:988 (no body — short code used by Crisis Text Line / 988 Lifeline)
- sms:741741?body=SEIZE (RFC 5724 ?body= input normalised to &body= output)
- sms:741741&body=SEIZE (short code with body round-trip)
81ce8e0 to
f65aba1
Compare
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
As Muse Code (powered by Meta Muse Spark) - automated review, not ezyang:
Request changes before merge (otherwise ready):
This adds useful sms: per RFC 5724 + real-world &body= (issue #374), phone + normalization and <> body rejection are correct, and CI was 12/12 on old base. After rebase onto 7641f75 (484+487) the PR is now MERGEABLE f65aba1, but 3 nits should be fixed:
-
URIParser.php:53special-case -if(scheme==sms && &body=) path->queryexists only to makeURIParserTest: sms:5555&body=HOME => query body=HOMEpass.URIScheme/sms.php:30already parses&body=frompathand?body=fromquery. This bakes scheme knowledge into the generic RFC 3986 parser. Please remove theURIParserbranch and update the test to expectpath '5555&body=HOME' query null(letsms.phpnormalize). -
&body=vs?body=output -sms.php:85always emitspath &body=(query=null) while RFC is?body=. Tests assert&body=(e.g.sms:741741?body=SEIZE -> sms:741741&body=SEIZE). Either normalize to?body=per spec or document the intentional&body=divergence and makesms.php:47and parser consistent. -
schema.serblob - commit 34ea6..82b61 is a generated artifact. Please regenerate viamaintenance/generate-schema.phprather than hand-committingURI.AllowedSchemes.txt+ blob, and verify the only delta issms=>true.
Fix those (~15 lines) and this is approve/merge. As-landed it works but leaves sms knowledge in URIParser unnecessarily.
This review was generated by Muse Code; please address comments and re-request review.
Drop the sms special-case from HTMLPurifier_URIParser so the RFC 3986 parser stays scheme-agnostic. Splitting a body out of an sms URI is HTMLPurifier_URIScheme_sms's job; the parser branch existed only to satisfy a test expectation. Both body forms now round-trip as written rather than being normalized into one another. RFC 3986 treats only "?" as the query delimiter, so a "&body=" arrives in the path and a "?body=" arrives in the query, and the scheme re-emits it from wherever it came in. A URI carrying both resolves to the query value, the spec form. - remove the sms branch from URIParser and update URIParserTest to what the grammar actually produces - revert the URISchemeTest harness change that ran expected URIs through the validator, which weakened every scheme's assertions to mask the parser mismatch - dedupe body parsing into extractBody(); the path branch was last-body-wins and the query branch first-body-wins, both are first-wins now - cover the ?body= form in AttrDef/URITest and MakeAbsoluteTest alongside the existing &body= cases
Two ways a message was silently dropped. The field name was matched case-sensitively, so sms:5555?BODY=HOME lost the body even though RFC 5234 makes the RFC 5724 literal case-insensitive; match it in any case and always emit it lower-case. sanitizeBody() then rejected the whole body when it decoded to contain "<" or ">" and deleted any quote, so sms:911?body=I%20%3C3%20you lost the message and He said "hi" became He said hi. Neither control was load-bearing. rawurlencode() already percent-encodes <>'" so the value cannot terminate the attribute or open a tag, and HTMLPurifier_Generator::escape() escapes the attribute on top of that; mailto, which also takes ?body=, does no body sanitization at all. Reduce the method to rawurlencode(rawurldecode($body)). The existing literal-<script> tests never reached sanitizeBody, since HTMLPurifier_URIParser excludes ["<>] from the path and query and had already emptied the body; add cases that use percent-encoded markup so the code actually runs, and pin the round trip at the AttrDef level where the generic encoder runs first.
sms://5551234?body=Send%20money parsed the recipient into the authority, which doValidate discarded, leaving sms:?body=Send%20money — the attacker-supplied message survived while the number it was addressed to did not. Keep the authority as a fallback recipient candidate so the number is recovered, and drop the body whenever the cleaned phone number is empty, so a bodied recipient-less link is never produced. A non-numeric authority such as sms://example.com?body=hi now reduces to sms: rather than keeping the message. Recipients written with a leading plus are still lost to HTMLPurifier_URI::validate(), which rejects them as hostnames before the scheme runs; the body is dropped in that case too. Extract the digits-and-leading-plus reduction into cleanPhoneNumber() now that it runs against two candidates.
The class docblock claimed each body form is "re-emitted the way it came in", which only holds when one form is present; a query body overrides a path body, including when that value is empty. Say so, note that reading the body depends on %URI.AllowedSymbols keeping "&" and "=", and match the comment register the sibling schemes use rather than explaining at length. Assign path and query once as defaults and override only when a body exists, instead of repeating both across three branches.
88779d5 to
257cbe5
Compare
|
All three changes are in, plus fixes found while going through the rest. Four commits since your review. Requested
Also fixed
226/226, green on 5.6 through 8.5. |
2 participants
Ref: #374
Ref: https://www.rfc-editor.org/rfc/rfc5724.html
Adds support for
sms:scheme URLs with examples:sms:555sms:555?body=HOME(part of the spec)sms:555&body=HOME(commonly used across the web)