| Project: | Secure Pages |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | grendzy |
| Status: | active |
Issue Summary
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.
<?php
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:
<?php
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?
Comments
#1
#2
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:
<?phpif (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;
}
}
}
?>