Closed Bug 887940 Opened 12 years ago Closed 12 years ago

Redirect non-latest, non-ESR versions of Firefox Desktop download requests to /firefox/new/

Categories

(www.mozilla.org :: Legacy PHP system, defect)

Product:
www.mozilla.org
Component:
Legacy PHP system
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: cmore, Assigned: kohei)

References

(Blocks 1 open bug)

Details

(Whiteboard: [kb=1038602] r=117883,118477,118478 b=trunk)

Attachments

(1 file, 3 obsolete files)

proposed patch 2.0
917 bytes, patch
osmose
: review+
Details | Diff | Splinter Review
Related to bug 886316, we have a lot of traffic to this URL from recent versions of Firefox: /en-US/products/download.html The top 5 requested versions are: /en-US/products/download.html?product=firefox-17.0.1&os=win&lang=en-US /en-US/products/download.html?product=firefox-15.0.1&os=win&lang=en-US /en-US/products/download.html?product=firefox-20.0&os=win&lang=en-US /en-US/products/download.html?product=firefox-16.0.2&os=win&lang=en-US /en-US/products/download.html?product=firefox-19.0.2&os=win&lang=en-US The majority of the downloads are to the first two links and most of them are coming from bing.com searches. I propose an intelligent redirect in the php that will use the product details meta data. Here's the logic that I am thinking: If ( product version >= 4.0 and product version <= latest-version-1) && (cgi. referrer does not contain "mozilla.org") && (product does not contain "esr") Then redirect: /firefox/new/ (no locale) This logic will redirect anyone trying to download non-ESR versions newer than version 4 up to the previous version but not any links from mozilla.org. This will have to be in the php logic here: http://viewvc.svn.mozilla.org/vc/projects/mozilla.com/trunk/en-US/products/download.html?view=markup Why is this urgent to do this now? Over the past 30 days 128k downloads that matches the logic above. This php page will be going away eventually, but for now, we are really damaging the onboarding experience by giving people a download an old version of Firefox.
Taking. I'd like to resolve Bug 710824 as well.
Assignee: nobody → kohei.yoshino.bugs
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Component: General → Legacy PHP system
BTW, I'd like to avoid referrer check; it's just annoying. Many people, including me, have disabled referrer on their browser or by a security software.
(In reply to Kohei Yoshino [:kohei] from comment #2) > BTW, I'd like to avoid referrer check; it's just annoying. Many people, > including me, have disabled referrer on their browser or by a security > software. Yeah, but you're probably a minority and power users probably won't fall into this SEO trap. I wanted to avoid messing up any links on mozilla.org that are purposely to those older version.
(In reply to Kohei Yoshino [:kohei] from comment #1) > Taking. I'd like to resolve Bug 710824 as well. I'd rather focus on getting this non-latest bug fixed first. As for bug 710814, I don't know if it is worth it since this php page will be going away all together later this year when download buttons are pointed to the second scene of the unified product download page. Let's focus on this redirect first.
I think the referrer check is fine. I have referrer turned off for different domains, but on for interdomain links so that things like Django's CSRF stuff doesn't fail for some logins. In cmore's logic I think it'll be fine, even in the case with referrers turned off completely, they'll just get the new version, which is what they probably meant. Are you sure that "esr" is in the product string for those builds? I know for the "do you need an update" logic on the site we just have to go by the version number (10 or 17), but I've never looked into the download flow for ESR builds. I'm not aware of any links to ESR builds on the site actually. The only other thing I can think of is that if this isn't what the people meant, and they really did want an old version, we could potentially add some messaging to the download page saying something like (you're downloading the latest version of Firefox. To download the older version from the link you followed, click here). That might be overly confusing. Also, my guess is that any page pointing to older builds deliberately will point directly at bouncer. So this is probably fine as stated by cmore.
ESR links are download from here: http://www.mozilla.org/en-US/firefox/organizations/all.html The links go directly to download.mozilla.org and not /en-US/products/download.html. Notice the "esr" at the end of the product query parameter. I have seen some requests to the /products/download.html with "esr" as a substring in the product parameter.
Yes, what Raymond said. :)
Okay, I'll add a referrer check. Bug 710824 will be resolved at the same time, as we anyway check > isset($_GET['product']) We can ignore ESR because all those links directly point to the bouncer. How about 3.6.x or older versions?
Attached patch proposed patch (obsolete) — — Splinter Review
This allows users to download only the latest beta and release versions.
Oops, the redirect should be permanent like this > header('Location: /firefox/new/', true, 302);
Attached patch proposed patch 1.1 (obsolete) — — Splinter Review
Attachment #768574 - Attachment is obsolete: true
(In reply to Kohei Yoshino [:kohei] from comment #12) > Created attachment 768617 [details] [diff] [review] > proposed patch 1.1 Nice work! If we can get pmac to review, I would like to test this on -dev. We will have to manually update the product details on dev since it hasn't been updated from prod in quite a while.
Comment on attachment 768617 [details] [diff] [review] proposed patch 1.1 Review of attachment 768617 [details] [diff] [review]: ----------------------------------------------------------------- r+ besides my one comment, do you need me to merge or anything? ::: en-US/products/download.html @@ +5,5 @@ > + strpos($_SERVER['HTTP_REFERER'], > + $config['url_scheme'] . '://' . $config['server_name']) !== 0) { > + return false; > + } > + list($product, $version) = explode('-', $_GET['product'], 2); I'm rusty on my PHP, but I believe this will vomit if product doesn't have a - in it. Can you catch this error (or do a check beforehand) and return false if it fails?
Attachment #768617 - Flags: review+
I glanced at the patch and it uses short array syntax which requires an up to date version of PHP (>5.4). I don't know which version we have as it is not documented, but historically we have always had old versions of PHP on mozilla.org, we are probably running on some 5.2 or 5.3 version that will have problems with recent PHP syntax. Also, it's a good practice to add an exit; command after redirecting via header() because the redirection does not mean that the rest of the script is not executed.
I am asking jakem if he can find out what version of php we are running on the legacy cluster.
We are running php 5.3.3. Kohei, can you get the code working with something compatible with 5.3.3?
(In reply to Pascal Chevrel:pascalc from comment #15) > it uses short array syntax which requires an up to date version of PHP (>5.4). I have totally forgot about that ;-) This code is not JavaScript nor Python... Will fix my patch along with other issues.
Attached patch proposed patch 1.2 (obsolete) — — Splinter Review
Thank you for your feedback! Hope it works out.
Attachment #768617 - Attachment is obsolete: true
I think there are several potential problems: - This download page can trigger downloads for all of our products, that's why it is under products/ and not under firefox/, in the past it was also used by thunderbird pages and if you change firefox by thunderbird in the url it still works. Potentially we could redirect requests to thunderbird downloads to the Firefox download page. - For the same reasons I think we should discard requests to ESR, they do work. - We (I, at least :) ) don't know which list of keywords exist for the download urls (latest, stub, beta...). I don't think we should redirect based on a short list of keywords, usually those keywords were created to avoid version problems and serve the latest build for a channel, we shouldn't try to replicate that logic. - generally speaking, I think we should also fix the cases of links to download.html outside of mozilla.org, not just fix the schemes that correspond to internal links. I wouldn't be surprised to know that there are links on the web pointing to download.html and using variables (download sites like softonic, clubix, download.com... that propose a 'free' download from the editor's website, links in support forums...) I patched your patch :) to fix those cases if you are interested (yay for insomnia...)
So the code should be simpler as cmore said in his comment 0. This won't affect Thunderbird nor ESR versions.
Attachment #769898 - Attachment is obsolete: true
Here is what I had come up with in case you want to look :) https://gist.github.com/pascalchevrel/5906395#file-gistfile1-php
Thanks, Kohei and Pascal. So, are we going forward with Kohei or Pascal's patch? I want mkelly to do a code review and I don't want to have him review two patches.
We need to get this resolved ASAP. We are possibly losing ton of users a day to old downloads and that's bad for ADIs and UX. Let's get this wrapped up this week, reviewed and QA'd. Who's patch should mkelly or sancus review?
Mkelly: can you help out with a review of patch in comment 21?
Flags: needinfo?(mkelly)
Whiteboard: [kb=1038602]
Comment on attachment 770100 [details] [diff] [review] proposed patch 2.0 Review of attachment 770100 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me (taken with a grain of salt considering my PHP rustiness :P)
Attachment #770100 - Flags: review+
Flags: needinfo?(mkelly)
Ok, thanks, mkelly! Let's get this merged and on -dev so that Raymond can test this. It should be pretty easy to test since we can define all of the different use-cases that should triger the redirect and the ones that should not. I can put together an etherpad of test cases with Raymond.
Committed to trunk at r117883. It will be testable on -dev soon.
Hmmm, nevermind. All the URLs in comment 29 are now magically working.
Also, I tested this URL and it doesn't redirect as expected: https://www-dev.allizom.org/en-US/products/download.html?product=firefox-stub&os=win&lang=en-US
Koehi: Can you check out the 3 URLs in comment 33 to see why they are not redirecting?
Hmmm, I opened chrome, cleared cached, pasted in the URL https://www-dev.allizom.org/en-US/products/download.html?product=firefox-18.0&os=win&lang=en-US hit enter, and it went to the download page and not to /firefox/new/.
Also, there is a link to Firefox 3 on this page: http://www.sharephim.net/xem/Phim-5778-425230.html (at the very bottom) to: http://www.mozilla.com/en-US/products/download.html?product=firefox-3.0.6&os=win&lang=en-US The original bug said any Firefox versions greater than version 4. Should we worry about pre-4? My main concern was post Firefox 4.
Ok, now all the URLs in comment 33 are redirecting fine. Still an open question about comment 37.
(In reply to Chris More [:cmore] from comment #36) > Hmmm, I opened chrome, cleared cached, pasted in the URL > https://www-dev.allizom.org/en-US/products/download.html?product=firefox-18. > 0&os=win&lang=en-US hit enter, and it went to the download page and not to > /firefox/new/. If the referer header is not set, you won't be redirected to /firefox/ (In reply to Chris More [:cmore] from comment #37) > The original bug said any Firefox versions greater than version 4. Should we > worry about pre-4? My main concern was post Firefox 4. My patch is a simple comparison like this, so it covers pre-4 versions. > intval($matches[1]) < intval(LATEST_FIREFOX_VERSION)
(In reply to Kohei Yoshino [:kohei] from comment #39) > (In reply to Chris More [:cmore] from comment #36) > > Hmmm, I opened chrome, cleared cached, pasted in the URL > > https://www-dev.allizom.org/en-US/products/download.html?product=firefox-18. > > 0&os=win&lang=en-US hit enter, and it went to the download page and not to > > /firefox/new/. > > If the referer header is not set, you won't be redirected to /firefox/ Ok, this explains why when I try to go to direct traffic via the address bar that it won't redirect until I click the link in bugzilla. thanks. > (In reply to Chris More [:cmore] from comment #37) > > The original bug said any Firefox versions greater than version 4. Should we > > worry about pre-4? My main concern was post Firefox 4. > > My patch is a simple comparison like this, so it covers pre-4 versions. > > > intval($matches[1]) < intval(LATEST_FIREFOX_VERSION) Then why doesn't this redirect? http://www.mozilla.com/en-US/products/download.html?product=firefox-3.0.6&os=win&lang=en-US
(In reply to Chris More [:cmore] from comment #40) > Then why doesn't this redirect? > > http://www.mozilla.com/en-US/products/download.html?product=firefox-3.0. > 6&os=win&lang=en-US I just tested it locally and was redirected as expected. 1. On the local environment, change the link of the download button to https://www-dev.allizom.org/en-US/products/download.html?product=firefox-3.0.6&os=win&lang=en-US using Firebug 2. Click the button Here, intval($matches[1]) is 3, $_SERVER['HTTP_REFERER'] is http://local.mozilla.org/en-US/firefox/new/ in my case (it doesn't contain https://www-dev.allizom.org/), so it should work.
It works now. The 301 redirect was cached in Firefox when I sent it direct traffic. :) That has got me all day. ha
:)
Whiteboard: [kb=1038602] → [kb=1038602] r=117883 b=trunk
Blocks: mozorg-redirects
So is it ready to merge?
(In reply to Kohei Yoshino [:kohei] from comment #44) > So is it ready to merge? If the code is good to go, reviewed, let's merge and get this out there.
mkelly: could you merge r117883 to stage/prod? Thanks!
Flags: needinfo?(mkelly)
Merged to stage in r118477 and production in r118478.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: needinfo?(mkelly)
Resolution: --- → FIXED
Whiteboard: [kb=1038602] r=117883 b=trunk → [kb=1038602] r=117883,118477,118478 b=trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: