Closed Bug 840705 Opened 11 years ago Closed 10 years ago

Use HTTPS instead of HTTP for Yahoo integration (Yahoo Mail and Yahoo RSS)

Categories

(Firefox :: File Handling, defect)

13 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 --- verified

People

(Reporter: briansmith, Assigned: raymondlee)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #840699 +++
+++ This bug was initially created as a clone of Bug #771788 +++

browser.contentHandlers.types.1.uri;http://add.my.yahoo.com/rss?url=%s
gecko.handlerService.schemes.mailto.0.uriTemplate;http://compose.mail.yahoo.com/?To=%s

These should use https:// links. However, neither of those domains uses a valid certificate when I change the "http://" to an "https://", so we need new URLs. Maybe Tanvi can help track down who we'd need to contact.
Tanvi: could you help us with that please?
Flags: needinfo?(tanvi)
Is this for when a user clicks on a mailto: link and their default mail provider is yahoo?
Flags: needinfo?(tanvi)
I've sent an email to a friend at Yahoo to see if they can take a look at the bug.
My friend got back to me and said the https version should work now.  Looks like this the code that needs to be updated to add the "s":

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser-region/region.properties

http://mxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/chrome/region.properties

The change could also probably use mochitests if test cases don't already exist.
(In reply to Tanvi Vyas [:tanvi] from comment #4)

https://compose.mail.yahoo.com/?To=raymond@appcoast.com works fine.

However, https://add.my.yahoo.com/rss?url=http://feeds.bbci.co.uk/news/rss.xml displays an invalid security certificate.
(In reply to Raymond Lee [:raymondlee] from comment #5)
> (In reply to Tanvi Vyas [:tanvi] from comment #4)
> 
> https://compose.mail.yahoo.com/?To=raymond@appcoast.com works fine.

Yes, I verified that. Raymond, do you want to create the patch for this part?

> However,
> https://add.my.yahoo.com/rss?url=http://feeds.bbci.co.uk/news/rss.xml
> displays an invalid security certificate.

Yes, I verified this too. We should move this to another bug, so we don't delay the improvement for compose.mail.yahoo.com.
Assignee: nobody → raymond
Attachment #750284 - Flags: review?(bsmith)
Comment on attachment 750284 [details] [diff] [review]
Part 1: Update mail links to https

Review of attachment 750284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, except for that trailing space, but I am not a Firefox reviewer, so sr?gavin.

::: browser/locales/en-US/chrome/browser-region/region.properties
@@ +27,5 @@
>  gecko.handlerService.schemes.webcal.0.uriTemplate=http://30boxes.com/external/widget?refer=ff&url=%s
>  
>  # The default set of protocol handlers for mailto:
>  gecko.handlerService.schemes.mailto.0.name=Yahoo! Mail
> +gecko.handlerService.schemes.mailto.0.uriTemplate=https://compose.mail.yahoo.com/?To=%s 

Remove the trailing whitespace.
Attachment #750284 - Flags: superreview?(gavin.sharp)
Attachment #750284 - Flags: review?(bsmith)
Attachment #750284 - Flags: review+
(In reply to Raymond Lee [:raymondlee] from comment #5)
> However,
> https://add.my.yahoo.com/rss?url=http://feeds.bbci.co.uk/news/rss.xml
> displays an invalid security certificate.

I will ask about this.
Attachment #750284 - Attachment is obsolete: true
Attachment #750284 - Flags: superreview?(gavin.sharp)
Attachment #750357 - Flags: review?(gavin.sharp)
Attachment #750357 - Flags: review?(gavin.sharp) → review+
Attachment #750357 - Attachment is obsolete: true
Attachment #754349 - Flags: checkin?
Attachment #754349 - Flags: checkin? → checkin+
I think we should close this and file a new bug for updating or removing the Yahoo RSS integration.
M(In reply to Tanvi Vyas [:tanvi] from comment #9)
> (In reply to Raymond Lee [:raymondlee] from comment #5)
> > However,
> > https://add.my.yahoo.com/rss?url=http://feeds.bbci.co.uk/news/rss.xml
> > displays an invalid security certificate.
> 
> I will ask about this.

They said they just requested a new cert.  Not sure how long it will take to get one and deploy it.
https://hg.mozilla.org/mozilla-central/rev/d0855b50f559
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Reopening since this only solves 1 of the 2 issues.  We could also file a separate bug for the second issue, as bsmith suggested, but for now I just want to make sure we don't lose track of this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Is there interest to get this ported to localizations?
(In reply to Axel Hecht [:Pike] from comment #17)
> Is there interest to get this ported to localizations?

Yes. I'm not exactly sure what you mean, but the overall goal is to change every http:// link embedded in Firefox to https://, in general.
How do you think this is going to happen then? Seriously, we're a localized product, if you're touching localized files and expect the changes to migrate, you can't just sit there silently.

http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=compose.mail.yahoo.com shows the size of the problem.
The snark isn't really necessary. I missed the localization impact, my bad.

Raymond, can you file a bug to patch all of the localizations as well?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> The snark isn't really necessary. I missed the localization impact, my bad.
> 
> Raymond, can you file a bug to patch all of the localizations as well?

Raymond, have you already filed this bug? If not, will you please CC me on it? Thanks
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> The snark isn't really necessary. I missed the localization impact, my bad.
> 
> Raymond, can you file a bug to patch all of the localizations as well?

Raymond, have you already filed this bug? If not, will you please CC me on it? Thanks
(In reply to jbeatty from comment #22)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #20)
> > The snark isn't really necessary. I missed the localization impact, my bad.
> > 
> > Raymond, can you file a bug to patch all of the localizations as well?
> 
> Raymond, have you already filed this bug? If not, will you please CC me on
> it? Thanks

I have missed this comment.  Just filed bug 881319.  Thanks!
As the bug is still open, I'm putting it here. I tested this and it does not seems to be working so good.

With a new profile, it works as intended, except that Yahoo seem to have missed to install a necessary intermediate cert, so I get a "sec_error_unknown_issuer" Visiting https://login.yahoo.com beforehand will pick up and store it though.

With an old profile, the new https URL isn't used at all, presumably because the gecko.handlerService.defaultHandlersVersion pref value is not incremented as the comment in region.properties state should be done when changing the URL.

Incrementing the number did made Firefox pick up the new Yahoo URL, but it also kept the old, so I now have two Yahoo Mail handlers listed in Preferences -> Content -> mailto.
Sorry, I meant Preferences/Options -> Applications -> mailto.

Also note that while the new Yahoo https handler is added when incrementing gecko.handlerService.defaultHandlersVersion, Firefox will continue to use the old http URL until the user manually changes the mailto handler to the new Yahoo https handler.
(In reply to Hasse from comment #24)
> As the bug is still open, I'm putting it here. I tested this and it does not
> seems to be working so good.
> 
> With a new profile, it works as intended, except that Yahoo seem to have
> missed to install a necessary intermediate cert, so I get a
> "sec_error_unknown_issuer" Visiting https://login.yahoo.com beforehand will
> pick up and store it though.
> 
> With an old profile, the new https URL isn't used at all, presumably because
> the gecko.handlerService.defaultHandlersVersion pref value is not
> incremented as the comment in region.properties state should be done when
> changing the URL.
> 
> Incrementing the number did made Firefox pick up the new Yahoo URL, but it
> also kept the old, so I now have two Yahoo Mail handlers listed in
> Preferences -> Content -> mailto.

I missed to update the gecko.handlerService.defaultHandlersVersion pref value last time. I believe we should update that for both region.properties files.

> Sorry, I meant Preferences/Options -> Applications -> mailto.
> 
> Also note that while the new Yahoo https handler is added when incrementing
> gecko.handlerService.defaultHandlersVersion, Firefox will continue to use
> the old http URL until the user manually changes the mailto handler to the
> new Yahoo https handler.

It looks that we only add new entry but removing old one.
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsHandlerService.js#216(In reply to Hasse from comment #25)

Gavin: what's your suggestion here?
Flags: needinfo?(gavin.sharp)
Blocks: 881319
The patch for Bug 840699 would update the gecko.handlerService.defaultHandlersVersion and the new Yahoo https handler would be added.
Depends on: 904350
Raymond, is there anything we can do to finish this off soon?
Flags: needinfo?(raymond)
The adding rss url is working with valid cert.

Note: need to file another bug to update other locale files.
Attachment #8358950 - Flags: review?(brian)
Flags: needinfo?(raymond)
Comment on attachment 8358950 [details] [diff] [review]
Part 2. Patch for check-in for Yahoo RSS

Review of attachment 8358950 [details] [diff] [review]:
-----------------------------------------------------------------

It seems right to me, but I really know nothing about how stuff in browser/ works or is supposed to work, so I'm not an appropriate reviewer. Please ask a Firefox peer.
Attachment #8358950 - Flags: review?(brian) → feedback+
Comment on attachment 8358950 [details] [diff] [review]
Part 2. Patch for check-in for Yahoo RSS

Stumbled across this bug, seems like a no-brainer.
Attachment #8358950 - Flags: review?(MattN+bmo)
Comment on attachment 8358950 [details] [diff] [review]
Part 2. Patch for check-in for Yahoo RSS

Review of attachment 8358950 [details] [diff] [review]:
-----------------------------------------------------------------

The URL works for me with a few different feeds so this seems fine.
Attachment #8358950 - Flags: review?(MattN+bmo) → review+
Raymond, is the needinfo for Gavin needed still?
Flags: needinfo?(raymond)
Keywords: checkin-needed
Target Milestone: Firefox 24 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/c00e3be62b88

Thanks for the patch, Raymond! One request, can you please make sure that future patches include commit information when requesting checkin? Makes life much easier for those landing on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c00e3be62b88
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
The needinfo was to verify that this patch actually has an effect on existing users without extra steps - I think it doesn't. We might want a separate bug to track that.
Flags: needinfo?(raymond)
Flags: needinfo?(gavin.sharp)
Keywords: verifyme
Verified on Windows 7, Ubuntu 13.10 32bit and Mac OSX 10.9 using Firefox 31.0b9 and gecko.handlerService.schemes.mailto.0.uriTemplate : https://compose.mail.yahoo.com/?To=%s works fine. 

Was a bug filled for update other locale files (comment #29)?
Flags: needinfo?(gavin.sharp)
(In reply to Camelia Badau, QA [:cbadau] from comment #37)
> Was a bug filled for update other locale files (comment #29)?
Bug 994248
Flags: needinfo?(gavin.sharp)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.