Bug 29674 – meta http-equiv Refresh is not honored when 'space' instead of semicolon is used.

• Component: Page Loading
• Status: RESOLVED CONFIGURATION CHANGED

[Jungshik Shin]

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.

[Jungshik Shin]

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.

[Alexey Proskuryakov]

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.

[Jungshik Shin]

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).

[Alexey Proskuryakov]

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.

[Jungshik Shin]

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.

[Jungshik Shin]

(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.

[Jungshik Shin]

(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

[Jungshik Shin]

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.

[Jungshik Shin]

Comment on attachment 40383 [details]
updated patch (passing the existing tests)

length check is missing.

[Jungshik Shin]

Created attachment 40389 [details]
patch updated

The missing length check was added.

[Darin Adler]

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

[Jungshik Shin]

(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.

[Roger Scott]

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.....

[Roger Scott]

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.

[Eric Seidel (no email)]

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.

[Roger Scott]

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?

[Adam Barth]

Comment on attachment 77852 [details]
updated with new tests

I assume this patch is superseded by the later patch.

[Adam Barth]

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.

[Simon Pieters (:zcorpan)]

FYI, the HTML spec was changed here to match IE/Gecko.

https://github.com/whatwg/html/commit/0a3dcdfa15d73334ba6c2a0a12c2f139d526da5a

[Alexey Proskuryakov]

Further discussion in <https://www.w3.org/Bugs/Public/show_bug.cgi?id=28338>.

[Simon Pieters (:zcorpan)]

Spec was fixed in
https://github.com/whatwg/html/pull/246

Tests at
http://w3c-test.org/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/parsing.html

[Ahmad Saleem]

From Test case of W3C in Comment 21, Safari 15.6 passes all 133 test cases similar to all other browsers (Chrome Canary 106 and Firefox Nightly 105). I am going to mark this as "RESOLVED CONFIGURATION CHANGED". Please reopen, if you think it is not fixed. Thanks!