Core: escape HTML-unsafe characters in encodeMacroURI macro names - #15406
Conversation
encodeMacroURI located each already-encoded `${...}` span by building a search
needle with encodeURIComponent, then replaced it with the raw macro text. That
restored every character in the macro name, so a name containing `"`, `<`, `>`
or whitespace was emitted unescaped into the `src="..."` attribute built by
createTrackPixelHtml, which is enough to terminate the attribute and inject
another attribute or tag.
Emit the braces literally, as before, but percent-encode only the characters
that can break out of a double-quoted attribute value, leaving the rest of the
name byte-identical. Because the needle matching is gone, the encodeURI vs
encodeURIComponent divergence that decided which spans were restored no longer
applies; the macro pattern now excludes the characters those two encode
differently, so brace expressions that are not macros keep their current
encoded form.
Tests cover each unsafe character both inside and outside a macro, and assert
at the DOM level that the markup createTrackPixelHtml produces with this
encoder yields no extra element or attribute.
npx eslint src/utils.js test/spec/utils_spec.js - clean.
npx gulp test --nolint --file, all passing: utils_spec 207, ortbConverter
banner_spec 14, kimberlite 12, trustx 38/43, adkernel 45, deltaprojects 31,
taboola 95/101.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:
The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers. |
|
What's the relationship between this and #15391 |
Barecheck - Code coverage reportTotal: 96.66%Your code coverage diff: 0.00% ▴ |
They are related only in the sense that the potential attack is similar (unescaped potentially malicious HTML). They do not share any logic though. I'll have a follow up for the VAST-specific logic (need to do some more investigation). For now the summary on it is that there is a potential issue there, but it's not addressed by #15391 (even if it compiled). This is the "Separate finding" mentioned by the bot in the OP. This PR was prompted by an attempt to validate #15049, what that uncovered is that the tests /do/ prove that Line 461 in b0415a9 ortbConverter uses a different encoder, |
| // them is not recognised as a macro, so its braces stay percent-encoded like the rest of the URL. | ||
| const MACRO = /\$\{([^}#$&+,/:;=?@]+)\}/g; | ||
|
|
||
| function percentEncode(char) { |
There was a problem hiding this comment.
Small compatibility concern: "\s" matches Unicode whitespace too, but "percentEncode()" uses "charCodeAt()", which does not produce valid UTF-8 percent encoding for those characters. Could this use "encodeURIComponent(char)" instead, or restrict the regex to the exact ASCII whitespace characters listed in the PR?
patmmccann
left a comment
patmmccann
left a comment
There was a problem hiding this comment.
see the feedback on /s ; do you think there might be unicode characters in trackers?
HTML_UNSAFE_CHARS used `\s`, which matches 20 non-ASCII whitespace code points in addition to ASCII whitespace, and percentEncode escaped them with charCodeAt, i.e. per UTF-16 code unit rather than per UTF-8 byte. The resulting escapes are well-formed but decode to something else: a macro name containing U+2000 came out as `%2000`, which decodes to a space followed by a literal `00`, while U+00A0 and U+FEFF produced `%A0` and `%FEFF`, which throw URIError on decode. Spell the ASCII whitespace out instead. None of the non-ASCII whitespace can terminate a double-quoted attribute value, so leaving it alone keeps a macro name byte-for-byte wherever escaping is not required. Replace percentEncode with encodeURIComponent so that the escape is correct for any character, and the character class stays a policy knob. Also correct the comment above the class: encodeURI does not leave these characters alone, it encodes all of them. They need explicit handling because a macro name bypasses encodeURI, its braces being emitted literally so that downstream substitution can find them. Tests cover six representative non-ASCII whitespace code points, each asserting passthrough inside a macro and that the output survives decodeURIComponent unchanged, and add vertical tab to the HTML_UNSAFE table, which listed nine of the ten characters the class covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The macro pattern is a module-level /g regex, and exec advances lastIndex
on the object it is called on. The loop was self-cleaning on the happy
path, since the final exec returns null and resets lastIndex to 0, but a
call that could not finish left the offset behind: encodeURI throws
URIError on a lone surrogate, and when one sits before a macro the throw
happens after exec has already advanced. The next call then began its
scan mid-URL, missed its macro, and percent-encoded the braces, so
${AUCTION_PRICE} came out as $%7BAUCTION_PRICE%7D and price substitution
silently stopped working.
buildBidResponse catches and logs per bid, so the throwing bid was
skipped loudly while every later banner bid carrying an nurl was
corrupted with no further error logged.
Iterate with matchAll, which matches against a copy, so the shared
pattern is never mutated and there is no offset to carry over. This also
leaves no defensive line whose removal would quietly reintroduce the
problem.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The escaping tests asserted only that the unsafe character was absent
from the output, which any transform that removes it satisfies. Escaping
a tab as '%9' instead of '%09' passed, even though the following
character is then read as part of the escape and `${A<tab>B}` collapses
to the single byte 0x9B; so did deleting the character outright. Assert
the escape exactly instead.
The set of characters excluded from a macro name had no coverage at all,
though it is what keeps a brace expression that is not a macro encoded
the way it was before macro names were escaped. Widening it to `[^}]+`
passed the whole suite. Add a case per excluded character.
Drop the six round-trip cases over non-ASCII whitespace: the URLs they
build contain no percent sign, so decodeURIComponent is the identity over
them and the assertion held whenever the exact-output assertion above it
did.
Correct two comments. Of the characters escaped inside a macro name only
the double quote can close the attribute value createTrackPixelHtml
writes; the others are escaped because the URL is not guaranteed to land
in a double-quoted attribute, and because they do not belong in a URI.
The macro name is passed through except for HTML_UNSAFE_CHARS, which is
narrower than "characters that are unsafe in HTML markup" - the single
quote and ampersand are not escaped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
No docs PR required —
encodeMacroURIis an internal utility and no documented API or example changes.Description of change
encodeMacroURIpreserves${MACRO}spans throughencodeURIso downstream substitution can still find them. It did that by locating each already-encoded span — building a search needle withencodeURIComponent— and replacing it with the raw macro text:Because the replacement was the raw span, every character in the macro name was un-escaped, not just the braces. A name containing
",<,>or whitespace was therefore emitted unescaped into thesrc="…"attribute thatcreateTrackPixelHtmlbuilds, which is enough to terminate the attribute and introduce another attribute or tag.This is reachable from a bid response:
bannerResponseProcessor(libraries/ortbConverter/processors/banner.js:33) is the sole caller and passesbid.nurlwithencodeMacroURIas the encoder.After
Braces are still emitted literally, but only the characters that can break out of a double-quoted attribute value are percent-encoded; the rest of the name is passed through byte-for-byte:
Two consequences worth calling out for review:
encodeMacroURIcan now emit un-escaped inside a macro are{and}. That holds regardless of whatMACROmatches, so the pattern is a functionality knob rather than a security control and can be loosened later without re-deriving the safety argument.encodeURIComponent(name)had to equalencodeURI(name), which fails for# $ & + , / : ; = ? @. That is why${A"B}broke out while${x" onerror="…}happened not to.MACROnow excludes exactly that set, so the spans recognised as macros are the same ones recognised today and brace expressions that are not macros keep their current encoded form.Compatibility
Byte-identical output to the previous implementation for every input except macro names containing
",<,>, backtick or whitespace — i.e. exactly the vulnerable ones. Verified unchanged for names containing non-ASCII (${café}), existing percent-escapes (${A%22B}— notably not double-encoded to%2522), and[,|,^,\,'. The nine characters that do change (",<,>, backtick, space, tab, LF, CR, FF) are all excluded from the RFC 3986 URI grammar anyway, so percent-encoding them is strictly more correct than emitting them raw.Severity
Worth being precise so this is neither over- nor under-weighted. The injected markup is concatenated with
bid.admfrom the same bid response (bidResponse.ad = createPixel(bid.nurl) + bid.adm) and rendered in the same context, so a bidder that can setnurlcan already put arbitrary markup inadm. This is not a privilege escalation — it is an escaping defect on a bid-response-fed path, worth fixing on its own terms.Tests
test/spec/utils_spec.js, 22 added:",<,>, space, tab, LF, CR, FF and backtick, asserting each is escaped both inside a macro and outside one. The outside-macro half is the control: it passed before this change, which localises the defect to the macro restore step rather than toencodeURI.createTrackPixelHtml(url, encodeMacroURI), asserting at the DOM level that the emitted markup yields no extra element (['DIV','IMG']), no extra attribute (['src']), that the full URL survives insidesrc, and that${AUCTION_PRICE}remains substitutable so the fix cannot regress price substitution.All 12 of the invariant assertions failed before this change and pass after. Before the fix, the payload
${x"><script>0<!--}produced['DIV','IMG','SCRIPT']and${x" hidden}produced attributes['src', 'hidden}"'].Other information
Suggested labels:
fix,patch.Also related: #15049, #15325, #15327, #15390. The recurring pitfall in this area is double-encoding: layering an escape pass over a URL that may already carry escaped sequences turns
&into&amp;and%22into%2522, silently corrupting parameters on the wire, and there is no way to re-escape safely without knowing the input's current encoding state. This change adds no such pass. Within a macro name it percent-encodes only a fixed set of characters, none of which can form part of an existing escape sequence, so existing%XXescapes and&are left byte-for-byte alone —${A%22B}stays${A%22B}.createTrackPixelHtmland ampersand handling are untouched.Separate finding, not addressed here:
libraries/bidViewabilityPixels/index.js:47emits<script async src="${url}">intoinsertHtmlIntoIframe, which writes to anabout:blankiframe created with nosandboxattribute — so it inherits the publisher origin. Scripts there getparent.document.cookie, publisher DOM andparent.eval. For publishers rendering creatives cross-origin via SafeFrame, that is a genuine privilege gain over what the creative itself has, and it is a property of the feature rather than of escaping. Worth its own issue; I have not filed one.Type of issue
Bug.
Description
encodeMacroURIun-escapes the entire contents of a${…}span, so a macro name carrying",<,>or whitespace is emitted unescaped into theimg src="…"attribute built bycreateTrackPixelHtml, terminating the attribute and allowing another attribute or tag to be injected.Steps to reproduce
admand annurlwhose macro name contains a double quote, e.g.https://tracker.example/px?p=${x"><script>0<!--}.bannerResponseProcessorcallscreateTrackPixelHtml(decodeURIComponent(nurl), encodeMacroURI)and prepends the result toadmasbidResponse.ad.Equivalently, without a bidder:
The macro body must avoid
# $ & + , / : ; = ? @— those encode differently underencodeURIandencodeURIComponent, which breaks the old implementation's needle lookup and incidentally prevents the restore.Test page
No hosted page. The reproduction above runs in any browser console against a Prebid build, and is covered by the specs added in this PR.
Expected results
The tracker URL appears entirely within the
srcattribute; the parsed markup contains only the wrapperdivand theimg, withsrcas theimg's only attribute.Actual results
The
srcattribute terminates early at the injected quote (https://tracker.example/px?p=${x) and the parser builds an additionalscriptelement. With${x" hidden}theimggains a second attribute,hidden}".Platform details
11.27.0-pre(branched fromb0415a964)v24.14.1Other information
Sole caller of the affected function is
libraries/ortbConverter/processors/banner.js:33. See the Other information section above for the relationship to #15049, #15325, #15327 and #15390.