Bug 29674 - meta http-equiv Refresh is not honored when 'space' instead of semicolon is used.
Summary: meta http-equiv Refresh is not honored when 'space' instead of semicolon is u...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Roger Scott
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-23 01:37 PDT by Jungshik Shin
Modified: 2016-04-26 05:36 PDT (History)
4 users (show)

See Also:


Attachments
1-line patch (without a layout test, yet) (639 bytes, patch)
2009-09-23 01:58 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
a patch with layout tests (handling both http refresh and meta refresh) (4.92 KB, patch)
2009-09-29 10:31 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
updated patch (passing the existing tests) (5.12 KB, patch)
2009-09-30 11:25 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch updated (5.13 KB, patch)
2009-09-30 12:20 PDT, Jungshik Shin
darin: review-
Details | Formatted Diff | Diff
updated with new tests (10.69 KB, patch)
2011-01-03 16:40 PST, Roger Scott
no flags Details | Formatted Diff | Diff
Added helper functions (12.20 KB, patch)
2011-01-05 09:51 PST, Roger Scott
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2009-09-23 01:37:01 PDT
Reported in Chromium bug tracker : http://crbug.com/20117. Also reported in the Chrome's Korean user group. 

Two URLs with this problem:

http://exploration.nasa.gov/programs/station/index.html
http://www.sweetmong.com/community/comm_05.php 

Both files uses <meta http-equiv="Refresh" content="0 url........"> 

Note that instead of semicolon, it uses a space. Firefox and IE honor Refresh request in this case. 

With a one-line patch to parseHTTPRefresh in WebCore/platform/network/HTTPParsers.cpp (which currently only works with semicolon/comma), Webkit can be more compatible with Firefox/IE.
Comment 1 Jungshik Shin 2009-09-23 01:58:09 PDT
Created attachment 39981 [details]
1-line patch (without a layout test, yet)

This patch works. I'm not sure if we have to allow 'space' in both http refresh header and html meta http-equiv refresh. Perhaps, we do. 

Anyway, I'll add a layout test and ask for review later.
Comment 2 Alexey Proskuryakov 2009-09-23 08:32:30 PDT
How do IE and Firefox behave with other non-numeric characters? E.g. will "0<url..." work?

> This patch works. I'm not sure if we have to allow 'space' in both http refresh
> header and html meta http-equiv refresh. Perhaps, we do. 

Sounds likely, but testing would answer that better.
Comment 3 Jungshik Shin 2009-09-29 10:31:10 PDT
Created attachment 40308 [details]
a patch with layout tests (handling both http refresh and meta refresh)

I experimented with Firefox and IE and they seem to support 'space', 'comma' and 'semicolon'. My test was not very comprehensive, though. (I just spot-checked a few characters).
Comment 4 Alexey Proskuryakov 2009-09-29 10:54:52 PDT
The reason I was asking is that the other browsers may just use sscanf or equivalent, in which case any non-numeric character would work as a delimiter, and special casing a space wouldn't make a lot of sense.
Comment 5 Jungshik Shin 2009-09-29 11:09:23 PDT
Comment on attachment 40308 [details]
a patch with layout tests (handling both http refresh and meta refresh)

