code review 5119041: exp/template/html: handle custom attrs and HTML5 embedd...

exp/template/html: handle custom attrs and HTML5 embedded elements.

HTML5 allows embedded SVG and MathML.
Code searches show SVG is used for graphing.

This changes transition to deal with constructs like
   <svg xmlns:xlink="http://www.w3.org/1999/xlink">
It changes attr and clients to call a single function that combines
the name lookup and "on" prefix check to determine an attribute
value type given an attribute name.

That function uses heuristics to recognize that
     xlink:href and svg:href
have URL content, and that data-url is likely contains URL content,
since "javascript:" injection is such a problem.

I did a code search over a closure templates codebase to determine
patterns of custom attribute usage.  I did something like

$ find . -name \*.soy | \
    xargs egrep perl -ne 'while (s/\b((data-|\w+:)\w+)\s*=//) { print "$1\n"; }' | \
    sort | uniq

to produce the list at the bottom.

Filtering that by egrep -i 'src|url|uri' produces

data-docConsumptionUri
data-docIconUrl
data-launchUrl
data-lazySrc
data-pageUrl
data-shareurl
data-suggestServerUrl
data-tweetUrl
g:secondaryurls
g:url

which seem to match all the ones that are likely URL content.
There are some short words that match that heuristic, but I still think it decent since
any custom attribute that has a numeric or enumerated keyword value will be unaffected by
the URL assumption.
Counterexamples from /usr/share/dict:
during, hourly, maturity, nourish, purloin, security, surly

Custom attributes present in existing closure templates codebase:
buzz:aid
data-a
data-action
data-actor
data-allowEqualityOps
data-analyticsId
data-bid
data-c
data-cartId
data-categoryId
data-cid
data-command
data-count
data-country
data-creativeId
data-cssToken
data-dest
data-docAttribution
data-docConsumptionUri
data-docCurrencyCode
data-docIconUrl
data-docId
data-docPrice
data-docPriceMicros
data-docTitle
data-docType
data-docid
data-email
data-entityid
data-errorindex
data-f
data-feature
data-fgid
data-filter
data-fireEvent
data-followable
data-followed
data-hashChange
data-height
data-hover
data-href
data-id
data-index
data-invitable
data-isFree
data-isPurchased
data-jid
data-jumpid
data-launchUrl
data-lazySrc
data-listType
data-maxVisiblePages
data-name
data-nid
data-nodeid
data-numItems
data-numPerPage
data-offerType
data-oid
data-opUsesEquality
data-overflowclass
data-packageName
data-pageId
data-pageUrl
data-pos
data-priceBrief
data-profileIds
data-query
data-rating
data-ref
data-rentalGrantPeriodDays
data-rentalactivePeriodHours
data-reviewId
data-role
data-score
data-shareurl
data-showGeLe
data-showLineInclude
data-size
data-sortval
data-suggestServerType
data-suggestServerUrl
data-suggestionIndex
data-tabBarId
data-tabBarIndex
data-tags
data-target
data-textColor
data-theme
data-title
data-toggletarget
data-tooltip
data-trailerId
data-transactionId
data-transition
data-ts
data-tweetContent
data-tweetUrl
data-type
data-useAjax
data-value
data-width
data-x
dm:index
dm:type
g:aspects
g:decorateusingsecondary
g:em
g:entity
g:groups
g:id
g:istoplevel
g:li
g:numresults
g:oid
g:parentId
g:pl
g:pt
g:rating_override
g:secondaryurls
g:sortby
g:startindex
g:target
g:type
g:url
g:value
ga:barsize
ga:css
ga:expandAfterCharsExceed
ga:initialNumRows
ga:nocancelicon
ga:numRowsToExpandTo
ga:type
ga:unlockwhenrated
gw:address
gw:businessname
gw:comment
gw:phone
gw:source
ng:controller
xlink:href
xml:lang
xmlns:atom
xmlns:dc
xmlns:jstd
xmlns:ng
xmlns:og
xmlns:webstore
xmlns:xlink

