Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always load recaptcha JS over HTTPS #13601

Merged
merged 1 commit into from
Feb 17, 2019
Merged

Always load recaptcha JS over HTTPS #13601

merged 1 commit into from
Feb 17, 2019

Conversation

alexymik
Copy link
Contributor

@alexymik alexymik commented Feb 14, 2019

Overview

Recaptcha api.js should always be loaded over HTTPS regardless if the Civi site is being served over insecure HTTP.

Before

Currently CiviCRM checks to see if the site is being served over SSL/HTTPS and attempts to load the recaptcha API over the same protocol:

if (CRM_Utils_System::isSSL()) {
  $useSSL = TRUE;
}

After

Force-set this option so the resulting include tag always loads api.js over https:// as a best-practice.

Technical Details

See also packages/recaptcha/recaptchalib.php:112

Comments

Related: https://lab.civicrm.org/dev/core/issues/425

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot civibot bot added the master label Feb 14, 2019
@civibot
Copy link

civibot bot commented Feb 14, 2019

(Standard links)

@seamuslee001
Copy link
Contributor

Jenkins ok to test

@eileenmcnaughton
Copy link
Contributor

@xurizaemon @totten thoughts ? I know @xurizaemon wrote a lot about this ... I just haven't read it :-) so I'm more looking for @xurizaemon to advise if this is consistent with his proposal & @totten to say whether the proposal makes sense / is safe

@seamuslee001
Copy link
Contributor

Given this is about loading an external service then i think it makes sense to go for the HTTPS version. This would ensure that there is no insecure or mixed mode messages or anything like that. I am a +1 on this

@eileenmcnaughton
Copy link
Contributor

merging based on @seamuslee001 @mlutfy - test fail unrelated

@eileenmcnaughton eileenmcnaughton merged commit 422e737 into civicrm:master Feb 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants