When use securepages module - the path for chart images stays with http:// instead of https:// which causes SSL error. How chart images can be prefixed correctly respecting secure connection?

Comments

tjholowaychuk’s picture

why would you ever need https with this? 1. its not your domain, 2. its a auto-generated chart. not EVERYTHING needs to be https, that will just be slow. images for example should not be severed over https (most cases)

boombatower’s picture

Status: Active » Closed (won't fix)

It doesn't seem https:// on the front works with Google Chart API anyway.

boombatower’s picture

Assigned: Unassigned » boombatower
Status: Closed (won't fix) » Postponed (maintainer needs more info)

Actually it seems that they do, but not with the old url currently be used.

define('CHART_URI', 'http://chart.apis.google.com/chart');

Would need to be:

define('CHART_URI', 'http://chart.googleapis.com/chart');

In 7.x branch the url() function is used so it should go through standard alter hooks to change to https if you want...not sure what the best solution is here...although changing the base url seems like a good idea.

boombatower’s picture

Committed that change to 6.x-1.x and 7.x-1.x branches.

grendzy’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Postponed (maintainer needs more info) » Needs review
Issue tags: +Needs backport to D6
StatusFileSize
new14.94 KB
new441 bytes

It's pretty standard for 3rd-party integration modules to detect and match the protocol when embedding external resources. For example, google_analytics does this. Without this support, browsers display scary warnings (some more severe than others):

grendzy’s picture

StatusFileSize
new439 bytes

D6 patch - this is the same condition, but the $is_https global isn't available in D6.

raspberryman’s picture

+1 - I can confirm that these browser security warnings are very, very scary.

ScottW’s picture

It seems to me that the D6 solution is to use the new URL and always specify https (which is what google seems to recommend) - or did I miss something?

--Scott

13rac1’s picture

Status: Needs review » Needs work

Perhaps it is the SSL setup on my development server, but the patch in #5 isn't working. Can someone confirm the patch works for them?

Here is how the securepages module does it the HTTPS check.

/**
 * Check if the current page is SSL
 */
function securepages_is_secure() {
  return (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') ? TRUE : FALSE;
}

http://drupalcode.org/project/securepages.git/blob/refs/heads/7.x-1.x:/s...

grendzy’s picture

eosrei: drupal_settings_initialize uses the same check to set the $is_https global. Are you using a reverse proxy, non-apache, or other unusual config? What is the value of $_SERVER['HTTPS'] on your box? (you can check by visiting /admin/reports/status/php, under the "apache environment" section).
There are some notes at http://www.metaltoad.com/blog/running-drupal-secure-pages-behind-proxy for dealing with these situations, but I'm certain that if ($is_https) is the right test.

13rac1’s picture

Ok. My server runs Varnish, I figured it was the issue. I don't need this functionality right now, so we won't be setting up Varnish for it.

I can't help but wonder: should both URLs be defined, and this check done in chart_url()?

greenreaper’s picture

Here's a better solution:

define('CHART_URI', '//chart.googleapis.com/chart');

Use a protocol-relative URI and let the user's browser decide whether or not it should be accessed securely. Works great with Varnish!

grendzy’s picture

Funny, I just stumbled into this last week. It is a better solution. http://paulirish.com/2010/the-protocol-relative-url/

boombatower’s picture

Seems like a good approach.

boombatower’s picture

Looks like changing it like #12 results in src="///chart..." so line 152 inside chart_url() should probably

function chart_url($chart) {
  if ($data = chart_build($chart)) {
    return url(CHART_URI, array('query' => $data));
  }
  return FALSE;
}
boombatower’s picture

Title: SSL prefix stays http » Use protocol relative Google Chart API URL
Status: Needs work » Needs review
StatusFileSize
new874 bytes
boombatower’s picture

Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6

Automatically closed -- issue fixed for 2 weeks with no activity.