[Webkit-unassigned] [Bug 32249] WebSocket test server handshake is not strict enough

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32249





--- Comment #9 from Yuzo Fujishima <yuzo at google.com>  2009-12-15 23:56:07 PST ---
(In reply to comment #7)
> (From update of attachment 44524 [details])
> > +        "--strict",
> 
> I don't think this should be an option - why would we want to encourage sloppy
> implementations?

Two reasons:
- pywebsocket is for general (although experimental) use, and
generally speaking, the receiver of data should be lenient.
The order of headers is minor enough to tolerate, in my opinion.
- strict checking is difficult to enforce in the Apache module version
of pywebsocket, and we want to keep the behavior as consistent as
possible.

> 
> > +_FIRST_FIVE_LINES = map(re.compile, [
> > +    r'^GET [\S]+ HTTP/1.1\r\n$',
> 
> The spec also says that path must begin with a slash.

Done.

> 
> > +_SIXTH_AND_LATER = re.compile(
> > +    r'^'
> > +    r'(WebSocket-Protocol: [\x20-\x7e]+\r\n)?'
> > +    r'([Cc][Oo][Oo][Kk][Ii][Ee]2?:[^\r]*(\r\n[ \t][^\r]*)*\r\n)?'
> > +    r'\r\n')
> 
> The Cookie header must also be in the restricted WebSocket form. The spec says
> its semantics matches HTTP, but doesn't say anything special about syntax.
> 
> Does this allow having both Cookie and Cookie2 headers in a single request?

Fixed to allow both Cookie and Cookie2 in a same request.

> 
> >      for c in protocol:
> > -        if not 0x21 <= ord(c) <= 0x7e:
> > +        if not 0x20 <= ord(c) <= 0x7e:
> >              raise HandshakeError('Illegal character in protocol: %r' % c)
> 
> I'm confused - bug 32266 has already introduced a test verifying that space is
> allowed in protocol name. How could that possibly work before this change?

The test added by 32266 is currently skipped.

> 
> > +_STRICTLY_GOOD_REQUESTS = (
> <...>
> > +    (  # Cookie with continuation lines
> 
> This shouldn't be allowed - all handshake header fields are subject to
> WebSocket syntax.

Fixed.

> 
> > +_NOT_STRICTLY_GOOD_REQUESTS = (
> > +    (  # Extra space after GET
> > +        'GET  /demo HTTP/1.1\r\n',
> > +        'upgrade: WebSocket\r\n',
> 
> Upgrade doesn't have a correct case here.

Good catch. Thank you!

> 
> Please also test path without a slash at the beginning.

Added.

> 
> > +    (  # Origin comes before Host
> 
> Please also test incorrect order of Upgrade and Connection

Added.

> 
> > +    (  # Unknow header
> 
> Typo: unknown.

Fixed.

> 
> > +# vi:sts=4 sw=4 et
> 
> We don't normally allow modelines in WebKit sources.

pywebsocket follows a coding rule different form that of WebKit. 

> 
> Looks good, thanks for tackling this! I can't deeply review Python code, but it
> looks reasonable to me, too.

Thank you for the review. I'll upload a new patch right after this.

Yuzo

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.