somehow, redirect-with-quotes.php fails with this patch.
Comment 6 Jungshik Shin 2009-09-29 11:15:40 PDT
(In reply to comment #5)
> (From update of attachment 40308 [details])
> somehow, redirect-with-quotes.php fails with this patch.

It's because the test tests a case like

Refresh:      0       ;       url=    \'redirect-step3.php\'  '


BTW, two new tests can be merged into one like redirect-with-quotes.php tests 4 different cases.
Comment 7 Jungshik Shin 2009-09-29 17:15:08 PDT
(In reply to comment #4)
> The reason I was asking is that the other browsers may just use sscanf or
> equivalent, in which case any non-numeric character would work as a delimiter,
> and special casing a space wouldn't make a lot of sense.

At least, Firefox doesn't. HTTP/Meta refresh header is parsed 
by SetupRefreshURIFromHeader() in nsDocShell.cpp :

http://mxr.mozilla.org/firefox/source/docshell/base/nsDocShell.cpp#4745
Comment 8 Jungshik Shin 2009-09-30 11:25:08 PDT
Created attachment 40383 [details]
updated patch (passing the existing tests)

Updated the patch to pass the existing tests. I didn't unify two new tests into one. I'll if it's regarded better that way.
Comment 9 Jungshik Shin 2009-09-30 11:26:22 PDT
Comment on attachment 40383 [details]
updated patch (passing the existing tests)

length check is missing.
Comment 10 Jungshik Shin 2009-09-30 12:20:33 PDT
Created attachment 40389 [details]
patch updated

The missing length check was added.
Comment 11 Darin Adler 2009-10-01 11:55:53 PDT
Comment on attachment 40389 [details]
patch updated

> +    if (pos < len && refresh[pos] == ' ') {
> +       skipWhiteSpace(refresh, pos, fromHttpEquivMeta);
> +       if (refresh[pos] != ',' && refresh[pos] != ';')
> +           --pos;
> +    }

I believe that after calling skipWhiteSpace, pos may point past the end of the buffer, so it's unsafe to access it without checking that it's != len. The return value from skipWhiteSpace will be of help, in fact.

There's no reason to check for the space before calling skipWhiteSpace, since that function already does so.

I think there are way too few tests here. When we have a parser, we need a test that covers all the various branches in the parser, and this adds only one case. We should explore to see if we can find a technique to use where we can test all the various branches in this parser? It's critical that the parser not have bugs like buffer overruns, so thorough testing is valuable.

review- because of the buffer overrun
Comment 12 Jungshik Shin 2009-10-02 11:10:17 PDT
(In reply to comment #11)
> (From update of attachment 40389 [details])
> > +    if (pos < len && refresh[pos] == ' ') {
> > +       skipWhiteSpace(refresh, pos, fromHttpEquivMeta);
> > +       if (refresh[pos] != ',' && refresh[pos] != ';')
> > +           --pos;
> > +    }
> 
> I believe that after calling skipWhiteSpace, pos may point past the end of the
> buffer, so it's unsafe to access it without checking that it's != len. The
> return value from skipWhiteSpace will be of help, in fact.

I'll add that check.

> I think there are way too few tests here. When we have a parser, we need a test
> that covers all the various branches in the parser, and this adds only one
> case. We should explore to see if we can find a technique to use where we can
> test all the various branches in this parser? It's critical that the parser not
> have bugs like buffer overruns, so thorough testing is valuable.

Either I can daisy-chain multiple cases for this issue in a new test or add cases to redirect-with-quotes.php). Perhaps, the former is better.
Comment 13 Roger Scott 2010-11-30 08:50:36 PST
Hello,

Is this fix being actively worked on? I have a similar internal Nokia error related to this. 

I would be happy to continue this work if you would like to assign it to me.....
Comment 14 Roger Scott 2011-01-03 16:40:24 PST
Created attachment 77852 [details]
updated with new tests

I added check of return value from skipWhiteSpace that Jungshik was going to add,
and removed the redundant "check for the space".

Also introduced new technique, as requested by Darin, to support adding many more tests for the parser.  
I refresh/redirect to same php file for each test, using cookies for state. Final test must redirect to "success" html 
or there was a failure. More tests can be easily added.  
I included relevant tests that exist singly in other files: if this technique is acceptable I could remove the other single test
files so that all refresh parser tests are in one place.
Comment 15 Eric Seidel (no email) 2011-01-03 23:38:51 PST
Comment on attachment 77852 [details]
updated with new tests

View in context: https://bugs.webkit.org/attachment.cgi?id=77852&action=review

> WebCore/platform/network/HTTPParsers.cpp:113
> +    while (pos != len && refresh[pos] != ',' && refresh[pos] != ';' && refresh[pos] != ' ')

I would have made this a helper function.  the check against , ; and ' ' that is.  Then it's while (pos != len && !isDelayDelimiter(refresh[pos]))  The function would obviously be inline so as not to affect performance (if this is in any way sensitive?).  But I think that would make it more readable.

> WebCore/platform/network/HTTPParsers.cpp:118
> +            --pos;  // for ++pos below

Wow.  Confusing.

> LayoutTests/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id= 29674

Tab.
Comment 16 Roger Scott 2011-01-05 09:51:08 PST
Created attachment 78012 [details]
Added helper functions

Added helper functions as suggested.
Removed tab.
Hopefully removed confusing part. I think helper functions do remove the confusion and make the code more readable.

Fixed one style check problem involving #include order.
There are a few more style check issues in existing code, (not in my changes)  but if I fix these its harder to follow my changes.
Perhaps I can fix these in later patch?
Comment 17 Adam Barth 2011-04-17 21:56:46 PDT
Comment on attachment 77852 [details]
updated with new tests

I assume this patch is superseded by the later patch.
Comment 18 Adam Barth 2011-04-17 22:01:18 PDT
Comment on attachment 78012 [details]
Added helper functions

View in context: https://bugs.webkit.org/attachment.cgi?id=78012&action=review

R- for nits.  Sorry your patch was up for review for so long.  Please feel free to grab me in #webkit when you've updated your patch for a more speedy review turn around time.  Thanks for working on this issue.  Compatibility with other browsers is very important to the project.

> WebCore/platform/network/HTTPParsers.cpp:76
> +static inline unsigned skipDelay(const String& str, unsigned& pos,  unsigned& len)

Extra space after the comma here.

> WebCore/platform/network/HTTPParsers.cpp:78
> +    while (pos != len && str[pos] != ',' && str[pos] != ';' && str[pos] != ' ')

Usually we use size_t as a type of length.  Also, we prefer full English works for variable names, such as position and length.  Finally, we usually check that position < length rather than position != length.

> WebCore/platform/network/HTTPParsers.cpp:135
> +    if (!skipDelimiter(refresh, pos, fromHttpEquivMeta)) { // no URL 

Comments should also be complete sentences.
Comment 19 Simon Pieters (:zcorpan) 2015-10-11 14:48:41 PDT
FYI, the HTML spec was changed here to match IE/Gecko.

https://github.com/whatwg/html/commit/0a3dcdfa15d73334ba6c2a0a12c2f139d526da5a
Comment 20 Alexey Proskuryakov 2015-10-11 23:10:27 PDT
Further discussion in <https://www.w3.org/Bugs/Public/show_bug.cgi?id=28338>.