Css parsing fails if @charset is present in css.

Css parsing fails if @charset is present in the css. 

Why it fails?
Actual CSS grammar: (ref : http://www.w3.org/TR/CSS21/grammar.html)
 stylesheet
   : [ CHARSET_SYM STRING ';' ]?
     [S|CDO|CDC]* [ import [S|CDO|CDC]* ]*
     [ [ ruleset | media | page ] [S|CDO|CDC]* ]*

As per the code CSS grammar of the css parser
 stylesheet
   : [S|CDO|CDC]* [ import [S|CDO|CDC]* ]*
     [ [ ruleset | media | page ] [S|CDO|CDC]* ]*

[satya3656, 2011-07-04 13:56:31]

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:225: skipTopLevelIgnorables();
@charset if present should be first in the file, you can't have comments section
or white spaces before it.

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:280: charset = new
CssTree.StringLiteral(t.pos, s);
You need to also check if it a valid IANA charset mime type. Actually parser
should use this char set while reading and writing strings. May be add a TODO
and go ahead if it is ok to do it later.

[pulkitgoyal2000, 2011-07-04 14:34:33]

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:225: skipTopLevelIgnorables();
On 2011/07/04 13:56:31, satya3656 wrote:
> @charset if present should be first in the file, you can't have comments
section
> or white spaces before it. 

Done.

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:280: charset = new
CssTree.StringLiteral(t.pos, s);
On 2011/07/04 13:56:31, satya3656 wrote:
> You need to also check if it a valid IANA charset mime type. Actually parser
> should use this char set while reading and writing strings. May be add a TODO
> and go ahead if it is ok to do it later.

I dont know whether this is the right place to check whether it is a valid IANA
character or not as we have to parse the css based on its grammar which says it
should be String like while parsing tags we dont check the correctness of its
attributes. 
Anyway I added TODO...

[anupama.dutta, 2011-07-04 15:24:24]

Some high level suggestions (summarizing my offline discussion with Pulkit):
Ideally, we should do the following:
1) Do the IANA charset check for the specified @charset string (as Satya
suggested)
2) Apply this charset to the css for decoding in some way (again Satya's
suggestion).

Since, the css seems to be handled as utf-8 throughout the current code, I
suggest that we check for "utf-8" (case insensitive) charset and if this is the
specified charset, we can handle it with the code Pulkit has added. If not, we
can write out a message (indicating that non-utf-8 charset has been specified)
and skip just the charset line and continue with the rest of the css (as was
being done before). Even for the non-utf-8 cases, we will be doing better than
before, because we will parse the @import line after the @charset line now. We
can add a TODO for the non-utf-8 case and discuss and see how best this can be
handled in a later patch.

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:274: expectSymbol("@charset");
Should we check for "@charset " with an ending space?

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssTree.java:240: 
Please add serialVersionUID using the following steps:
1) Run
serialver -classpath <path-to-pluginc.jar>/pluginc.jar -show
2) In the window that opens up, enter the full path for the "Class Name", e.g.
com.google.caja.parser.css.CssTree.Charset

[anupama.dutta, 2011-07-04 15:26:55]

Adding back the discussion groups so that the suggestions are visible to
all.

Thanks,
Anupama.

On Mon, Jul 4, 2011 at 8:54 PM, <anupama.dutta@gmail.com> wrote:

> Some high level suggestions (summarizing my offline discussion with
> Pulkit):
> Ideally, we should do the following:
> 1) Do the IANA charset check for the specified @charset string (as Satya
> suggested)
> 2) Apply this charset to the css for decoding in some way (again Satya's
> suggestion).
>
> Since, the css seems to be handled as utf-8 throughout the current code,
> I suggest that we check for "utf-8" (case insensitive) charset and if
> this is the specified charset, we can handle it with the code Pulkit has
> added. If not, we can write out a message (indicating that non-utf-8
> charset has been specified) and skip just the charset line and continue
> with the rest of the css (as was being done before). Even for the
> non-utf-8 cases, we will be doing better than before, because we will
> parse the @import line after the @charset line now. We can add a TODO
> for the non-utf-8 case and discuss and see how best this can be handled
> in a later patch.
>
>
>
> http://codereview.appspot.com/**4662076/diff/9001/src/com/**
>
google/caja/parser/css/**CssParser.java<http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/css/CssParser.java>
> File src/com/google/caja/parser/**css/CssParser.java (right):
>
> http://codereview.appspot.com/**4662076/diff/9001/src/com/**
>
google/caja/parser/css/**CssParser.java#newcode274<http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/css/CssParser.java#newcode274>
> src/com/google/caja/parser/**css/CssParser.java:274:
> expectSymbol("@charset");
> Should we check for "@charset " with an ending space?
>
> http://codereview.appspot.com/**4662076/diff/9001/src/com/**
>
google/caja/parser/css/**CssTree.java<http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/css/CssTree.java>
> File src/com/google/caja/parser/**css/CssTree.java (right):
>
> http://codereview.appspot.com/**4662076/diff/9001/src/com/**
>
google/caja/parser/css/**CssTree.java#newcode240<http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/css/CssTree.java#newcode240>
> src/com/google/caja/parser/**css/CssTree.java:240:
> Please add serialVersionUID using the following steps:
> 1) Run
> serialver -classpath <path-to-pluginc.jar>/pluginc.**jar -show
> 2) In the window that opens up, enter the full path for the "Class
> Name", e.g. com.google.caja.parser.css.**CssTree.Charset
>
>
>
http://codereview.appspot.com/**4662076/<http://codereview.appspot.com/4662076/>
>

[MikeSamuel, 2011-07-04 19:54:12]

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssParser.java:287: List<CssTree> children =
Lists.newArrayList(1);
Prefer
    List<CssTree> children = Collections.<CssTree>singletonList(charset);

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssTree.java:239: public static final class
Charset extends CssStatement {
This class should probably have zero children, and instead of having a
StringLiteral containing the charset, the charset should be the string value
(implement getCharset() in terms of getValue()) of the node.

[pulkitgoyal2000, 2011-07-05 06:42:17]

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssParser.java:287: List<CssTree> children =
Lists.newArrayList(1);
On 2011/07/04 19:54:12, MikeSamuel wrote:
> Prefer
>     List<CssTree> children = Collections.<CssTree>singletonList(charset);
As Charset class has no child, so this line is removed

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/13001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssTree.java:239: public static final class
Charset extends CssStatement {
On 2011/07/04 19:54:12, MikeSamuel wrote:
> This class should probably have zero children, and instead of having a
> StringLiteral containing the charset, the charset should be the string value
> (implement getCharset() in terms of getValue()) of the node.

Done.

[Jasvir, 2011-07-05 09:30:45]

LGTM

+1 to anupama's summary.  The patch renders @charsets with UTF-8 only.  HTTP
headers override the charset declared indocument including in @charset decls. 
Clients should take care to ensure the rendered document is only served with
UTF-8 encoding in HTTP Content-Type header to avoid misinterpretation of content
by browsers.

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssParser.java:284: mq.getMessages().add(msg);
mq.addMessage(msg) should be fine.

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssTree.java:239: public static final class
Charset extends CssStatement {
Note: This node does not need to be stripped out by CssRewriter since the
renderer expects to render UTF-8 which is the currently the only charset
accepted by @charset.  If other charsets are accepted, CssRewriter will need to
be updated to prevent charset confusion attacks.

[pulkitgoyal2000, 2011-07-05 10:20:46]

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:274: expectSymbol("@charset");
On 2011/07/04 15:24:24, anupama.dutta wrote:
> Should we check for "@charset " with an ending space?
No... css grammar has no reference about this space..

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/9001/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssTree.java:240: 
On 2011/07/04 15:24:24, anupama.dutta wrote:
> Please add serialVersionUID using the following steps:
> 1) Run
> serialver -classpath <path-to-pluginc.jar>/pluginc.jar -show
> 2) In the window that opens up, enter the full path for the "Class Name", e.g.
> com.google.caja.parser.css.CssTree.Charset

Done.

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/15001/src/com/google/caja/parser/c...
src/com/google/caja/parser/css/CssParser.java:284: mq.getMessages().add(msg);
On 2011/07/05 09:30:45, jasvir wrote:
> mq.addMessage(msg) should be fine.

Done.

[anupama.dutta, 2011-07-05 11:37:55]

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:225: if
(lookaheadSymbol("@charset")) {
In http://www.w3.org/TR/CSS21/grammar.html, the CHARSET_SYM is allowed to match
only "@charset " - the ending space is important, so we should lookahead for
this exact string.

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:273: expectSymbol("@charset");
Add ending space here as well.

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:279: //TODO Handle non utf-8
charset
// TODO(pulkitgoyal2000):

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssTree.java:258: 
Extra newline.

[pulkitgoyal2000, 2011-07-05 12:27:49]

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssParser.java (right):

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:225: if
(lookaheadSymbol("@charset")) {
On 2011/07/05 11:37:55, anupama.dutta wrote:
> In http://www.w3.org/TR/CSS21/grammar.html, the CHARSET_SYM is allowed to
match
> only "@charset " - the ending space is important, so we should lookahead for
> this exact string.

Done.

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:273: expectSymbol("@charset");
On 2011/07/05 11:37:55, anupama.dutta wrote:
> Add ending space here as well.

Done.

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssParser.java:279: //TODO Handle non utf-8
charset
On 2011/07/05 11:37:55, anupama.dutta wrote:
> // TODO(pulkitgoyal2000): 

Done.

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
File src/com/google/caja/parser/css/CssTree.java (right):

http://codereview.appspot.com/4662076/diff/6004/src/com/google/caja/parser/cs...
src/com/google/caja/parser/css/CssTree.java:258: 
On 2011/07/05 11:37:55, anupama.dutta wrote:
> Extra newline.

Done.

[anupama.dutta, 2011-07-05 15:04:03]

LGTM.

[pulkitgoyal2000, 2011-07-05 17:25:19]

Thanks..
committed as r4536