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.

Sign up for GitHub

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

Jump to bottom

x/net/html: Incorrect parsing of form elements within a template element #25703

Closed
a9y opened this issue Jun 2, 2018 · 6 comments
Closed

x/net/html: Incorrect parsing of form elements within a template element #25703

a9y opened this issue Jun 2, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@a9y
Copy link

a9y commented Jun 2, 2018

What version of Go are you using (go version)?

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOOS="linux"

What did you do?

The current implementation of the html parser generates an invalid tree when it encounters a form element nested inside a template element. Every successive sibling to the form will be inserted as children instead.

I encountered this when parsing YouTube's new polymer design. A stripped down version of the section in question:

<template>
  <yt-icon-button></yt-icon-button>
  <form>
    <paper-input></paper-input>
  </form>
  <style></style>
</template>

What did you expect to see?

The following test inside testdata/webkit/template.dat to succeed:

#data
<body><template><yt-icon-button></yt-icon-button><form><paper-input></paper-input></form><style></style></template>
#errors
#document
| <html>
|   <head>
|   <body>
|     <template>
|       content
|         <yt-icon-button>
|         <form>
|           <paper-input>
|         <style>

What did you see instead?

--- FAIL: TestParser (0.04s)
	parse_test.go:233: testdata/webkit/template.dat test #91 "<body><template><yt-icon-button></yt-icon-button><form><paper-input></paper-input></form><style></style></template>", got vs want:
		----
		| <html>
		|   <head>
		|   <body>
		|     <template>
		|       content
		|         <yt-icon-button>
		|         <form>
		|           <paper-input>
		|           <style>
		----
		| <html>
		|   <head>
		|   <body>
		|     <template>
		|       content
		|         <yt-icon-button>
		|         <form>
		|           <paper-input>
		|         <style>
		----

The issue can be resolved by applying the following patch. However, beyond passing the tests associated with the package, no other testing has been performed.

diff --git a/html/parse.go b/html/parse.go
index d23e05e..4739e28 100644
--- a/html/parse.go
+++ b/html/parse.go
@@ -1103,10 +1103,6 @@ func inBodyIM(p *parser) bool {
 					return true
 				}
 				p.generateImpliedEndTags()
-				if p.tok.DataAtom == a.Form {
-					// Ignore the token.
-					return true
-				}
 				p.popUntil(defaultScope, a.Form)
 			} else {
 				node := p.form
@gopherbot gopherbot added this to the Unreleased milestone Jun 2, 2018
@meirf
Copy link
Contributor

meirf commented Jun 3, 2018

cc @nigeltao

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 3, 2018
@nigeltao
Copy link
Contributor

nigeltao commented Jun 4, 2018 •
edited

@namusyaka wrote the <template> support for x/net/html.

@namusyaka namusyaka self-assigned this Jun 5, 2018
@namusyaka
Copy link
Member

Thanks for your report!
I'm going to take a look at the issue, in a few days.

@gopherbot
Copy link

Change https://golang.org/cl/120658 mentions this issue: html: fix incorrect implementation for specification

@namusyaka
Copy link
Member

@a9y Sorry for late response.
I have just mailed my patch for fixing this issue. Could you take a look?

@a9y
Copy link
Author

a9y commented Jun 28, 2018

LGTM

@namusyaka namusyaka added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 29, 2018
@golang golang locked and limited conversation to collaborators Jul 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@namusyaka @agnivade @a9y @meirf @nigeltao @gopherbot