The module does not correctly transform the addthis urls to https which causes browsers to block the loading of the js on http leading to a javascript error since addthis is not defined which breaks all other site javascript after that point.

In classes/AddThis.php:

  private function getWidgetUrl() {
    $url = ($this->currentlyOnHttps() ?
      $this->getBaseWidgetJsUrl() : // Not https url.
      $this->transformToSecureUrl($this->getBaseWidgetJsUrl()) // Transformed to https url.
    );
    return check_url($url);
  }

The above logic is incorrect and should be !$this->currentlyOnHttps() . However the method transformToSecureUrl() is already checking currentlyOnHttps() so it could be reduced to:

    // Transformed to https url if needed.
    $url = $this->transformToSecureUrl($this->getBaseWidgetJsUrl());
    return check_url($url);

All other frontend addthis urls should be passed through transformToSecureUrl() to ensure the proper handling.

CommentFileSizeAuthor
#34 2285177-addthis-https_links.patch1.05 KBpaulsheldrake
#30 use_schemeless_urls_to_2285177-30.patch14.97 KBBaysaa
#27 use_schemeless_urls_to-2285177-27.patch15.27 KBmatglas86
#21 addthis-use_schemeless_urls-2285177-21-7.patch2.4 KBmatason
#20 addthis-use_schemeless_urls-2285177-20-7.patch2.4 KBmatason
#17 use_schemeless_urls_to-2285177-17.patch1.01 KBrpsu
#15 use_schemeless_urls_to-2285177-15.patch2.26 KBmatglas86
#9 addthis-2285177-schemeless-url-9-cleanup.patch3.66 KBjcisio
#8 addthis-2285177-schemeless-url-8.patch1.01 KBjcisio
#1 addthis-fix-https-2285177-1.patch1.19 KBrecrit
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

recrit’s picture

recrit CreditAttribution: recrit at Phase2 commented
Status: Active » Needs review
FileSize
addthis-fix-https-2285177-1.patch1.19 KB

The attached patch updates getWidgetUrl() per the above recommended change and updates all other url handling

mshepherd’s picture

mshepherd CreditAttribution: mshepherd commented
Status: Needs review » Reviewed & tested by the community

With patch applied, addthis scripts and css get loaded properly on my https site.
Thanks.
Matthew

aether’s picture

aether CreditAttribution: aether commented

Confirming this fixes https issues for me.

mr.andrey’s picture

mr.andrey CreditAttribution: mr.andrey commented

I applied the patch, cleared the cache and theme registry and I'm still getting the following in the rendered code:

<script src="http://s7.addthis.com/js/300/addthis_widget.js#async=1"></script>

I'm using the widget via an AddThis "Basic toolbox" block in the footer.

Any thoughts?

mr.andrey’s picture

mr.andrey CreditAttribution: mr.andrey commented

Turns out $base_url in settings.php and SSL don't play nicely. I unset the $base_url and all works well.

robin.vaughan’s picture

robin.vaughan CreditAttribution: robin.vaughan commented

Hello,

I am using Add this - "7.x-4.0-alpha4" on a website which recently migrated from http to https. After the website URL not have a HTTPS in it the add this widgets works fine on IE, Chrome and Safari but it does not work well in Mozilla. When I click to open the Add this POP up it does not load in the pop up window.

Can any please provide me with any use full information on the same, it would be a great help.

Regards,
Robin Vaughan

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

Wouldn't it just be easier/better to use protocol relative URLs throughout the code instead of transforming between HTTP and HTTPS?

jcisio’s picture

jcisio CreditAttribution: jcisio commented
Title: URLs not transformed to HTTPS when needed » Use schemeless URLs to avoid mixed HTTP/HTTPS problem
Status: Reviewed & tested by the community » Needs review
FileSize
addthis-2285177-schemeless-url-8.patch1.01 KB

Yes, let's so it to simplify the code. I'm posting a simple patch first, then a second patch with removal of unnecessary functions.

jcisio’s picture

jcisio CreditAttribution: jcisio commented
FileSize
addthis-2285177-schemeless-url-9-cleanup.patch3.66 KB

A comprehensive patch. We don't need check_url (which check protocol or check_plain) because we don't output it directly, but with drupal_add_js() which already escape it properly.

So patch #8 is to be reviewed, patch #9 would be a nice to have clean up.

stewart.adam’s picture

stewart.adam CreditAttribution: stewart.adam commented
Status: Needs review » Needs work

#9 results in a fatal error (Call to undefined method AddThis::getBaseWidgetUrl()). I believe there's a typo and getBaseWidgetUrl should read getBaseWidgetJsUrl.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Another workaround before this gets fixed: in the "Advanced Settings" at admin/config/user-interface/addthis/advanced, you can manually change all the URLs to HTTPS instead of HTTP. Seems to work for me.

matglas86’s picture

matglas86 CreditAttribution: matglas86 commented

We do have to keep a check in there. If someone provides its own url in http and we are on htps it will fail to use the url.

I think that patch #1 should work. I did not test it yet.

SocialNicheGuru’s picture

SocialNicheGuru CreditAttribution: SocialNicheGuru commented

the patch no longer applies clieanly

matglas86’s picture

matglas86 CreditAttribution: matglas86 commented

Do you mean its not a bug anymore?

matglas86’s picture

matglas86 CreditAttribution: matglas86 commented
Status: Needs work » Needs review
FileSize
use_schemeless_urls_to-2285177-15.patch2.26 KB

I tested it and created a patch based on the work that already done. But now against the latest dev. I left the schema in there. I think its better. @jcisio and @recrit will receive all the credit for this.

leendertdb’s picture

leendertdb CreditAttribution: leendertdb commented
Status: Needs review » Reviewed & tested by the community

I had the same problem, AddThis not working when website is accessed over https. Applied the patch from #15, applies cleanly and everything seems to work now. No more console errors, and functionality is restored. Thanks everyone for the effort.

Updating status to RTBC.

rpsu’s picture

rpsu
he/him
Primary language Finnish
CreditAttribution: rpsu commented
Version: 7.x-4.x-dev » 7.x-4.0-alpha6
FileSize
use_schemeless_urls_to-2285177-17.patch1.01 KB

This is purely a replica of patch #8 but applies on 7.x-4.0-alpha6. Will re-change the version tag once tests are done.

rpsu’s picture

rpsu
he/him
Primary language Finnish
CreditAttribution: rpsu commented
Version: 7.x-4.0-alpha6 » 7.x-4.x-dev

Apparently automated testing is not enabled, so back to -dev -branch.

matason’s picture

matason CreditAttribution: matason at Code Enigma commented

I tested the patch on #17, it applies cleanly and works as expected, thank you @rpsu

matason’s picture

matason CreditAttribution: matason at Code Enigma commented
FileSize
addthis-use_schemeless_urls-2285177-20-7.patch2.4 KB

Actually, I'm not so sure, I thought it was working but today I get a warning message when I visit /admin/config/user-interface/addthis

AddThis services could not be loaded from @service_url

which is repeated and consequently the Included and Excluded services lists are empty.

I think this is caused by URI's being passed to drupal_http_request() which returns a -1002 code with the message "missing schema"

I found I had to prefix these URI's with the scheme before they were passed to drupal_http_request()

The patch attached has the one from #17 as its starting point, plus the method I added to prefix the scheme.

Has anyone else run into this or are the schemeless URI's working for you?

matason’s picture

matason CreditAttribution: matason at Code Enigma commented
FileSize
addthis-use_schemeless_urls-2285177-21-7.patch2.4 KB

Sorry, missed a closing bracket in that patch, this new one should be right!

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle as a volunteer commented

Confirming that #21 is good. It would be great to see this pushed.

edysmp’s picture

edysmp
Primary language Spanish
Location Nicaragua
CreditAttribution: edysmp at MTech, LLC commented

The patch #21 works.

mErilainen’s picture

mErilainen CreditAttribution: mErilainen at Wunder commented

Seems to work, but the protocol is appended to the url everytime I go the the settings and save them. So I need to make sure to remove the protocol when saving the values at /admin/config/user-interface/addthis/advanced

  • gisle committed 5552bb3 on 7.x-4.x authored by matason 
    Issue #2285177 by matason, jcisio, matglas86, rpsu, recrit: Use...
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle as a volunteer commented

mErilainen wrote in #24:

Seems to work, but the protocol is appended to the url everytime I go the the settings and save them. So I need to make sure to remove the protocol when saving the values at /admin/config/user-interface/addthis/advanced

This has now been fixed: #2713271: Everytime I 'click save configuration' another 'http' is added to each service url.

matglas86’s picture

matglas86 CreditAttribution: matglas86 at .VDMi/ commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
FileSize
use_schemeless_urls_to-2285177-27.patch15.27 KB

I worked on this issue again because this was committed but did not work #2713271: Everytime I 'click save configuration' another 'http' is added to each service url. When looking a this issue the problem seems bigger. The root of the problem is starting at saving the url's in the configuration. We accepted all the input values and did no validation at the root of the problem.

I have changed that behavior to validate the links when saving and only load the saved url's from configuration. When validating the url on save we also handle the scheme less url better. Also when loading the widget url as admin and seeing a mismatch in the used scheme when on https we show a warning message to let the admin know it will give a problem. The message might need to be more helpful. Suggestions are welcome. This will not break the functionality but provide helpful information to correct problems. Also when you save configuration information is not altered unexpectedly.

Please review and test this patch.
I have set this state to because incorrect saving and rendering of the url (scheme) will interfere with the user when giving security warnings. We don't want that.

philipsens’s picture

philipsens CreditAttribution: philipsens commented

Patch #27 applies cleanly. AddThis still works and I can see that the urls get loaded and contain http (which is correct). But I get these errors in the AddThis configuration:

AddThis services could not be loaded from @service_url
AddThis services could not be loaded from @service_url

I'm quite curious why this happens and why this happens twice. I also find it interesting that AddThis stil works.

Edit: If I change the url to https in the config it gets loaded this way.

Baysaa’s picture

Baysaa CreditAttribution: Baysaa commented
Status: Needs review » Needs work

Patch at #27 applies cleanly against latest dev.

There's a small issue:
AddThis bookmark URL should not be run through addthis_url_validate. Or addthis_url_validate should accomodate for one of these URL fields to be only '#' value. In the current iteration, you can't save the Advanced Configuration page, since the "AddThis bookmark URL" field contains # as value and doesn't validate against FILTER_VALIDATE_URL.

Another issue is, I have to force https on all service URLs now as #28 says on the edit. Otherwise, if I leave it schemeless then the list of available services don't load on the Basic settings page, and displays a warning AddThis services could not be loaded from @service_url. Forcing all service URLs (except the Bookmark URL #) to be on https works on both http and https pages (mixed mode).

FYI, just to answer your question @philipsens , the above warning is displayed twice since there's 2 places where you can select it:
- Compact Menu Enabled Services
- Excluded Services

Baysaa’s picture

Baysaa CreditAttribution: Baysaa as a volunteer commented
FileSize
use_schemeless_urls_to_2285177-30.patch14.97 KB

Attached patch is basically patch at #27 but removes the validation from the "AddThis bookmark URL" field. For context, see #29

Baysaa’s picture

Baysaa CreditAttribution: Baysaa as a volunteer commented
Status: Needs work » Needs review
SocialNicheGuru’s picture

SocialNicheGuru CreditAttribution: SocialNicheGuru commented
Status: Needs review » Needs work

I had to add https to the services link for this to work :(

jcisio’s picture

jcisio CreditAttribution: jcisio commented
Title: Use schemeless URLs to avoid mixed HTTP/HTTPS problem » Always use https

I think today we should use https in all cases. Schemeless URL is no longer considered good: https://twitter.com/paul_irish/status/588502455530311680 (oh, that tweet was on the same day I posted my schemeless url patch).

paulsheldrake’s picture

paulsheldrake CreditAttribution: paulsheldrake at Kanopi Studios commented
FileSize
2285177-addthis-https_links.patch1.05 KB

Here is a patch that does HTTPS links and not schemeless.

nehajyoti’s picture

nehajyoti CreditAttribution: nehajyoti as a volunteer and at QED42 commented

Thanks. Patch https://www.drupal.org/files/issues/use_schemeless_urls_to-2285177-15.patch i.e #15 works and patch https://www.drupal.org/files/issues/addthis-fix-https-2285177-1.patch does not work.