#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
---------------------+--------------------
Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4
Keywords: | Blocked By:
Blocking: | Patch: 1
---------------------+--------------------
I found an issue when loading files via wxFileSystem from http server. If
HTTP returns Redirect (3xx) status, wxHTTP treats it as OK and happily
returns error code HTML text as input stream.

It is not expected thing when you loading, say, PNG bitmap, yet you can't
tell that redirect happened - there is no error code

and no attempt from wxFileSystem/wxURL to fetch data via redirected URL.

Attached is a patch to make wxURL detect success/redirect status
and return correct redirected wxInputStream during
wxURL::GetInputStream(), making it recursive, and, possibly, endless :)

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* Attachment "http_3xx.patch" added.

patch common/url.cpp to support http/3xx redirects

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------

Comment (by vadz):

Thanks, but I'm indeed worried about redirect loops, I think we should
stop after some number (10?) of redirects. Ideally we would also handle
this inside `wxInternetFSHandler` rather than in the common code.

Also, more generally, I think what we really need is a new
`wxWebFSHandler` that would handle HTTP and HTTPS URLs using
`wxWebRequest`. This would work much more reliably and is the only way to
support HTTPS.

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:1>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* Attachment "http_3xx_v2.patch" added.

new patch with redirect limit set to 10

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* cc: borsky@… (removed)


Comment:

I updated patch, now with redirect limit. And because of this, instead of
recursive call there is goto to function top. It could be done with
while/loop but I feel that putting while/loop into ifdefs just looks even
worse than goto.

It is a bit harder to put this into wxInternetFSHandler as it needs to
access wxURL' internals. Also I think that wxURL may also be used outside
of wxFileSystem to issue HTTP requests directly and process responses.

I will also look into wxWebRequest/wxWebResponse, looks very interesting!

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:2>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------

Comment (by vadz):

Sorry, I didn't realize this when looking at the first patch, but now I
see that this actually modifies `wxURL` itself and this seems very
surprising to me. I think we'd better add a new method, maybe `static
wxInputStream* wxURL::GetInputStreamFollowingRedirects(const wxString&
url, int maxRedirects = 10)` and use it in `wxInternetFSHandler` instead
of just `GetInputStream()`.

This would also allow to write a normal loop in the new function, so it
should make things cleaner.

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:3>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* cc: borsky@… (added)


Comment:

Let me disagree on that one. A little context is this: some time ago a
client moved some windows share to linux http server (apache). That made
all urls case sensitive ones, breaking lots of links.

There is a simple way to make apache case insensitive with module named
mod_speling. It works by redirecting case-wrong url request to correct
one. And all is good again.

The important part is this: it worked fine with wx2.8 with existing wxHTTP
using native WinNet implementation under Windows. And it breaks
spectaculary under linux. Looks like WinNet will do redirects for us
transparently under the hood.

My proposed patch makes linux port behave similar to windows here without
any need to modify existing code outside wxURL. Side effect is that wxURL
will then point to redirected URL, but only under linux.

Your proposal will leave wxURL::GetInputStream() inconsistent between
platforms. It will load either redirected stream with correct data when
using WinNet or load redirect error message stream under linux. For one,
this will lead to porting/testing problems for any app that make use of
wxURL outside wxFileSystem.

Maybe wxURL::GetInputStream() should return error like HTTP_REDIRECT and
null stream, but then again, we can't do that at least with WinNet.

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:4>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------

Comment (by vadz):

I didn't know that WinINET followed the redirects by default, this does
change things, thanks. However even when using WinINET implementation we
don't modify `wxURL` object itself, so could we at least avoid this, e.g.
by using a temporary object instead of `*this`?

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:5>

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* Attachment "http_3xx_v3.patch" added.

patch that will fix memory leak and correctly process(reject) 300/multiple
redirect

[wxTrac]

#19122: wxHTTP,wxURL,wxFileSystem and HTTP/3xx/Redirect
----------------------+-------------------

Reporter: brambor | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: network | Version: 3.1.4

Resolution: | Keywords:

Blocked By: | Blocking:
Patch: 1 |

----------------------+-------------------
Changes (by brambor):

* cc: borsky@… (removed)


Comment:

I tried several ways to avoid modifying wxURL obect itself, but it always
end up either ugly or broken or both. It seems that we really need
consistency between wxURL, wxHTTP m_protocol member of it and
wxInputStream returned, and that means that wxURL will contain redirected
URL upon return.

I also found that previous patch contained a leak, redirection stream not
deleted (bummer!).

And there is a special case of HTTP redirect code 300 (Multiple Choices)
where
redirect location field is not set at all. Latest patch will return NULL
stream in this case.

--
Ticket URL: <https://trac.wxwidgets.org/ticket/19122#comment:6>