PKP Bugzilla – Full Text Bug Listing
|Summary:||Redirects are broken by Firefox's redirect blocking|
|Product:||OJS||Reporter:||David Allingham <david.allingham>|
|Component:||User Interface||Assignee:||beghelli <bruno.beghelli>|
|Severity:||normal||CC:||alec, bart, h.j.g.warmelink, jason.nugent, luke.skibinski|
|Version Reported In:||Also Affects:|
Description David Allingham 2011-06-06 19:34:11 PDT
When "Warn me when web sites try to redirect or reload the page" is enabled in Firefox's options (Options > Advanced > General), it breaks the redirects used in OCS 126.96.36.199. This happens with Firefox 4 and 5beta. Most other sites work fine, with a "this site tried to redirect [allow]" notification, but whatever mechanism OCS uses for its redirects are not detected by Firefox but are blocked, and the user ends up at a blank page. Reloading on the blank page takes the user to the desired location, but if it was a form submission, the submission is made twice.
Comment 1 Alec Smecher 2011-06-07 13:31:21 PDT
This may well be a result of bug #3520 -- you could try reversing that commit if you like, but we'd need to find a solution that works in both cases.
Comment 2 Alec Smecher 2011-06-07 13:31:40 PDT
(Filing against 2.3.4 for next release.)
Comment 3 Alec Smecher 2011-08-24 11:26:34 PDT
Comment 4 Alec Smecher 2012-03-12 10:19:39 PDT
Comment 5 h.j.g.warmelink 2012-05-02 08:06:30 PDT
Since Alec has requested feedback... I think this issue is still occurring with my installation of OJS and Firefox. I've installed OJS recently purely for testing purposes. In Firefox I often get blank screens, e.g. upon login/logout. After a refresh I get the standard 'this site is trying to redirect you' warning. If I click Allow all goes well. In Internet Explorer all this doesn't happen - everything works perfectly. Given Allingham descriptions I am confident I am experiencing the same issue, even though he was referring to OCS at the time. Details: OJS version 188.8.131.52 installed on Linux with Apache 2.0.46, PHP 5.3.3 and MySQL 4.1.21. Using Firefox 11 and Internet Explorer 8 on Windows 7 in the test.
Comment 6 Alec Smecher 2012-05-02 09:58:54 PDT
Thanks; the code is indeed shared between OJS and OCS so it's likely you're seeing the same issue. Scheduling against 2.3.8.
Comment 7 Bart Nagel 2012-08-14 23:26:39 PDT
Reverting the commit for bug #3520 does indeed fix the issue. One solution without reverting that would be to do something like echo "Redirect to <a href='$url'>$url</a>"; before the header() call in PKPRequest.inc.php's redirectURL method. That way the redirect still works if the browser isn't stopping them such as in this case, and if the browser is stopping them the user still sees a link to the destination page and can click it. It could be a plain link or a fully styled page, but note that the server has transmitted and the user has received the full page even if the redirect is going to work anyway, which is a little wasteful. A plain link exists in my commit at https://github.com/tremby/pkp-lib/commit/8d4d2a0f53908168477a3c8cb61f09773795fc92 Any thoughts?
Comment 8 Alec Smecher 2012-08-15 07:40:41 PDT
Bart, have you tried that proposal? OTOH it will cause a "Cannot modify header information - headers already sent by ..." warning and break the redirect header.
Comment 9 Bart Nagel 2012-08-15 10:09:53 PDT
Excellent point -- I should get that error but I have tried it and no error. I honestly don't know why that is, and will investigate. I hadn't seen the "Refresh" header before, maybe it works differently somehow. But to be safe there's no reason the echo can't go /after/ the header, and everything should work. I'll test this when I'm back home and let you know.
Comment 10 Jason Nugent 2012-08-15 10:14:44 PDT
Bart, you won't see the error if you have output buffering turned on in your PHP configuration. That's a mixed bag option since some enable it for performance reasons, but I believe that it comes disabled by default.
Comment 11 Bart Nagel 2012-08-15 10:21:48 PDT
That makes sense, thanks. A patch putting the link after the header: https://github.com/tremby/pkp-lib/commit/c34182bee73ed7ec88833bbe1105b5c57b0406e6 Do you want it to send a full page of HTML to frame this link? Or I can do a pull request of this commit.
Comment 12 Alec Smecher 2012-08-21 13:10:18 PDT
Deferring to 2.4.1. Bart, I left a comment on that commit re: localizing text.
Comment 13 h.j.g.warmelink 2012-08-22 04:17:21 PDT
Thanks guys for looking into this. I'm guessing you'll be adding the final patch to http://pkp.sfu.ca/wiki/index.php/OJS_2.3.7_Recommended_Patches once you're happy with it? Since the issue itself is deferred to v2.4.1?
Comment 14 Alec Smecher 2012-08-28 13:41:00 PDT
We tend to keep the "recommended patches" list to high-priority issues, and we haven't had very many reports of this one causing problems. We don't yet have a suitable implementation to include with 2.4.0, so it'll most likely come out with 2.4.1. I'd suggest staying CC'd on this entry if you'd like to apply a patch before we release an official version with the fix included.
Comment 15 Bart Nagel 2012-08-31 22:23:59 PDT
Here's a new attempt, using the error template and a new language key: https://github.com/tremby/pkp-lib/commit/fdf4774f02567a919154d02070b88b1c83ace951
Comment 16 Alec Smecher 2012-11-28 12:57:48 PST
According to http://pkp.sfu.ca/support/forum/viewtopic.php?f=8&t=9356 there might be side-effects to using Refresh: for bots and crawlers. I wonder if the original modification wasn't mis-diagnosed -- I don't see references to IE ignoring hashes in some quick googling. Suggest starting with some IE testing with the original Location: header.
Comment 17 Alec Smecher 2012-12-19 12:08:31 PST
Bruno, since you've got the best browser testing setup, could you take a look at this one? Start from comment #16; I'm not happy that we're currently straying so far from standard behavior and I'll bet it's unnecessary.
Comment 18 Alec Smecher 2012-12-21 10:10:11 PST
*** Bug 8070 has been marked as a duplicate of this bug. ***
Comment 19 beghelli 2013-01-10 12:29:48 PST
(In reply to comment #17) > Bruno, since you've got the best browser testing setup, could you take a > look at this one? Start from comment #16; I'm not happy that we're currently > straying so far from standard behavior and I'll bet it's unnecessary. Tested in IE7 and IE8 and both header("Location: $url") and header("Refresh: 0; url=$url") works well with anchors. I was redirected and the anchor was followed. IE6 is still a concern? I can test if it is.
Comment 20 Alec Smecher 2013-01-10 16:34:25 PST
No, I think at this point we can drop IE6 support. Go ahead and revert bug #3520.
Comment 21 Luke Skibinski 2013-01-11 01:53:47 PST
Comment 22 Alec Smecher 2013-01-11 08:45:10 PST
Thanks for the confirmation, Luke. Addressing IE's High security level is outside the scope of this bug, but if you do have any suggestions, feel free to drop them in.
Comment 23 beghelli 2013-01-28 11:50:03 PST
Comment 24 beghelli 2013-01-28 11:50:03 PST