[MikeSamuel, 2011-09-23 20:19:53]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2011-09-25 09:03:13]

LGTM.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
File src/pkg/exp/template/html/attr.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
src/pkg/exp/template/html/attr.go:94: // since we have special handling in
attrOfType.
s/attrOfType/typeOfAttr/, but I'd call the function attrType, and the map
variable attrTypeMap.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
src/pkg/exp/template/html/attr.go:169: if strings.Index(name, "src") != -1 ||
Use strings.Contains instead of strings.Index.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape_test.go:1363: `<a g:url`,
Can I get a test for `<a g:`?

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
File src/pkg/exp/template/html/transition.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
src/pkg/exp/template/html/transition.go:515: func isASCIIDigit(c byte) bool {
isASCIILetterOrDigit seems like a more useful function. You could also drop the
"is", I'd name the functions "asciiAlpha" and "asciiAlphaNum".

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
src/pkg/exp/template/html/transition.go:520: func eatTagName(s []byte, i int)
(int, element) {
I'd write the function body as:
----
if i == len(s) || !asciiAlpha(s[i]) {
  return i, elementNone
}
j := i + 1
for j < len(s) {
  x := s[j]
  if asciiAlphaNum(x) {
    j++
    continue
  }
  // Allow "x-y" or "x:y" but not "x-", "-y", or "x--y".
  if (x == ':' || x == '-') && j+1 < len(s) && asciiAlphaNum(s[j+1]) {
    j += 2
    continue
  }
  break
}
return j, elementNameMap[strings.ToLower(string(s[i:j]))]
----

[MikeSamuel, 2011-09-26 11:50:32]

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
File src/pkg/exp/template/html/attr.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
src/pkg/exp/template/html/attr.go:94: // since we have special handling in
attrOfType.
On 2011/09/25 09:03:13, nigeltao wrote:
> s/attrOfType/typeOfAttr/, but I'd call the function attrType, and the map
> variable attrTypeMap.

Done.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/att...
src/pkg/exp/template/html/attr.go:169: if strings.Index(name, "src") != -1 ||
On 2011/09/25 09:03:13, nigeltao wrote:
> Use strings.Contains instead of strings.Index.

Done.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape_test.go:1363: `<a g:url`,
On 2011/09/25 09:03:13, nigeltao wrote:
> Can I get a test for `<a g:`?

Done.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
File src/pkg/exp/template/html/transition.go (right):

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
src/pkg/exp/template/html/transition.go:515: func isASCIIDigit(c byte) bool {
On 2011/09/25 09:03:13, nigeltao wrote:
> isASCIILetterOrDigit seems like a more useful function. You could also drop
the
> "is", I'd name the functions "asciiAlpha" and "asciiAlphaNum".

Renamed.

Before this CL, tag names were defined as a letter followed by alphanums.  I
didn't define an AlphaNum function so as not to allow tag names that start with
digits.

http://codereview.appspot.com/5119041/diff/2005/src/pkg/exp/template/html/tra...
src/pkg/exp/template/html/transition.go:520: func eatTagName(s []byte, i int)
(int, element) {
On 2011/09/25 09:03:13, nigeltao wrote:
> I'd write the function body as:
> ----
> if i == len(s) || !asciiAlpha(s[i]) {
>   return i, elementNone
> }
> j := i + 1
> for j < len(s) {
>   x := s[j]
>   if asciiAlphaNum(x) {
>     j++
>     continue
>   }
>   // Allow "x-y" or "x:y" but not "x-", "-y", or "x--y".
>   if (x == ':' || x == '-') && j+1 < len(s) && asciiAlphaNum(s[j+1]) {
>     j += 2
>     continue
>   }
>   break
> }
> return j, elementNameMap[strings.ToLower(string(s[i:j]))]
> ----

Done.

[MikeSamuel, 2011-09-28 21:07:52]

*** Submitted as http://code.google.com/p/go/source/detail?r=224c7e1777ed ***

exp/template/html: handle custom attrs and HTML5 embedded elements.

HTML5 allows embedded SVG and MathML.
Code searches show SVG is used for graphing.

This changes transition to deal with constructs like
   <svg xmlns:xlink="http://www.w3.org/1999/xlink">
It changes attr and clients to call a single function that combines
the name lookup and "on" prefix check to determine an attribute
value type given an attribute name.

That function uses heuristics to recognize that
     xlink:href and svg:href
have URL content, and that data-url is likely contains URL content,
since "javascript:" injection is such a problem.

I did a code search over a closure templates codebase to determine
patterns of custom attribute usage.  I did something like

$ find . -name \*.soy | \
    xargs egrep perl -ne 'while (s/\b((data-|\w+:)\w+)\s*=//) { print "$1\n"; }'
| \
    sort | uniq

to produce the list at the bottom.

Filtering that by egrep -i 'src|url|uri' produces

data-docConsumptionUri
data-docIconUrl
data-launchUrl
data-lazySrc
data-pageUrl
data-shareurl
data-suggestServerUrl
data-tweetUrl
g:secondaryurls
g:url

which seem to match all the ones that are likely URL content.
There are some short words that match that heuristic, but I still think it
decent since
any custom attribute that has a numeric or enumerated keyword value will be
unaffected by
the URL assumption.
Counterexamples from /usr/share/dict:
during, hourly, maturity, nourish, purloin, security, surly

Custom attributes present in existing closure templates codebase:
buzz:aid
data-a
data-action
data-actor
data-allowEqualityOps
data-analyticsId
data-bid
data-c
data-cartId
data-categoryId
data-cid
data-command
data-count
data-country
data-creativeId
data-cssToken
data-dest
data-docAttribution
data-docConsumptionUri
data-docCurrencyCode
data-docIconUrl
data-docId
data-docPrice
data-docPriceMicros
data-docTitle
data-docType
data-docid
data-email
data-entityid
data-errorindex
data-f
data-feature
data-fgid
data-filter
data-fireEvent
data-followable
data-followed
data-hashChange
data-height
data-hover
data-href
data-id
data-index
data-invitable
data-isFree
data-isPurchased
data-jid
data-jumpid
data-launchUrl
data-lazySrc
data-listType
data-maxVisiblePages
data-name
data-nid
data-nodeid
data-numItems
data-numPerPage
data-offerType
data-oid
data-opUsesEquality
data-overflowclass
data-packageName
data-pageId
data-pageUrl
data-pos
data-priceBrief
data-profileIds
data-query
data-rating
data-ref
data-rentalGrantPeriodDays
data-rentalactivePeriodHours
data-reviewId
data-role
data-score
data-shareurl
data-showGeLe
data-showLineInclude
data-size
data-sortval
data-suggestServerType
data-suggestServerUrl
data-suggestionIndex
data-tabBarId
data-tabBarIndex
data-tags
data-target
data-textColor
data-theme
data-title
data-toggletarget
data-tooltip
data-trailerId
data-transactionId
data-transition
data-ts
data-tweetContent
data-tweetUrl
data-type
data-useAjax
data-value
data-width
data-x
dm:index
dm:type
g:aspects
g:decorateusingsecondary
g:em
g:entity
g:groups
g:id
g:istoplevel
g:li
g:numresults
g:oid
g:parentId
g:pl
g:pt
g:rating_override
g:secondaryurls
g:sortby
g:startindex
g:target
g:type
g:url
g:value
ga:barsize
ga:css
ga:expandAfterCharsExceed
ga:initialNumRows
ga:nocancelicon
ga:numRowsToExpandTo
ga:type
ga:unlockwhenrated
gw:address
gw:businessname
gw:comment
gw:phone
gw:source
ng:controller
xlink:href
xml:lang
xmlns:atom
xmlns:dc
xmlns:jstd
xmlns:ng
xmlns:og
xmlns:webstore
xmlns:xlink

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/5119041