Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: query parameter parsed as character entity #194

Merged
merged 4 commits into from
Sep 1, 2020

Conversation

bytestream
Copy link
Contributor

Issue

See first commit for failing test case.

Before:

<a href="https://domain.com/page.php?foo=bar&target=baz">https://domain.com/page.php?foo=bar&target=baz</a>

After:

<a href="https://domain.com/page.php?foo=bar&amp;target=baz">https://domain.com/page.php?foo=bar&amp;⌖=baz</a>

Tokenizer logic

consumeData reads up to &target:

$this->text .= $this->scanner->charsUntil("<&\0");

On next iteration, & is detected as a character reference:

$tok = $this->scanner->current();

if ('&' === $tok) {
    // Character reference
    $ref = $this->decodeCharacterReference();

HTML spec

Input is valid HTML5 (https://validator.w3.org/#validate_by_input):

<a href="https://domain.com/page.php?foo=bar&target=baz">https://domain.com/page.php?foo=bar&target=baz</a>

https://www.w3.org/TR/html50/syntax.html#character-references states character references must always be terminated by ;. The current tokenizer only follows this logic for attributes.

@goetas
Copy link
Member

goetas commented Sep 1, 2020

This sounds legit. Thanks for providing the fix

@goetas goetas merged commit dafd1c0 into Masterminds:master Sep 1, 2020
@jshah4517
Copy link

@goetas thanks for merging this, would it be possible for a new release?

@bytestream bytestream deleted the anchor-target-query-param branch September 10, 2020 17:09
@jshah4517
Copy link

@goetas sorry to bump again, when you are free it would be great to have a new release so we can lock to it :-)

@goetas
Copy link
Member

goetas commented Oct 1, 2020

https://github.com/Masterminds/html5-php/releases/tag/2.7.4

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.

3 participants