The securepages module has a hook_form_alter() hook to encrypt the login form, because it could appear on any page and not just a secure page.

However, it only seems to work for the user_login_block login form. On our site we built our own custom login form whose form_id is redirected to user_login_block via hook_forms(). In this case, I can get our form to submit to HTTPS the first time by simply setting $form['#https'] = TRUE. However, if the form is invalid (wrong username/password), the securepages module will reset it back to HTTP on the second submission.

function securepages_form_alter(&$form, &$form_state, $form_id) {
    // ..
    // When already in secure mode and submitting a login form to an arbitrary page, this will take it out of secure mode
    elseif ($page_match === 0 && securepages_is_secure() && variable_get('securepages_switch', FALSE)) {
      $url['https'] = FALSE;
      $url['absolute'] = TRUE;
      $form['#action'] = url($url['path'], $url);
    }
  }

  // If the user/login block matches, also secure the login block.
  if (securepages_match('user/login') && $form_id == 'user_login_block' && !securepages_is_secure()) {
    $form['#https'] = TRUE;
  }
}

It seems that an easy way to fix this would be to simply check for $form['#https'] and not take the form out of secure mode in this case. So, line 85 becomes:

    elseif (empty($form['#https']) && $page_match === 0 && securepages_is_secure() && variable_get('securepages_switch', FALSE)) {

On the other hand, submitting an unsecure form on a secure page throws a browser warning. Wouldn't it be better to always submit forms on secure pages in secure mode, and then redirect to insecure mode, if necessary, when the form is redirected after submission?

CommentFileSizeAuthor
#4 securepages_1226702_secure_user_login_block_and_keep_form_action_secured_7.x-1.0-beta1.patch1.21 KBchemical
#2 securepages_1226702_secure_user_login_block_and_keep_form_action_secured.patch1.07 KBchemical
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grendzy’s picture

grendzy CreditAttribution: grendzy commented
Assigned: Unassigned » grendzy
chemical’s picture

chemical CreditAttribution: chemical commented
FileSize
securepages_1226702_secure_user_login_block_and_keep_form_action_secured.patch1.07 KB

The action for the user_login_block should always be through https if the user/login page requires https. If on unencrypted page make user-login form use encryption. When page is already encrypt no further action is needed:

<?php
  if (securepages_match('user/login') && $form_id == 'user_login_block') {
    if (!$is_https) {
      $form['#https'] = TRUE;
    }
  }
?>

If we know it is the user_login_block that is being examined and possibly modified, we make sure that no additional modifications happens in securepages_form_alter() by adding a return statement. This means that checking if the form is user_login_block must be moved to the start of the function body.

Forms on secured pages shouldn't have actions changed to go to unencrypted pages cause this would make browsers warn the user that there is a security issue. Redirection to an unencrypted page must be done elsewhere if the 'securepages_switch' is set to true.

Attached patch which implements the changes described above will make securepages_form_alter() look like this:

<?php
function securepages_form_alter(&$form, &$form_state, $form_id) {
  global $is_https, $user;

  if (!variable_get('securepages_enable', 0)) {
    return;
  }

  // If the user/login block matches, secure the login block.
  if (securepages_match('user/login') && $form_id == 'user_login_block') {
    if (!$is_https) {
      $form['#https'] = TRUE;
    }
    return;
  }

  if (isset($form['#action']) && securepages_can_alter_url($form['#action'])) {
    // Remove the base_path, and extract the path component.
    $url = substr($form['#action'], strlen(base_path()));
    $url = drupal_parse_url($url);
    $path = drupal_get_normal_path($url['path']);

    $page_match = securepages_match($path);
    $role_match = securepages_roles($user);
    if ($role_match) {
      if (!$is_https) {
        $form['#https'] = TRUE;
      }
      return;
    }

    if ($page_match && !$is_https) {
      $form['#https'] = TRUE;
    }
  }
}
?>
chemical’s picture

chemical CreditAttribution: chemical commented
Status: Active » Needs review
chemical’s picture

chemical CreditAttribution: chemical commented
FileSize
securepages_1226702_secure_user_login_block_and_keep_form_action_secured_7.x-1.0-beta1.patch1.21 KB

Updated patch due to release of version 7.x-1.0-beta1.

minorOffense’s picture

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

It's been used for a while now on our site. Seems to work.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

My comments FWIW: I read through this issue's explanation and it all looks valid.

w.r.t. the deleted code block:
IMHO, disabling secure pages should just be that: do not take action. Do not redirect to HTTPS. The deleted code block explicitly does something (redirects to HTTP) if the functionality is 'disabled'.
If there is a use case for this explicit behavior, it should be commented and a 3rd option added to the securepages_enable variable. IMHO.

Since I don't see this use case: +1 for RTBC.