This is a follow-up to #1577: Mapping privilege separation on non-SSL/SSL.

Steps to reproduce:

  • Install HEAD
  • Apply the following patch, which sets the #https key on the comment reply form (obviously users will use hook_form_alter; hacking comment.module is for demonstration purposes)
    --- a/modules/comment/comment.module
    +++ b/modules/comment/comment.module
    @@ -1836,6 +1836,7 @@ function comment_form($form, &$form_state, $comment) {
       // comments on nodes.
       if (empty($comment->cid) && empty($comment->pid)) {
         $form['#action'] = url('comment/reply/' . $comment->nid);
    +    $form['#https'] = TRUE;
       }
       if (isset($form_state['comment_preview'])) {
  • add $conf['https'] = TRUE; to settings.php
  • log as admin via https://example.com/user (SSL)
  • create an article node
  • navigate to http://example.com/node/1 (PLAIN HTTP)
  • Submit the comment form.

Expected result: The comment is saved.

Actual result: "This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists."

For a more thorough demonstration, you can grab the DRUPAL-7--1 branch of securepages and run the tests.

I think it's caused by the dual session cookies, which causes session_id() to vary between requests, which means form tokens don't validate. I assume (though haven't tested) this would also cause anything that relied on drupal_valid_token() (such as /comment/42/approve?token=bczrJ7m...) to fail if redirected to HTTPS.

In D6, the securepages_prevent_hijack module implements similar dual-cookie functionality. However the secondary secure cookie isn't an actual PHP session cookie, it simply a hash from drupal_get_token() that authenticates the primary session. In this way session_name(), session_id(), and drupal_valid_token() work normally.

One last note - I realize an insecure form with an HTTPS action is not secure at all - there's no way to know the form itself hasn't been tampered with. An attacker could rename all the form elements, and substitute them with hidden fields of his own choosing. Or install a javascript keylogger. Or steal the form token and use it for an XSRF. Similarly, redirecting an unencrypted ?token=bczrJ7m link is asking for XSRF.
Nonetheless, we have the #https FAPI key - which appears to have been designed to support this use case.

Files: 
CommentFileSizeAuthor
#285 https-session-cookies-961508_285.patch9.74 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#285 interdiff-281-285.txt4.21 KBmartin107
#283 interdiff-279-281.txt895 bytesmartin107
#281 interdiff-279-281.txt0 bytesmartin107
#281 https-session-cookies-961508_281.patch9.47 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,075 pass(es), 48 fail(s), and 26 exception(s).
[ View ]
#279 https-session-cookies-961508_279.patch9.49 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,121 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#276 interdiff-273-276.txt2.7 KBmartin107
#276 https-session-cookies-961508_276.patch9.5 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,733 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#273 https-session-cookies-961508_273.patch10.26 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,616 pass(es), 38 fail(s), and 24 exception(s).
[ View ]
#266 interdiff-260-266.txt881 bytesmartin107
#266 https-session-cookies-961508_266.patch10.24 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,385 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#260 https-session-cookies-961508_260.patch10.24 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 64,453 pass(es), 8 fail(s), and 8 exception(s).
[ View ]
#252 interdiff-961508_1-251.patch993 bytesmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-961508_1-251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#251 https-session-cookies-961508_251.patch10.18 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch https-session-cookies-961508_251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#241 961508.patch10.18 KBSalah Messaoud
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#232 961508-232.patch1.35 KBraphaelhuefner
PASSED: [[SimpleTest]]: [MySQL] 40,734 pass(es).
[ View ]
#227 interdiff-961508-227.txt5.45 KBdamiankloip
#227 961508-227.patch10.18 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-227.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#211 drupal_https-961508-211-TESTS-ONLY.patch4.32 KBheddn
PASSED: [[SimpleTest]]: [MySQL] 58,768 pass(es).
[ View ]
#211 drupal_https_sessions-961508-211.patch6.07 KBheddn
PASSED: [[SimpleTest]]: [MySQL] 58,862 pass(es).
[ View ]
#165 drupal_https_sessions-961508-165-TESTS-ONLY.patch3.74 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 54,774 pass(es), 17 fail(s), and 4 exception(s).
[ View ]
#165 drupal_https_sessions-961508-165-WRONG-FIX.patch5.26 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 54,720 pass(es), 28 fail(s), and 18 exception(s).
[ View ]
#165 drupal_https_sessions-961508-165.patch5.31 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_https_sessions-961508-165.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#164 drupal_https_sessions-961508-164-TESTS-ONLY.patch3.72 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 54,681 pass(es), 14 fail(s), and 2 exception(s).
[ View ]
#164 drupal_https_sessions-961508-164-WRONG-FIX.patch5.24 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 54,694 pass(es), 23 fail(s), and 17 exception(s).
[ View ]
#164 drupal_https_sessions-961508-164.patch5.29 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 54,666 pass(es).
[ View ]
#159 961508-159.patch1.53 KBmducharme
PASSED: [[SimpleTest]]: [MySQL] 40,367 pass(es).
[ View ]
#153 961508-153.patch1.54 KBmducharme
PASSED: [[SimpleTest]]: [MySQL] 40,338 pass(es).
[ View ]
#141 961508-141.patch1.69 KBjazzdrive3
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#134 drupal_https_sessions-961508-134-TESTS-ONLY.patch3.79 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 50,021 pass(es), 10 fail(s), and 2 exception(s).
[ View ]
#134 drupal_https_sessions-961508-134.patch5.2 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 51,245 pass(es).
[ View ]
#131 drupal_https_sessions-961508-129.patch7.23 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 50,159 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#129 drupal_https_sessions-961508-129.patch7.23 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]
#127 drupal-https_session-961508-127.patch7.23 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https_session-961508-127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#124 drupal-https_session-961508-124.patch7.2 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 50,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#110 drupal-https_session_cookies-961508-110.patch7.76 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https_session_cookies-961508-110.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#106 drupal-https_session_cookies-961508-106.patch7.29 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 48,749 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#103 drupal-https_session_cookies-961508-103.patch7.61 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 48,779 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#101 drupal-https_session_cookies-961508_101.patch6.99 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 19 fail(s), and 17 exception(s).
[ View ]
#98 drupal-https_session_cookies-961508-98.patch6.67 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 48,713 pass(es), 20 fail(s), and 17 exception(s).
[ View ]
#96 drupal-https_session_cookies-961508-96.patch6.67 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 48,703 pass(es), 35 fail(s), and 21 exception(s).
[ View ]
#92 drupal-n961508-92-d8.patch6.58 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 46,888 pass(es), 20 fail(s), and 17 exception(s).
[ View ]
#90 drupal-n961508-90-d8.patch6.55 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php.
[ View ]
#85 drupal-n961508-85-d8.patch8.33 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 41,978 pass(es), 20 fail(s), and 17 exception(s).
[ View ]
#82 drupal-n961508-82-d8.patch6.92 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 41,956 pass(es), 27 fail(s), and 17 exception(s).
[ View ]
#75 961508-75-https-dual-tokens.patch6.92 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 40,723 pass(es), 37 fail(s), and 21 exception(s).
[ View ]
#71 drupal-https-only-961508-23-32.patch6.21 KBkrlucas
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https-only-961508-23-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 961508-23-32-D7.patch1.53 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-23-32-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 961508-23.patch1.57 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 961508-20.patch1.53 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 961508.patch1.5 KBgrendzy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#5 961508_TEST_ONLY.patch4.93 KBgrendzy
FAILED: [[SimpleTest]]: [MySQL] 31,595 pass(es), 15 fail(s), and 4 exception(es).
[ View ]
#3 961508.patch840 bytesgrendzy
PASSED: [[SimpleTest]]: [MySQL] 27,019 pass(es).
[ View ]

Comments

Add a $sid parameter to drupal_get_token, defaulting to session_sid(). Form API passes in ssid if $form['#https'] is set. If the user doesn't have a ssid then just throw an error.

subscribe just for fun (I'm not a fan of $conf['https'] but curious to follow the #action)

Status:Active» Needs review
StatusFileSize
new840 bytes
PASSED: [[SimpleTest]]: [MySQL] 27,019 pass(es).
[ View ]

Here's one simple approach that allows the tokens to be shared. Module authors that *want* tokens to be restricted to one protocol can still do so, they'd just need to check $_SERVER['https'] in the validation logic.
(I feel like using $_COOKIE in this patch is a tad clumsy - it could be refined by tweaking session.inc to give us the sid. )

Regarding comment #1, I'd prefer a solution that's broader in scope. Fixing tokens in FAPI only would leave URL tokens unresolved, potentially forcing us to deal with #755584: Built-in support for csrf tokens in links and menu router in D7.

Module authors that *want* tokens to be restricted to one protocol can still do so, they'd just need to check $_SERVER['https'] in the validation logic.

My reading of the patch is that the token is received from the server via HTTP, then could be passed back to the server along with the insecure session cookie via either HTTP or HTTPS as a valid token.

It depends how the token is used (I need to dig into the fapi when I have time), but theoretically what chx talks about in #1 would be more secure: you could create a token that would only be valid if passed back with the secure session cookie via HTTPS.

Status:Needs review» Needs work
StatusFileSize
new4.93 KB
FAILED: [[SimpleTest]]: [MySQL] 31,595 pass(es), 15 fail(s), and 4 exception(es).
[ View ]

This patch isn't a fix; it only contains tests for the issue.

BTW mfb: you're right, if we follow the approach in my first patch, drupal_validate_form() should require an HTTPS connection when the #https key is set.

to clarify my comment in #4, I was just wondering if there's any chance of token validation alone being used for some sort of privilege escalation (accessing a cached form or something) if the token is passed in via HTTPS by an attacker (with insecure session key but not the secure session key needed for user authentication). I haven't yet had a chance to dig in to how tokens are used (by core or contrib)

Issue tags:+SecurePages

subscribing

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

mfb: I'm pretty sure the risk you ask about isn't an issue, because tokens can never be validated without the correct session_id. Here's a better patch. The tests will go into #1050746: HTTPS sessions not working in all cases

@grendzy: The issue is that this patch allows a token to be validated with the insecure session id (which could be intercepted by an attacker via packet sniffing) on the secure site (which otherwise requires the secure session id for user authentication).

This isn't necessarily a problem if both token validation and user authentication are always required by the code that relies on tokens, since an attacker shouldn't be able to authenticate on the secure site with just the insecure session id. But I did want to point out the risk.

Ah, yes. (Too little sleep at drupalcon). As I see it using mixed-mode in the first place means being willing to accept some compromises and also taking responsibility for configuring your modules wisely. I do share your general skepticism about mixed-mode: with modern CPUs, it's a compromise that's becoming less necessary, and with SSL-stripping one that's less effective. Nonetheless this is a use-case that has been possible in the past and still has a place in the ecosystem.

If we can get this in I'll commit to documenting this compromise at http://drupal.org/https-information and in the securepages documentation.

Version:7.x-dev» 8.x-dev
Category:bug» task
Issue tags:+needs security review, +needs backport to D7

AFAICS, Drupal never supported this so far, so this is a good and possibly backportable improvement, but not a bug.

Category:task» bug

sun, it's a regression bug, because the straightforward use cases (submitting a form with action=https, or clicking a tokenized link) work fine in D5 and D6. There is no known workaround (except possibly forking session.inc, which is pluggable). As it stands both use cases are simply impossible in 7.x.

I'm not really sure what to do next. I've invested probably 30+ hours in this and #1050746 together, but it's really hard to find reviewers. Can you help? I've already asked my colleagues on the security team, on IRC, and friends in the local user group. As always I'm happy to trade reviews or other tasks, but so far everyone has balked when they see the complexity of session.inc.

You might have better luck if you do some of the thinking for the reviewers.

For example, in #1050746 you either have to install Secure Pages and run the tests or code-read the patch just to know what session issues you've fixed. Unknowns are often written off as too much work. By stating what the patch fixes in words people can get an idea of what's involved before they start. It might even encourage people to review just a portion of the issue rather than the whole (as I did).

Just my $0.02. ;)

Tagging issues not yet using summary template.

Subscribing

I think this tag is appropriate.

The last submitted patch, 961508.patch, failed testing.

StatusFileSize
new1.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Needs a reroll?

Status:Needs work» Needs review

StatusFileSize
new1.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled for core/.

In an effort to move these along by providing feedback, here is a quick overview.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Thanks for fixing the shoddy work I did with the HTTP/HTTPS mechanism. However, this needs a test. Thanks again.

Could anyone elaborate on just what one would test? I'm not quite there in my skill yet to simply know how to proceed with that and googling didn't really turn up anything useful beyond SimpleTest tutorials in general.

Issue tags:+session

Adding session tag.

This is still a problem for me too when trying to use the secure_pages module. If I understand right these patches are not for the secure_pages module but for Drupal core?

For what it's worth, #3 (for D7) and #23 (for D8) are almost identical and #23 applies fine on D7.10 as well.

I think a test could fake $GLOBALS['is_http'] and verify that a form with this property submits fine with it set to true, and fails when set to false.

Status:Needs work» Needs review
StatusFileSize
new1.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-23-32-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch from #23 re-rolled against 7.x for drush make file.

Status:Needs review» Needs work
Issue tags:+Needs tests

Still needs a test I think. Thanks!

If someone can suggest where (which file) the tests should go, and give a high level summary of what to test I have some funded time to get this done.

Everett, that's great! Here's a quick outline for a test.
Add a new menu item in form_test.module, with the path /form_test/https_only. This page should render a simple form with $form['#https'] = TRUE. Post a mock HTTPS request to that form, via $this->httpsUrl. Assert a successful form submission. Then, repeat the form submission with $this->httpUrl (a mock non-HTTPS request), and assert the form error is present.

@grendzy

Thanks for the help. I think I can do this.

I created the menu item and form in form_test.module, and I created a new test case in form.test. I notice that httpUrl() and httpsUrl() are not part of the web test case, but are in session.test. What is the preferred way to load the required class?

Also, it looks like what I need to do is get the proper absolute URLs from httpsUrl and httpUrl() and then to do a $this->drupalPost(), is that correct?

Oh, I didn't realize httpsUrl() was in a different class. You might have to move those two functions to DrupalWebTestCase.

Does anyone disagree with @grendzy's recommendation to move httpUrl() and httpsUrl() to DrupalWebTestCase?

I moved httpUrl and httpsUrl to DrupalWebTestCase, that is working.

In my test I do:

$this->drupalPost($this->httpsUrl('form_test/https_only'), array('textfield' => '123'), t('Submit'));

This gets the page, but then does a GET back to index.php, instead of simpletest/tests/https.php, since the form returns without the action set accordingly.

1. I understand why drupalPost() needs to do a GET before it does a POST, but I don't understand why the second request is a GET.

2. What is the best way for me to dynamically set the action for this form to the correct absolute URL?

I'm not sure about 1. For 2, it looks like session.test alters the action this way:

<?php
    $this
->drupalGet('user');
   
$form = $this->xpath('//form[@id="user-login"]');
   
$form[0]['action'] = $this->httpsUrl('user');
?>

Okay, using

    $this->xpath('//form[@id="-form-test-https-only-form"]');

Is giving a PHP unexpected t_variable error. Can anyone see the error? This is used in a test function within a class that extends DrupalWebTestCase.

Perhaps the hyphen at the beginning of -form-test-https-only-form? I'm not sure though. I've found the xpath error messages not the greatest for debugging.

How do I apply this patch? Does not work the command git (git apply -v 'http://drupal.org/node/707484') -- I'm using 7.10 Dupal

chaskype, the URL in your comment ( http://drupal.org/node/707484 ) is a link to the handbook page documenting how to apply patches, not part of the apply command. #21 will apply to Drupal 7, and #23 is for Drupal 8.

Thank you @grendzy - That was my problem then (I was applying the patch to D7 D8)

Thank you...

Something went wrong with the other tags it seems. Re-adding them...

@Everett Zufelt
Did you ever get your test working? Do you have a patch for what you've completed thus far? It seems from a code perspective, no one has any complaints... it's just a test that's holding this up.

Additionally, I'm not sure if anyone gave feedback on #39. Anyone have any complaints with this approach?

@rickmanelius

Time is my current blocker, I'll add this to my todo list for May 4 if I don't find time to get to it before that.

I really appreciate everyone's work on this, but is there any update? I must admit using the combination of the other modules I've cobbled together to redirect is tough to maintain and not nearly as clean as Secure Pages on Drupal 6.x. Wish I knew enough to contribute.

Hey Robin (#54). It seems we're waiting on Everett in #52 because he's already made progress on the testing portion.

Can we get an update on this? May 4th was expected to be worked on 2 months later no progress? Preventing secure pages module full release, probably even preventing people from upgrading to drupal 7.

@Everett Zufelt
Can you please post what you have so far of the test? It would be helpful to use that as a starting point to build from and get this patch committed. It looks like @xjm's patch from #21/23 would essentially be RTBC, but for this test.

Hi,

Thanks for all the time everyone's put into this. I'm a little confused in relation to Drupal 7 having found this thread on the SecurePages project page under fixes for D7.

Does anybody know if the patch has been put into D7 core (e.g. the latest 7.15 release)? Or is even on the agenda for D7 core development?

Version:8.x-dev» 7.15

Can we please get this included in core? D7 is touted as a highly optimized platform for eCommerce with Drupal Commerce but yet the basic tentant of doing such business - SSL - is tharwalted by problems in core.

Priority:Major» Critical

Bumping this to critical. Without SSL switching, how can you build an eCommerce site.

+1 for critical, 7.16 release blocker

Could we see the same priority for this as well?

http://drupal.org/node/471970

Then Secure Pages can be released properly. I have built an e-commerce site around D7, but we can't go live with online sales until these issues are fixed.

Version:7.15» 8.x-dev

The patch needs to go into Drupal 8 first (http://drupal.org/node/767608).

But this issue already has the "needs backport to D7" tag and can certainly be backported after that.

@alexp999 - It's completely valid to "patch" (not hack) core. Just go ahead and apply the patch yourself until this makes it in. Just be careful that you don't loose the patch the next time you update though.

Patching core is the same like hacking core. We have no idea if it may ever get committed. Missing propper SSL support can be safely seen as security issue with the critical severity. Unbelivable that core got named final with such an issue.

Agreed with #66. I've had to reapply at least 2 of the patches with each new Drupal 7.x branch release... and I understand the need to properly place in D8 and backport with tests, etc... but we've had a terrible time getting the momentum to push this through despite the fact that it works, etc.

See my early comments, reviews, etc. I've been running 2 commerce sites with said patches and have also marked this RBTC one at least one occasion. But we're stuck on #52 to provide tests in order to get this fully committed, etc.

If the Drupal community truly takes security seriously, then it's kinda sad that this module STILL doesn't have a full release despite all the help in this thread!

Sorry, didn't mean to rant... you guys all rock. But this patch seems to be in perpetual purgatory...

We'll see if anyone agrees with me but after reading some of the drupal documentation, this is what I'd like to see.

http://drupal.org/node/767608
Did anyone read the Backport policy? This issue seems like a special case:

Sometimes, the ideal way to fix the bug in Drupal 8 may involve far-reaching changes or refactoring that would not be appropriate to backport. In those cases, it is best to create a separate Drupal 8 issue for the refactoring and cross-link it to the original issue. This allows the minimal fix to be committed to Drupal 8 and backported as quickly as possible, while work on the more far-reaching fix for Drupal 8 can continue at its own pace.

I don't think we need a test, not for d7, the far reaching change for d8 would be to write some tests. I think we should mark this issue as a 7.x issue and create a separate Drupal 8 issue for the refactoring and cross-link it to the original issue. We can submit this minimal fix to d8 and it's already backported for d7.

Then we need to get two experienced people to review the patch in #21 on 7.15 then mark it Reviewed and tested by the community.

http://drupal.org/patch/review

Patches usually need a positive review by at least two people other than the original author before being declared Reviewed and tested by the community. In the normal course of development, patches will go through several revisions, often with changes by multiple authors, before being committed to the official code repository. Although this process isn't foolproof, it helps ensure that code has been peer reviewed for performance, readability, and usability (good), as well as regressions and bugs (bad).

No, tests are generally needed for all bug fixes, regardless of version, because that's how we make sure we don't break things ever again. There are once in a great while exceptions to this, but given how critical this functionality is, it seems even more prudent to ensure there are tests for it.

Step 1 is for one of the 78 people following this issue to analyze the work Everett's done and try and get it working. If it's not possible, explain where you got stuck. From there we can figure out if it's worth committing the fix in advance of the test, but hopefully the more likely scenario is just pinging someone with specific knowledge to move it forward once we understand what the blocker is.

I've contact form'd @Everett Zufelt to see if he can provide his work so far.

But I need this patch and need to learn tests so I will take a crack at it.

StatusFileSize
new6.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https-only-961508-23-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a patch based on #32 with a test added as suggested by @grendzy in #35. It also moves the httpUrl and httpsUrl methods to DrupalWebTestCase from SessionTestCase.

This doesn't include tests for the changes to drupal_get_token, just the #https form property.

Should I include a test based on #5 (the TEST ONLY patch)?

Status:Needs work» Needs review

Oh, right, the form submission should implicitly test the drupal_get_token changes.

Changing to needs review.

I'm guessing the patch will fail because the issue version is set to 8. I don't know the protocol for changing versions since previous comments have suggested a d8 patch needs to be committed first. My attempt to write a d8 patch this evening has been thwarted by install wizard issues.

Status:Needs review» Needs work

The last submitted patch, drupal-https-only-961508-23-32.patch, failed testing.

Sorry for disturbing you all, but I'm not sure what to do. I'd like to test this module, but atm it seems that patching is not possible (using drupal 7.14). So what is to do for a beginner with this module or patch atm???
Thanx in advance,

maen

Status:Needs work» Needs review
StatusFileSize
new6.92 KB
FAILED: [[SimpleTest]]: [MySQL] 40,723 pass(es), 37 fail(s), and 21 exception(s).
[ View ]

@maen: you can patch D7 by using the patch in #32 -- usual warnings concerning patching Drupal core apply.

I have re-factored the patch from #71 for D8:

  • There was a syntax error in one of the asserts (t()-function not correctly closed).
  • Moved SessionHttpsTest::httpUrl() and SessionHttpsTest::httpsUrl() to WebTestBase since they are now shared between multiple test cases.
  • Removed an excess space from the form_set_error message.

How can we get this done?

Maybe if someone creates a new issue with the version set to D7, so a D7 patch will actually pass testing?

StatusFileSize
new6.92 KB
FAILED: [[SimpleTest]]: [MySQL] 41,956 pass(es), 27 fail(s), and 17 exception(s).
[ View ]

This patch fixes the paths to the https.php/http.php files in WebTestBase.php, but still has problems submitting the form.

Status:Needs work» Needs review

Lets test it anyway.

Status:Needs review» Needs work

The last submitted patch, drupal-n961508-82-d8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.33 KB
FAILED: [[SimpleTest]]: [MySQL] 41,978 pass(es), 20 fail(s), and 17 exception(s).
[ View ]

After help from chx on IRC I was able to fix the "Form HTTPS only" test, but the others need work.

What do we need to do to get this backported to Drupal 7? This is one of the critical patches that is need for serious commerce, and continue to keep Drupal easy to maintain. Can someone in the know list exactly what needs to happen, so we know where we are? I will try to help where I can.

In order for the patch to be backported to D7, it first has to be completed, reviewed, and committed to D8. See http://drupal.org/node/767608.

The latest D8 patch is in #85 and is currently failing tests. If you want to push this forward, you should check out a copy of Drupal 8 from Git, apply the patch, and attempt to fix the remaining failing tests. #drupal-contribute on Freenode is a good place to ask for help on this, and there are dedicated core mentoring hours if you are new to core development, to get any basic questions answered.

Thanks for offering to help, rather than just asking "Why is this not done yet?" It's really appreciated.

StatusFileSize
new6.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php.
[ View ]

The patch in #85 failed:

quickstart@qs10:/home/dm8$ git apply drupal-n961508-85-d8.patch
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php:46
error: core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php: patch does not apply

So just re-rolling it.

From the test details, I don't know why we are getting "HTTP response expected 200, actual 403" in this unless it isn't being sent via https. This should work:
$this->assertResponse(200);

The errors in Drupal\system\Tests\Session\SessionTest all look like they should be green. I don't understand what the bot is looking for.

Status:Needs review» Needs work

The last submitted patch, drupal-n961508-90-d8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.58 KB
FAILED: [[SimpleTest]]: [MySQL] 46,888 pass(es), 20 fail(s), and 17 exception(s).
[ View ]

Nixed an extra }

Status:Needs review» Needs work

The last submitted patch, drupal-n961508-92-d8.patch, failed testing.

Subscribing, it is hard to know which patch to use.

Assigned:Unassigned» mbrett5062

Taking this over for a little while. Re-rolling against latest head and cleaning some doc blocks.

Status:Needs work» Needs review
StatusFileSize
new6.67 KB
FAILED: [[SimpleTest]]: [MySQL] 48,703 pass(es), 35 fail(s), and 21 exception(s).
[ View ]

And here is new patch for test bot.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session_cookies-961508-96.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.67 KB
FAILED: [[SimpleTest]]: [MySQL] 48,713 pass(es), 20 fail(s), and 17 exception(s).
[ View ]

Missed some forward slashes. Try again.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session_cookies-961508-98.patch, failed testing.

Working on fixing tests. Hope to post new patch tomorrow.

Status:Needs work» Needs review
StatusFileSize
new6.99 KB
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 19 fail(s), and 17 exception(s).
[ View ]

OK think I have fixed some of the tests. But will probably still fail some. Will continue working on this tomorrow.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session_cookies-961508_101.patch, failed testing.

StatusFileSize
new7.61 KB
FAILED: [[SimpleTest]]: [MySQL] 48,779 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Seem to have cleared some more test errors. The last patch changed form.inc line #1153, but this did not fix the errors, just shifted from SessionTest (which we had not modified) to SessionHttpsTest.

if (!empty($form['#https']) && !$GLOBALS['is_https'])

was changed to

if (!empty($form['#https']) && $GLOBALS['is_https'])

So after further investigation. I saw that in session_test.module we were altering the login form, but unconditionaly.

function session_test_form_user_login_form_alter(&$form) {
  $form['#https'] = TRUE;
}

This gave errors in SessionTest which required #http form.

So changed above to the following.

function session_test_form_user_login_form_alter(&$form) {
  global $is_https;
  if ($is_https) {
    $form['#https'] = TRUE;
  }
}

This now cuts down the errors significantly on my local tests.

Lets see what test bot has to say. I hope this is going in the right direction.

If someone with more experience in tests and PHP then myself would like to chime in, please feel free. I am only a novice.

Status:Needs work» Needs review

ooppps and send to bot.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session_cookies-961508-103.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.29 KB
FAILED: [[SimpleTest]]: [MySQL] 48,749 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

OK cleared up an incorrect assertion. Now left with only one fail I believe.

Can not figure out why it's failing.

Need the assistance of someone with more experience. Would also like my work checked, as I am not sure I am making changes to the test's correctly. It seems logical to me but could be wrong.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session_cookies-961508-106.patch, failed testing.

I might be saying this a bit late, but this patch may be wiped out if #335411: Switch to Symfony2-based session handling gets in.

OK, let me get it green anyway. Just one more test. Just in case.

Status:Needs work» Needs review
StatusFileSize
new7.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https_session_cookies-961508-110.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

OK fixed the final test. Not sure if I am correct in my assumption, but I believe the login page should also be non-secure.

At least if this comes back green and #335411: Switch to Symfony2-based session handling does not get in, we are good to go.

Also this work may be of use for backport to D7

Yes, it would be great for D7 (and D8 if Symfony session handling does not get in).

Yes, regardless, this needs a backport to Drupal 7, regardless of what ends up happening with 8.

This patch for D7 has been ready since #21 (granted, it needs the test coverage added). The issue is simply getting it committed to D8 first so that the backport can be accepted, which would be awesome so that secure pages for D7 can finally get to a 1.0 release without requiring these patches.

#71 added test coverage to the D7 patch. It still needs to be cleaned up but it's almost there:

  • syntax error in one of the asserts (t()-function not correctly closed).
  • excess space from the form_set_error message.

#21: 961508-20.patch queued for re-testing.

This should not be blocked for D7 just because we don't know where D8 goes.

This should not be blocked for D7 just because we don't know where D8 goes.

+1

Assigned:mbrett5062» Unassigned

Un-assigning myself, as my work here is done for now.

Status:Needs review» Needs work

I reviewed the patch and read through the entire issue. Comments:

  1. The drupal_get_token() changes look pretty solid to me. My only question is whether this hurts security in the case of a site that is "mostly" HTTPS (meaning, HTTPS-enough that it does not suffer from this bug in practice) but happens to put $conf['https'] = TRUE in its settings.php file anyway? Because then all tokens on the site will start being derived from the insecure session for no particular reason. I don't see this as a blocker because it could be dealt with via documentation (at http://drupal.org/https-information, and in release notes when this gets backported to Drupal 7), but we do really need to understand what the consequences are for various sites on the entire "all-HTTPS-to-no-HTTPS spectrum" so this can be documented.
  2. This part of the patch I don't understand:

    +  // Ensure the correct protocol when #https is set.
    +  if (!empty($form['#https']) && !$GLOBALS['is_https']) {
    +    form_set_error('', t('This form requires HTTPS. Contact the site administrator if the problem persists.'));
    +  }

    First, it's totally changing the meaning of $form['#https'] from "use HTTPS for this form if the server supports it" to "the server must support HTTPS for the form to be submitted". So for example, on my non-HTTPS server, this completely breaks installing modules via the UI (using authorize.php).

    Is the intention to check variable_get('https', FALSE) here too? Then, I think it makes sense.

    However, I'm still not clear on how it relates to the rest of this issue. I read comments #4 and #5 but I'm still not seeing the scenario it protects against. (And in any case, if it's doing something to compensate for a less-secure token, then only doing it in the form API seems problematic since tokens are used outside the form API too.)

    Finally (less important), the message looks outdated... I believe at some point Drupal core moved away from telling people to spam the site administrator :) I think just "This form must be submitted over a secure connection" would work.

  3. As for the tests...
    • Er, aren't the tests themselves missing??? The patch in #85 seems to be the last version that has the new test file :)
    • +    $this->assertText(t('Log in'), 'Login page loaded for first session.');

      I can't figure out why these are being added and what they accomplish?

    •      // Check that user login form action is secure.
           $this->drupalGet('user');
           $form = $this->xpath('//form[@id="user-login-form"]');
      -    $this->assertEqual(substr($form[0]['action'], 0, 6), 'https:', 'Login form action is secure');
      +    $this->assertNotEqual(substr($form[0]['action'], 0, 6), 'https:', 'Login form action is not secure');

      Well, at a minimum the code comment would need to be changed too, but it seems like a problem to me that this expectation is changing. Isn't it a fundamental part of the test?

    Note that some of the above may be fixed if it turns out we can remove the $form['#https'] changes from this patch (since then we could just remove the corresponding test changes also).

  4. For what it's worth, what ever happened to the tests @grendzy wrote way back in #5? I'm not sure they actually got moved anywhere else, and they look pretty relevant to the drupal_get_token() changes in this issue.

Assigned:Unassigned» mbrett5062

Working on fixing the issues from #120. Thanks for the review @David_Rothstein.

StatusFileSize
new7.2 KB
FAILED: [[SimpleTest]]: [MySQL] 50,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

OK have addressed some of the issues raised by @David_Rothstein.

1) Not really sure what is required here, maybe just documentation? I too am not sure of the consequences across the entire "all-HTTPS-to-no-HTTPS spectrum". I feel this needs more discussion.

2) I believe what is trying to be achieved is exactly what you quote "the server must support HTTPS for the form to be submitted".
Surely this would only break something if the form is set as HTTPS only. How would that stop you from installing modules on a non-HTTPS server? Maybe I am not understanding your concern correctly.
I would have thought that a server that supports HTTPS could still have insecure URL's, this is exactly what the issue is trying to resolve. Not sending secure forms submissions to insecure URL's, but allowing the form to be submitted to a secure URL on mixed sites.
I have however added variable_get('https', FALSE as you suggested.
Also I believe, again, this needs more investigation/discussion, as you point out tokens are used elsewhere also.
To be honest this whole concept of mixed sessions is confusing, and INMHO fraught with issues. Unless the site admin is particularly careful, there will be mistakes made. Do we really want to go to this much trouble to support such a use case?

3) I missed the actual HTTPS form test on the patch. That is now added.
I too do not understand why those assertions were added, just seemed like noise. I have deleted from the latest patch.
The last point in 3 I agree with you, it was however the only way I could get the tests failure to green. However, that was wrong. I have reverted that in this patch.
4) The tests from #5 where broken and would never pass. In particular the 'token' tests, they were calling drupal_get_token each time, and then testing to see that each time it sent the same token. However, it could not, as the function contains drupal_get_private_key() which returns a unique key each time called.
The other tests were just repeating the functionality of the new FormHttpsOnlyTest.

So finally, this patch will more then likely not pass, and somewhere there is an issue with sessions, and forms, that I have been chasing around in circles, to no avail. I think this needs a more thorough investigation and I personally do not have what it takes. Someone with more in depth knowledge of sessions and session handling needs to unravel this.

Assigned:mbrett5062» Unassigned
Status:Needs work» Needs review

ooppps here bot.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session-961508-124.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-https_session-961508-127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

OK addressing the token issue, as drupal_private_key is set as a global variable, have changed the drupal_get_token to use the variable instead of calling drupal_get_private_key.

This should help.

Status:Needs review» Needs work

The last submitted patch, drupal-https_session-961508-127.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.23 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

OK lets try that again.

Status:Needs review» Needs work

The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.23 KB
FAILED: [[SimpleTest]]: [MySQL] 50,159 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok missed a ';', lets try that again.

Status:Needs review» Needs work

The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.

I believe what is trying to be achieved is exactly what you quote "the server must support HTTPS for the form to be submitted".
Surely this would only break something if the form is set as HTTPS only. How would that stop you from installing modules on a non-HTTPS server?

When you install modules via the UI (at admin/modules/install) it often redirects you to the FTP credentials form. This form has $form['#https'] set to TRUE and thus the earlier patches broke that form on non-HTTPS servers. However, I confirmed that the addition of variable_get('https', FALSE) in the newer patch fixes this and restores the behavior to normal.

There's a form in the existing core tests which has $form['#https'] set to TRUE as well, and I'm guessing this may have been the cause of some earlier test failures?

***

The remaining test failure looks to me like we need a variable_set('https', TRUE) to trigger the HTTPS behavior... That combined with the above makes me also wonder if we can't just merge this with the existing tests, which already have a code path that does this and already come very close to testing the same thing. I'll look into it...

I don't think I understand the idea of making $drupal_private_key a global variable? drupal_get_private_key() should work OK in tests as it is (at least I'd think).

StatusFileSize
new5.2 KB
PASSED: [[SimpleTest]]: [MySQL] 51,245 pass(es).
[ View ]
new3.79 KB
FAILED: [[SimpleTest]]: [MySQL] 50,021 pass(es), 10 fail(s), and 2 exception(s).
[ View ]

This patch contains the code from #124, with the tests for HTTPs forms merged into the existing tests that were already there, as well as a version of the drupal_get_token() tests from #5.

If all goes well, the first patch will have failures in both sets of new tests, and the second patch won't have any :)

I'm still not sure why the $form['#https'] changes are part of this issue, though (even though I think they make sense on their own). Maybe if @mfb or @grendzy are still following this issue they will have an explanation...

Status:Needs work» Needs review

David, thanks very much for your help here. I have been pulling my hair out over this.

I see that your improved 'token' tests now only call 'drupal_get_token' once and compare it on HTTPS and HTTP submissions, so yes, it will return the same token, that was exactly the problem with previous version of the test. Calling it twice will never return the same token. The reason I made the change to not calling the function, but retrieving the already stored token was to get over that shortcoming, however your solution seems better IMO.

I think the reason for my issue with the form tests, is that it seemed they were trying for a fail of HTTPS form over HTTP. Exactly what you say we do not want. So yes the new test's pass with variable_get('https', FALSE), but we need it confirmed that this is the expected behavior previously sought.

Anyway, thank you very much for your support here. I am a novice and have been learning a lot as I worked through this.

Status:Needs review» Needs work

sounds like #136 Identified some things that need to be addressed, so marking needs work.

Status:Needs work» Needs review

It sounds more like needs review.

David_Rothstein:

First, it's totally changing the meaning of $form['#https'] from "use HTTPS for this form if the server supports it" to "the server must support HTTPS for the form to be submitted". So for example, on my non-HTTPS server, this completely breaks installing modules via the UI (using authorize.php).

Is the intention to check variable_get('https', FALSE) here too? Then, I think it makes sense.

Looking at form.inc, I see that the #https key is indeed only considered when $conf['https'] is enabled. So yes I agree the form_set_error() should only fire when in this mode (the latest patch does include this check, so I think we're good).
As to why we set ''This form requires HTTPS" in the first place... it was based on feedback from mfb early in the issue. At the moment I'm having trouble stating a specific threat model that form_set_error() protects against. It's more of a sanity check: something has gone wrong if 1. $form['#https'] is set, 2. $conf['https'] is set, and 3. we're not actually on an SSL page.

I admit I was surprised by the bad effect on authorize.php. I'm not sure, there may be a misunderstanding about (the horribly named) variable $conf['https'] itself. It does not mean merely "my server supports HTTPS" (though I suppose that's implied). It actually means "Enable mixed-mode session handling".

My only question is whether this hurts security in the case of a site that is "mostly" HTTPS (meaning, HTTPS-enough that it does not suffer from this bug in practice) but happens to put $conf['https'] = TRUE in its settings.php file anyway?

Enabling mixed-mode radically changes the behavior of sessions, and if someone "just happens" to enable this out of ignorance, I'm not sure the code can protect them (though documentation could help).

StatusFileSize
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I have re-rolled the required patch against Drupal 7.19 for those who need it, while we are waiting to see what happens.

Thanks.

The last submitted patch, 961508-141.patch, failed testing.

Status:Needs work» Needs review

This issue is 29 months old. Is there anything that can be done to get a fix committed for 7.x? Any tests I could run?

This fix is required for Secure Pages, which is security-critical for many sites. So, core has to be re-patched with every update. It's completely unclear which patch should be applied to the 7.x (currently 7.22), as the minitest results are showing some patches red, some green....

Direct $_COOKIE usage seems rather hack-ish. drupal_get_token() is just meant to generate a URL-safe token that's used for validation. Manipulating the session ID outside of session_id() seems wrong. Should this logic be in that function instead?

Rather than patching core on every release, I think the better approach is to redirect all traffic to https. That's the approach I've taken on all D7 sites that require SSL.

#147: a mature CMS like Drupal 7+ probably needs to offer a solid mixed-mode handling if it is to retain its leading position now that security is on the rise "everywhere".

RobLoach: I agree that accessing $_COOKIE here isn't ideal, however I believe it's the only solution that doesn't involve a non-backwards-compatible API change in D7.

DanZ: The patches deep-linked from the securepages project page are the ones I recommend.

Ditto to what @grendzy said about which patches to use. I haven't run into a case where they need to be re-rolled for applying; the normal patch fuzz factors take into account the line number changes.

Tests seem ok, but this seems more like an issue of approach.
Will this ever get released to core so we can move secure pages to 1.0? Looks like a logjam - another solutions perhaps?

I agree with @DanZ. Acquia's Insight tool recommends that everyone use the Secure Pages module. This really should have been fixed a while back.

StatusFileSize
new1.54 KB
PASSED: [[SimpleTest]]: [MySQL] 40,338 pass(es).
[ View ]

Here's a rewrite of 961508-20.patch for 7.22

Status:Needs review» Needs work

The last submitted patch, 961508-153.patch, failed testing.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review

Thanks, mducharme. Let's set it to 7.x-dev long enough to run the auto-test on #153. Set it back to 8.x when done.

#153: 961508-153.patch queued for re-testing.

There is trailing white space and all tests are missing.

Version:7.x-dev» 8.x-dev

...that's great! Now back to 8.x ;)

Version:8.x-dev» 7.x-dev
StatusFileSize
new1.53 KB
PASSED: [[SimpleTest]]: [MySQL] 40,367 pass(es).
[ View ]

Fixed whitespace issue. @hass: there are no tests in the original patch this is updating either. Feel free to add tests and re-up.

Version:7.x-dev» 8.x-dev

NOW back to 8.x

Assigned:Unassigned» David_Rothstein

The most recent patch will break non-HTTPS sites, as discussed above. We need to build off the patch in #134 (which also has tests).

I'll try to work on this (hopefully in the next day or so) and see if we can bring it home. It looks like there's only a little feedback left to address?

This is definitely an important issue to fix. However:

Acquia's Insight tool recommends that everyone use the Secure Pages module.

I do not know if the actual recommendation is more nuanced than that, but if it's not, it's incorrect. Secure Pages (if configured carefully and correctly) is more secure than a site running HTTP only, that is true. However, the most secure way to run a site is to force all requests to be over HTTPS, which does not require either the Secure Pages module or the patch in this issue...

Eh, it also occurs to me that if the patch in #159 can break non-HTTPS sites but still passes tests, we're probably missing some important test coverage... I'll see if I can add something for that too.

This is OT here, but recommending Secure Pages and running a SSL cert is a valid recommendation. This is something that every site should run. Login forms or pages with personal data without SSL are not acceptable in production. d.o should also enable this, too!

Assigned:David_Rothstein» Unassigned
StatusFileSize
new5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,666 pass(es).
[ View ]
new5.24 KB
FAILED: [[SimpleTest]]: [MySQL] 54,694 pass(es), 23 fail(s), and 17 exception(s).
[ View ]
new3.72 KB
FAILED: [[SimpleTest]]: [MySQL] 54,681 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

Here's a new patch. I believe this covers everything in the last few months of comments (as well as other fixes required by recent changes in Drupal 8). Some responses:

  1. David, thanks very much for your help here. I have been pulling my hair out over this.

    I see that your improved 'token' tests now only call 'drupal_get_token' once and compare it on HTTPS and HTTP submissions, so yes, it will return the same token, that was exactly the problem with previous version of the test. Calling it twice will never return the same token.

    You're welcome! Interesting, I would think call it twice should return the same token (unless the user is logged out and not maintaining a session across requests), but in any case, it seems to work now.

  2. As to why we set ''This form requires HTTPS" in the first place... it was based on feedback from mfb early in the issue. At the moment I'm having trouble stating a specific threat model that form_set_error() protects against. It's more of a sanity check: something has gone wrong if 1. $form['#https'] is set, 2. $conf['https'] is set, and 3. we're not actually on an SSL page.

    Yeah, that makes sense to me; I just don't see how it's related to the rest of this issue. However, if no one else does either I guess that gives some confidence we aren't missing anything important, and fixing two bugs at once is fine by me :)

  3. Direct $_COOKIE usage seems rather hack-ish. drupal_get_token() is just meant to generate a URL-safe token that's used for validation.

    We probably should put the code that reads from $_COOKIE in a helper function instead (there is similar code throughout session.inc that could be consolidated) but since it's duplicated in a couple places already, even before this patch, I think we can leave consolidating it to a followup issue (in the interest of not making this patch bigger and harder to review).

  4. Manipulating the session ID outside of session_id() seems wrong. Should this logic be in that function instead?

    We're not manipulating the session ID itself; the idea is that we need to build the token off of something that isn't the session ID. However, the fact that previous patches used $session_id as the variable name definitely made that unclear... That's fixed in the attached patch.

  5. Eh, it also occurs to me that if the patch in #159 can break non-HTTPS sites but still passes tests, we're probably missing some important test coverage... I'll see if I can add something for that too.

    This actually seems like it might be covered already (at least in Drupal 8). I'm uploading a version that contains the incorrect fix in form.inc to test that it already will fail without having to add any more tests than what we already added.

StatusFileSize
new5.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_https_sessions-961508-165.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.26 KB
FAILED: [[SimpleTest]]: [MySQL] 54,720 pass(es), 28 fail(s), and 18 exception(s).
[ View ]
new3.74 KB
FAILED: [[SimpleTest]]: [MySQL] 54,774 pass(es), 17 fail(s), and 4 exception(s).
[ View ]

Those test failures/passees look good to me. I did notice one small issue with the patch:

+    // Test that tokens can be shared in mixed mode.
+    $this->drupalGet('session-test/drupal-token');

This should probably use $this->drupalGet($this->httpUrl('session-test/drupal-token')) instead to be consistent with other code in the tests. (I think the difference matters only if the tests themselves are run over an HTTPS connection.)

Fixed in the attached.

So this patch grants the last unencrypted session token access to all the data of the last encrypted session? So totally Firesheepable. I suppose that backwards compatibility needs to be respected... but the securepages model of "some pages are secure, some are not" is utterly wrong-headed. Sessions, not pages, should be encrypted.

The way I would describe it is, with this patch, if mixed-mode sessions are enabled, an eavesdropper can grab a token from a user's HTTP request, and then cause the user's browser to send it to the HTTPS site via cross-site request forgery as an authenticated and token-validated request. Without the patch, the tokens used on the HTTPS site are always different from those used on the HTTP site, so an eavesdropped token cannot be successfully submitted to the HTTPS site. The patch doesn't change anything for HTTPS sites not using mixed-mode sessions.

I'd consider the patch to be problematic from a security perspective, but then again, I don't use mixed-mode sessions at all (for reasons of Firesheep, hostile networks, user privacy, etc.), so I don't really understand what the threat model and security model are supposed to be in mixed-mode land :)

@David_Rothstein What do you think about #167?

I don't personally think #167 is an issue. This is just what site admins must accept with mixed mode environments. Perhaps there can be a warning in the settings for the value to enable mixed sessions (if there isn't already).

The main idea with Secure pages is generally to protect the transmission of protected data. i.e. if a customer is submitting card or login details to the server, or if the server is sending address or order details back to the user.

It doesn't really matter in my opinion if a hacker can intercept a HTTP cookie and use it to access the HTTPS site, that isn't really the problem. What users don't want is to be entering private details without seeing the little padlock, secure pages allows us as admins to enable https on pages which might show or ask for visitors private details, thus preventing the interception of said info.

It doesn't really matter in my opinion if a hacker can intercept a HTTP cookie and use it to access the HTTPS site, that isn't really the problem.

@alexp999 which is to say, you as a site admin, don't mind that once a user has entered their username and password with the little padlock, that someone sharing their coffeeshop wifi can then cruise into their account and change their password and update their profile?

I should say that what we do with Drupal 7 (without this patch) is that we allow anonymous users to tool around most of the site with HTTP. But once they try to do anything interesting--log in, add something to their cart, submit a form--they are redirected to the HTTPS site (using .htaccess usually). And we never link them back to HTTP after that. Or if we do (because a content creator maybe entered an absolute link (and there is a module for that)) they lose their session.

The main feature behind mixed mode is that you may be able to hijack a non-ssl cookie, but you cannot use this session data to get access to SSL secured sections of your site. Normally you don't transfer personal data on http, but when you switch to SSL (on a newsletter subscription page as example) you get a new session and you will enter your data there. Once you leave the subscription page you will be switched back to the non-ssl session and your SSL session is still secure.

This is all important for proxies, tablets, mobile phones and internet café. Someone may be able to jump back in browser history or multistep shopping cart's and gather your personal data in the used forms if you have not invalidated your session. This is all about privacy. It's also recommended to expire the SSL session on browser window close as I know. The SSL session should also calculated based on IP address to prevent session hijacks with other IPs. It's not allowed to grant access to this shopping cart with the non-ssl session id.

The main reason I use HTTPS on sites is to secure the submission of data to stop packet sniffing of user credentials.

I don't profess to know as much as a lot of your guys about Drupal and cookies/https etc, my point was simply that as an admin, the above is what I use secure pages for.

If we can do what hass in #172 has suggested even better. I have been following this thread for some time now and while I agree we don't want to introduce a security hole, I was just putting out there what I as a site admin use it for, so we could perhaps find a good compromise, without trying to do the impossible. :)

@alexp999 sorry I didn't mean to call you out. I'm also following this issue as a past user of securepages, someone who thought it was necessary but have started using a workaround (see my previous comment) that actually seems more secure with no obvious downside.

That said, I'm definitely entering a world of FUD so I don't want to hijack this thread with untested paranoia.

As @hass says it's all about privacy. A shopping cart block typically follows a user all around a site and may reveal all sorts of personal stuff about that user. It's not up to the site admin what should be "really" private. Securepages, with its emphasis on URLs (pages), ignores that.

So the data sent through these calls will not be compromised by hijacking a session?

Is there testing needed? If yes, what should be tested?

The only known 100% secure way is encrypting *all* requests. All other variants may theoretically lead to troubles and hijacking. This is the reason why Google, Facebook and some others enabled SSL by default. So if you have an SSL accelerator in front of your box, it could be seen as a recommendation to enable SSL on all pages, not only a few pages like a mixed mode environment. If you have low budget servers you may not have the horse power to run all URLs via SSL encryption. SSL slow down page loading... all OT here.

I think the only 100% secure way of doing it while still keeping some requests plain HTTP is to use something like the Secure Login module for Drupal. Then Anonymous users browse over HTTP, but all authenticated users browse via HTTPS.

That is obviously not what this issue was started for though, Drupal has the basis for mixed HTTP/HTTPS sessions although it is not working fully.

This was the point I was trying to make earlier, do we just need to forget trying to make it 100% secure, as it is just not going to be possible? Then have an explanation about the risks of dual sessions in the setting file?

Not quite sure why my post came up with all those tags, I didn't set or change any..... :S

I guess I crossposted earlier...

So basically we could agree that there may be use cases where you want to use mixed mode just to provide a minimal level of security to protect the data being transferred as an alternative to not use SSL at all. There is no reason for Drupal not to support this, right? This does not prevent anyone to enforce SSL globally.

When using Commerce, however, this is still an issue, even if you don't want to go back to a non-secure HTTPS session.

Here is our main use case:

- Anonymous user visits site, begins adding things to cart.
- Anonymous want to begin checkout.
- Checkout sends them to HTTPS, which then loses their cart.
- OR, anonymous user goes to login screen, which sends them to HTTPS, and so they lose their cart.

We never send them back to HTTP once they go to HTTPS. But there must be a way to go to HTTPS for any type of normal, expected Commerce operation.

This patch fixes this issue for us, and is very important.

Just to chime in, my use case is the exact same as @jazzdrive3 in #181 above. Anonymous users never hit https (for performance sake) but of course as a Commerce site we require it for handling sensitive customer data.

I have the same issue as in #181 as well.

But again my question:

So basically we could agree that there may be use cases where you want to use mixed mode just to provide a minimal level of security to protect the data being transferred as an alternative to not use SSL at all. There is no reason for Drupal not to support this, right? This does not prevent anyone to enforce SSL globally.

The use case of data from the anonymous insecure session being migrated to the secure session actually should already work at present. So I'm not sure why it doesn't work w/ Commerce.

There is logic in _drupal_session_read() which, on the HTTPS site, if no secure session is found, checks the sessions table for an anonymous session with the non-HTTPS-only cookie. Any session data found there (e.g. shopping cart) is read into the session - and will be included in the new secure session which is saved.

If you put this in your settings.php and visit the HTTP site followed by the HTTPS site, you should see that the secure session ends up with both the data from the insecure session and the secure session (you have to look in the sessions db table to see the session data):

<?php
if (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') {
 
$_SESSION['secure'] = 'foo';
}
else {
 
$_SESSION['insecure'] = 'bar';
}
?>

To add to the chorus- Our use case is the same as @jazzdrive3 in #181 and @torgosPizza in #182- and we agree with #183- tens of thousands of nodes that can (and should) be browsed without SSL, then when it comes to putting money down or viewing their personal profile, secure the session from then on.

This may be a web commerce specific need, but if one looks at eBay, Amazon, etc, all sessions are mixed mode- even authenticated users are only SSL when paying for something or viewing their account profiles, then switches back to non-SSL (while maintaining the session) when browsing and "shopping". This switch is done quickly and seamlessly.

Granted, they have more resources than many Drupal site admins, but the concept is there and working on the largest commerce sites in the world. Perhaps this is a counter point to the social networks that make all authenticated session SSL.

If I were king for a day, I would want D7 / D8 to support "either, both or neither" and then support each use case with a best practices guide and allow admins a choice and the tools to do it right.

Will continue to do the hacks / fixes for now.

Cheers!

krlucas: I think your analysis is overlooking the fact that mixed mode is intended to be combined with forced HTTPS redirects to "important" pages (where important is defined by the site builder). An attacker can hijack the insecure half of the mixed session, but not the secure half.

repeating my comment from #11:

As I see it using mixed-mode in the first place means being willing to accept some compromises and also taking responsibility for configuring your modules wisely. I do share your general skepticism about mixed-mode: with modern CPUs, it's a compromise that's becoming less necessary, and with SSL-stripping one that's less effective. Nonetheless this is a use-case that has been possible in the past and still has a place in the ecosystem.

We are using civicrm 4.33 which shares drupal 7.22 sessions.
Issue: Login with drupal HTTP works for civicrm HTTP over port 80. Civicrm switches to HTTPS and authentication breaks.
Is there a proposed fix/patch for 7.22?

This issue could use an updated issue summary, based on the templates at http://drupal.org/issue-summaries.

#23: 961508-23.patch queued for re-testing.

cleaning up tags

According to the Secure Pages Project Page this is still an issue in Drupal 7... How do we flag this issue for D7 development?

This issue has been a problem for a long time with D7 and it is preventing useful modules like Secure Pages from producing a stable release.

EDIT: Did not intend to add the 'stable release blocker' tag... It somehow was automatically added back when I posted this reply...

@rbrownell - The 'needs backport to D7' tag means that as soon as this is fixed in 8.x, this issue will go back to 7.x for a backport (to fix the issue in D7 as well).

So the best way to get this into D7 is to help with fixing it in D8 first :)

Thanks for the reply Cottser

According to the history of this, this was originally a problem with D7 that seemed to become overlooked when it was upgraded to be worked on in Drupal 8.

It seems that Drupal 7's development for contributed modules is becoming fragmented because of the frequency of which Drupal releases come out and the amount of work it takes to upgrade modules and sites... It almost reminds me a bit of the problem Google is experiencing with the Android ecosystem. Don't get me wrong though. I love Drupal and everything it stands for... It would just be nice to have more time between releases so that developers create their modules as opposed to falling into the "I'll work on it for the next version only" mindset because frankly Drupal 6 still looks better than Drupal 7 for contributed modules and harmony in the ecosystem.

Drupal 7 is 'advertised' as being eCommerce ready and the best version of drupal thusfar but issues that prevent one of the most popular security modules from releasing a full stable version makes it exactly the opposite.

Anyway... Enough of my rant... How do we fix this?

There appears to be fully functional patches for D7 that aren't being implemented into core because of the 8.x-dev version selection for this issue. How can we get these patches in D7's core? Or is it because D7 and D8 are very similar in terms of coding? Is the testing of such patches for D7 failing because of the issue being marked as D8?

Because of the backport policy. We fix bugs in Drupal 8 first so that we don't have any regressions.

@Cottser. Thank you for pointing that out. Not sure I agree with the policy 100% but things make more sense now.

EDIT: Reading this there is an exception for Security patches... Though this is not a 'patched worked on in private' one wouldn't this still fall into that category because it is dealing with https?

@rbrownell no that only applies to reported security vulnerabilities which are worked on in a separate issue tracker by a limited set of people.

You asked how to fix it - there's a patch in #165 that needs review.

If I understand #177 correctly, does this mean I could adapt my workflows to use the Secure Login module and just drop support for securepages? Skip this entire thread and the others like it? If so, thanks @alexp999 for the recommendation, I'm going to test this and see if it will work for us.

I have 5 years of Drupal experience without patching core in production, I don't want to start now.

Serious sad face that this issue is not closed yet.

Re: #197 although I also generally support the backport policy, I definitely buy in to the argument that this is a security issue. If people don't apply their patches correctly... trouble.

+1 for #197 Don't let the perfect get in the way of the good ...

@Ryan Weal, this is what we have done, just stopped using secure pages and use secure login instead.

Much more secure, means all Anon users are http, all authenticated users over https.

No worry about security between sessions as if a user tries to visit a non http page then they just appear logged out. As soon as they try and do anything meaningful they are redirected to https and find themselves logged in again.

It may not work for everyone's situation but it is ideal for our e-commerce site where we only require users to login when they actually want to complete checkout.

@alexp999 does not work for us. We have webforms and registration pages that need SSL for anon.

Note securepages has over 100,000 downloads and 21,000 active users - most on D7. It's one of the most popular modules. I reiterate #197 that requiring this patch because of the backport policy is not very pragmatic. If you look at the number of https / http issues in google there are many issues that secure pages could solve if properly supported.

Installed Secure Login... what a dream!

@sonicthoughts have you tried using Secure Login? It is actually more of a "form encryption" thing than a "login" module. I think it would work in your case. You install the module, then by default it redirects any page with the login form (and some other forms) to the https version of the same page. You can add additional form IDs to the configuration page, so it will then target all of your other forms.

The only case where I could see Secure Login not working is if you have anon users adding to the cart... HOWEVER, you can simply put the "add to cart" form name in the list. Most add-to-cart forms I've seen append an ID to the end, so there may need to be a patch if the module doesn't support wildcards (I haven't tried that yet - my workflows are all directed to logged-in users). Maybe it already works?

Edit: that would already work per #184, thanks @mfb!

As I mentioned in #184, anonymous users adding items to their cart in HTTP and then switching to HTTPS actually should work. That's because _drupal_session_read() has logic to look for an insecure session if no secure session is found. It's possible that this might be broken somehow, but it's intended to work...

Yes, but we ALSO want many admin pages which do not necessarily have a form to go to SSL. I do see it as a compromise and appreciate the suggestion (may go that route temporarily) but I just think it is yet another band-aid.

Do a search in google or drupal.org on these related topics and there are endless issues that could be solved if securepages were released and not require a dev version and patches. I'm just saying ...

@sonicthoughts I think you should give it a try. By virtue of the login form being SSL it means that all your admin pages are too because that's the session you're using. Good luck. I consider myself done with this issue. :)

The last submitted patch, drupal_https_sessions-961508-165.patch, failed testing.

I second that. This problem is a security issues - black-eye on D7 - a backport from D8 shouldn't be required. I know we, as a community, are trying to push D8 development but policies like this harms Drupal as a whole - they don't help! D7 is (by Commerce Guys work) been pushed as the premier eCommerce solution and it works really well but without the ability properly handle SSL - it's absolutely useless. I am very shocked that over a year later this has not been fixed in D7.

Can we have an exception to policy and get this fixed NOW in D7 that is being used on production websites. This is harming people that are trying to conduct business (eCommerce) with the most usable version of Drupal which is Drupal 7. Please, let us use a little common sense here. Support the thousands of websites our there conducting business with Drupal 7 then port to D8.

Status:Needs work» Needs review
StatusFileSize
new6.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,862 pass(es).
[ View ]
new4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,768 pass(es).
[ View ]

So we can get this back-ported, here's a re-roll of #165. Tests ran fine locally. Let's see how we fair with the testbot. No interdiff since this is simply a re-roll.

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
@@ -149,13 +149,23 @@ protected function testHttpsSession() {
+    $this->assertText(t('This form must be submitted over a secure connection.'), 'Form submission failed over HTTP.');

Hmm, tests didn't fail. Probably need to throw in $this->settingsSet('mixed_mode_sessions', TRUE); somewhere prior to this call. I'll see if I have time later today.

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
@@ -211,6 +221,20 @@ protected function testHttpsSession() {
+    $this->assertEqual($token_plain, $token_secure, 'Tokens are shared in mixed HTTPS sessions.');

I'll have to dig into why this didn't fail. It isn't obvious.

@Ryan Weal

Thanks for the suggestion about Secure Login.

StatusFileSize
new2.64 KB
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 59,466 pass(es).
[ View ]
new4.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,651 pass(es).
[ View ]

Here's another attempt. Cleaned up the tests a little and altered the logic inside CsrfTokenGenerator->get()

Status:Needs review» Postponed

Status:Postponed» Needs review
StatusFileSize
new6.54 KB
FAILED: [[SimpleTest]]: [MySQL] 59,840 pass(es), 7 fail(s), and 6 exception(s).
[ View ]
new5.37 KB
FAILED: [[SimpleTest]]: [MySQL] 59,826 pass(es), 8 fail(s), and 6 exception(s).
[ View ]

#2122761: SessionHttpsTest only runs half the tests seemed to fair well. While not committed yet, I'd like to see how tests run here.

The last submitted patch, drupal_https_sessions-961508-215.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

219 comments later ... and still not working for D7 :(

Issue summary:View changes
Issue tags:+Needs tests, +Needs issue summary update, +session, +SecurePages, +needs security review, +needs backport to D7, +Contributed project blocker
Parent issue:» #1577: Mapping privilege separation on non-SSL/SSL

...adding back tags removed in #217

Would love if we had a better D7 solution in Core, particularly with yesterday's security update.

Agreed. What's holding this up right now is D8's SimpleTests were all messed up. I haven't had time to go through and fix all the broken tests. But once we can fix those, this should be able to slip into D8 nicely and the backport for D7 is fairly simple.

Version:8.x-dev» 7.24
StatusFileSize
new1.25 KB

This is a reroll of the patch in #159 which applies on Drupal 7.24.

Version:7.24» 8.x-dev

Setting Issue back to 8.x-dev

Hi - sorry but will #224 be committed or does it have to wait for 8.x to be marked fixed? There are a lot of dependencies on this single issue.... thanks for the hard work.

Status:Needs work» Needs review
StatusFileSize
new10.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 961508-227.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.45 KB

Here is a reroll, but also we need to make some changes. The CSRF token generator doesn't set the request anymore. We were having issues that the generator was being used outside the request scope, so this may or may not work :/ We could get away with this easier before as get() did not need the request, then we could use the current_user service. We will see...

Left an @todo in the CsrfTokenGeneratorTest, as we need to add more assertions to check any new logic.

The last submitted patch, 227: 961508-227.patch, failed testing.

Hi @nerdcore - why are you setting back to 8.dev if this is might be contributing to major security issues can we create another issue for D7? There is understandable reluctance to apply patches to production systems (especially one's that are so pervasive as this.) Furthermore patches often get lost in upgrades ... the bottom line is that over 25,000 sites are using https://drupal.org/project/securepages and this may affect many more D7 sites. I understand the backport policy that @Cottser referenced, but for security issues this could clearly be seen as an exception.

This is not a security issue. If you are aware of a security issue, you should report it privately to the security team (https://drupal.org/node/101494) and in that case it would be fixed in Drupal 7 first.

In fact, this issue may be the opposite since #167 raised the concern that the current approach in the patch could actually weaken security. I think that has been adequately addressed by the subsequent comments (basically, the suggestion is that (a) in the case of a form on an HTTP page submitting to an HTTPS URL, it's as secure as it can be, and (b) it won't hurt security in other cases?) but I'm not sure.

Not sure the patch in #224 is totally correct because in common.inc it should probably use $session_id and not session_id() when calling drupal_hmac_base64 on the return line:

+  // For mixed HTTP(S) sessions, use a constant identifier so that tokens can be shared between protocols.
+  if (variable_get('https', FALSE) && $GLOBALS['is_https'] && isset($_COOKIE[substr(session_name(), 1)])) {
+    $session_id = $_COOKIE[substr(session_name(), 1)];
+  }
+  else {
+    $session_id = session_id();
+  }
   return drupal_hmac_base64($value, session_id() . drupal_get_private_key() . drupal_get_hash_salt());

StatusFileSize
new1.35 KB
PASSED: [[SimpleTest]]: [MySQL] 40,734 pass(es).
[ View ]

This is a reroll of the patch in #159 which applies on Drupal 7.25.

I agree with #231 that the patch in #224 was missing a line. Why set $session_id when you won't use it after?

Status:Needs review» Needs work

The last submitted patch, 232: 961508-232.patch, failed testing.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review

Moving issue to 7.x so patch will get tested.

232: 961508-232.patch queued for re-testing.

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

#232 passes. Moving issue back to 8.x

Issue tags:+Needs reroll

Tagging for reroll.

Assigned:Unassigned» Salah Messaoud
Issue tags:+TUNIS_2014_JANUARY, +#SprintWeekend2014

Started work

227: 961508-227.patch queued for re-testing.

The last submitted patch, 227: 961508-227.patch, failed testing.

StatusFileSize
new10.18 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I tried patch to reroll patch from #227 it doenst apply for HEAD, I applied it to the commit e75ac46431444c5feacbd744ce38ef350049b468 it worked but after trying to merge with 8.x I didnt get any conflicts, the patch attached apply cleanly

Status:Needs work» Needs review

Assigned:Salah Messaoud» Unassigned

Nice work Salah! And if we get [#4196144] tackled as well, we can finally get a stable release of secure pages! https://drupal.org/project/securepages

Status:Needs review» Needs work

The last submitted patch, 241: 961508.patch, failed testing.

+++ b/core/core.services.yml
@@ -427,9 +427,10 @@ services:
+      - [setRequest, ['@?request']

I think this line needs another ], based on the following error seen after applying the patch and running `drush cr`:

Fatal error: Uncaught exception 'Symfony\Component\Yaml\Exception\ParseException' with message 'Malformed inline YAML string [setRequest, ['@?request'] at l
ine 434 (near "- [setRequest, ['@?request']")' in /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php:312
Stack trace:
#0 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php(59): Symfony\Component\Yaml\Inline::parseSequence('[setRequest, ['..
.', 26)
#1 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(409): Symfony\Component\Yaml\Inline::parse('[setRequest, ['...', fal
se, false)
#2 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(114): Symfony\Component\Yaml\Parser->parseValue('[setRequest, ['...'
, false, false)
#3 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(186): Symfony\Component\Yaml\Parser->parse('- [setCurrentUs...', fal
se, false)
#4 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfo in /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php on line
312
Fatal error: Call to a member function get() on a non-object in /Users/Scott/Sites/d8.dev/core/lib/Drupal.php on line 530

…and searching through core.services.yml to see how other setRequest items were formatted.

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

I got this error here when applying that patch /core/install.php?langcode=en&profile=standard

Symfony\Component\Yaml\Exception\ParseException: Malformed inline YAML string [setRequest, ['@?request'] at line 446 (near "- [setRequest, ['@?request']") in Symfony\Component\Yaml\Inline::parseSequence() (line 312 of core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php).

Issue tags:-Needs reroll+Novice

Adding Novice tag to make the small update mentioned in #246 and provide an interdiff.

Status:Needs work» Needs review
StatusFileSize
new10.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch https-session-cookies-961508_251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've been annoyed by this issue for too long to wait.

$ grep -ir setRequest core/core.services.yml

Now the setRequests are almost consistent:

core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]

Not sure if the 2nd one should include the "?" but it's at least fixed in this issue.

StatusFileSize
new993 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-961508_1-251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

forgot the interdiff.

Status:Needs review» Needs work

The last submitted patch, 252: interdiff-961508_1-251.patch, failed testing.

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.24 KB
FAILED: [[SimpleTest]]: [MySQL] 64,453 pass(es), 8 fail(s), and 8 exception(s).
[ View ]

This is just a re-roll. The patch broke couldn't apply for some reason.

Status:Needs review» Needs work

The last submitted patch, 260: https-session-cookies-961508_260.patch, failed testing.

Which is the preferred patch for Drupal 7.26 for the proper operation of securepages?

@webservant316 — The patch from #232.

Wow - surprising that Drupal 7 is not functioning smoothly with an HTTPS module for mixed mode sessions. I've been fighting with this for a day! How can others even be using this on a commerce site? I see that the Drupal site itself appears to always be on HTTPS. Maybe that is the solution, do not use mixed sessions?

I followed the directions at https://drupal.org/project/securepages. Since I have Drupal 7.26 installed I could not use #13, https://drupal.org/comment/4196144#comment-4196144, so I used #100 instead, https://drupal.org/comment/8241501#comment-8241501. I also could not use #71, https://drupal.org/comment/6391460#comment-6391460, so I used #232, https://drupal.org/comment/8361131#comment-8361131. After setting $conf['https'] = TRUE; in settings.php is was not better off, but continued to permanently lose my session on HTTPS pages. Then I tried https://drupal.org/project/securelogin but with further problems.

Current I have a configuration with securepages that only loses the session when on HTTPS pages, but regains it when on HTTP pages. Fortunately, I can limp along with this for now.

Any idea when this will get straightened out?

ok this got secure pages working for me - https://drupal.org/comment/8524769#comment-8524769

StatusFileSize
new10.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,385 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
new881 bytes

Identified a single problem :-

Removed confusion between drupalPost() and drupalPostForm()

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 266: https-session-cookies-961508_266.patch, failed testing.

Ok .. I can't quite see what is wrong, so I will provide notes and ask for help

Looking at SessionHttpsTests ... just to abstract what is going on I will repeat the comments detailing the steps taken above the two failing tests

205 //Clear Browser cookie jar
..
208 // Start an anonymous session on secure site
..
211 // Mock login to secure site using secure session cookie.
..
217 // Test that the user is also authenticated on the insecure site.
..

221 // Test that tokens can be shared in mixed mode. .
Looking at the verbose from browser testing ( /admin/config/development/testing/results/2 )
222 drupalGet() returns "Valid HTML found on "http://dev.drupal.co.uk/core/modules/system/tests/http.php/session-test/..."
its contains the standard Access denied | Drupal html page.

I think this test get access denied because the of the brand new entry in sytem_test.routing.yml

session_test.shared_token:
  path: '/session-test/shared-token'
  defaults:
    _title: 'Test that tokens can be shared in mixed mode'
    _controller: '\Drupal\session_test\Controller\SessionTestController::sharedToken'
  requirements:
    _permission: 'access content'

_permission: 'access content' I don't think that has been properly mocked.

Please tell me I am wrong ... or steer me in the direction of another place in testing that mocks 'access content' correctly.

The last submitted patch, 266: https-session-cookies-961508_266.patch, failed testing.

I'd prefer when CsrfTokenGenerator would get decoupled from session_id() completely. Instead I suggest to add a method (e.g. getTokenSeed) to AccountInterface. The token seed would be generated the first time it is used (just like the private key) and then stored in the session. Note that when using a generated token seed, it wouldn't be necessary to concatenate that with the private key anymore, because the risk of disclosing the session id via tokens wouldn't exist any longer.

Optionally it would be possible to only generate and store the token seed when actually using mixed mode SSL and otherwise just continue to use the original method.

StatusFileSize
new10.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,616 pass(es), 38 fail(s), and 24 exception(s).
[ View ]

patch no longer applied - reroll

(No conflicts during rebase just auto-merging)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 273: https-session-cookies-961508_273.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,733 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
new2.7 KB

call to access 'mixed_mode_sessions' has change to be

Settings::get('mixed_mode_sessions', FALSE)

I have stripped out the now redundant $settings property from CsrfTokenGenerator.

Lets see what testbot has to say..

Status:Needs review» Needs work

The last submitted patch, 276: https-session-cookies-961508_276.patch, failed testing.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -829,6 +829,11 @@ public function validateForm($form_id, &$form, &$form_state) {
+      \Drupal::formBuilder()->setErrorByName('', $form_state, t('This form must be submitted over a secure connection.'));

Aren't you adding this to the FormBuilder object? Why not create a new instance of this object? Or use $this?

Status:Needs work» Needs review
StatusFileSize
new9.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,121 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Patch no longer applied .. no conflicts in reroll just 3-way merging

I will address #278 ...next

Status:Needs review» Needs work

The last submitted patch, 279: https-session-cookies-961508_279.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,075 pass(es), 48 fail(s), and 26 exception(s).
[ View ]
new0 bytes

addressed #278. I can see other example of \Drupal::request() in this file ... in comments -- but the comments don't make that much sense to me..
That is they don't connect to the code below it

So I have left them alone for now...

  1. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
    @@ -53,6 +62,16 @@ public function setCurrentUser(AccountInterface $current_user = NULL) {
    +   * Sets the current request.
    +   *
    +   * @param \Symfony\Component\HttpFoundation\Request $request
    +   *   The current request.
    +   */
    +  public function setRequest(Request $request) {
    +    $this->request = $request;
    +  }
    +

    Instead of injection the request like that we should better use the request stack already.

  2. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
    @@ -72,7 +91,22 @@ public function setCurrentUser(AccountInterface $current_user = NULL) {
    +    if ($this->request->isSecure() && Settings::get('mixed_mode_sessions', FALSE)) {

    Can't we also use the settings service instead of this singleton call? this makes it potentially easier to test in the future.

StatusFileSize
new895 bytes

reposting interdiff

Status:Needs review» Needs work

The last submitted patch, 281: https-session-cookies-961508_281.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.21 KB
new9.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Addressed issues from #282.

Status:Needs review» Needs work

The last submitted patch, 285: https-session-cookies-961508_285.patch, failed testing.