In DrupalWebTestCase, getAbsoluteUrl eventually calls url(). The url() function uses the $base_url set by the simpletest runner, not the internal web browser. One result of this is it's impossible to POST to an https URL.

CommentFileSizeAuthor
#159 471970-159.patch7.75 KBravi.shankar
#148 drupal-n471970-148.patch7.84 KBDamienMcKenna
#140 drupal7-fix-simpletest-https-471970-140-7.x.patch10.13 KBkaidjohnson
#139 drupal7-fix-simpletest-https-471970-139-7.x.patch10.13 KBkaidjohnson
#136 471970-135.patch7.76 KBmpdonadio
#126 drupal7-471970-126-fix-simpletest-https-d7.patch10.17 KBmpdonadio
#116 drupal8.simpletest-module.471970-116.patch7.35 KBmpdonadio
#100 drupal7-471970-100-fix-simpletest-https-d7.patch10.17 KBmikeytown2
#99 drupal8.simpletest-module.471970-99.patch8.02 KBdlu
#89 interdiff.txt866 bytesdcam
#89 471970-89.patch8.36 KBdcam
#87 471970-87.patch3.27 KBalarcombe
#82 471970-82.patch8.39 KBattila.fekete
#80 interdiff-24-76.txt16.05 KBmgifford
#77 core--7.x-471970-77-base-url-for-test.patch8.38 KBalberto56
#76 471970-76.patch8.34 KBdcam
#73 471970-73.patch8.33 KBstar-szr
#67 471970_1.patch8.43 KBEclipseGc
#60 471970.patch4.92 KBkrishworks
#49 471970-49.patch8.45 KBsbandyopadhyay
#28 471970-28.patch8.41 KBmstef
#24 471970-24-D7.patch8.41 KBDavid_Rothstein
#24 471970-24-TESTS-ONLY.patch5.1 KBDavid_Rothstein
#24 471970-24.patch8.45 KBDavid_Rothstein
#22 471970.patch2.73 KBmfb
#19 471970-19.patch3.31 KBxjm
#13 471970.patch3.27 KBgrendzy
#11 471970-absolute-url.patch3.57 KBmfb
#9 471970-absolute-url.patch1.61 KBmfb
#8 471970-absolute-url.patch3.55 KBmfb
#5 471970.patch620 bytesgrendzy
#2 471970_getAbsoluteUrl.patch.txt1.8 KBgrendzy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower’s picture

Status: Active » Closed (duplicate)
grendzy’s picture

Thanks for the update! This is what I had been working on, but I like the new browser object a lot better.

grendzy’s picture

Status: Closed (duplicate) » Needs review

This is still a real bug in 6.x and 7.x, and the the browser was taken out of core in 7: #600032: Back out browser.inc

grendzy’s picture

Status: Needs review » Needs work

patch in #2 no longer applies.

grendzy’s picture

Status: Needs work » Needs review
FileSize
620 bytes

new patch

Status: Needs review » Needs work

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

mfb’s picture

Subscribe. This is a big problem for tests that span across multiple schemes or domains.

mfb’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

Let's see if this passes tests..

mfb’s picture

FileSize
1.61 KB

Let's see if this one passes too. It's a simpler patch which assumes $path is a drupal path if it doesn't start with a slash (rather than handling it as a relative URL).

Status: Needs review » Needs work

The last submitted patch, 471970-absolute-url.patch, failed testing.

mfb’s picture

Status: Needs work » Needs review
FileSize
3.57 KB

The upgrade path tests fail because update.php isn't a Drupal path. So back to the earlier working logic..

grendzy’s picture

Issue tags: +SecurePages

I'll try to review this soon.

grendzy’s picture

FileSize
3.27 KB

Works for me. Needed a re-roll:

BenK’s picture

Subscribing

greggles’s picture

tagging.

grendzy’s picture

#13: 471970.patch queued for re-testing.

xjm’s picture

Status: Needs review » Needs work

Note that this patch needs to be rerolled for the new directory structure.

xjm’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Or rather to be fixed in 8.x period to prevent a regression.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

Reroll for D8 attached.

rickmanelius’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

Title: DrupalWebTestCase->getAbsoluteUrl should use internal browser's URL » DrupalWebTestCase->getAbsoluteUrl should use internal browser's base URL
Status: Reviewed & tested by the community » Needs work

drupalPost does $action = isset($form['action']) ? $this->getAbsoluteUrl((string) $form['action']) : $this->getUrl();

why can't drupalPostAjax call similarly getAbsoluteUrl on the path passed in, why the caller needs to care about url() ?

Also, the regex used is not anchored. Is that what you wanted? I... don't think so. I am thinking you wanted an ^ in the beginning but I am unsure.

mfb’s picture

Status: Needs review » Needs work
FileSize
2.73 KB

@chx We do end up calling getAbsoluteUrl() on the path later on. The tricky thing here is that getAbsoluteUrl() is intended to run on URLs that appear in HTML, not Drupal paths. If we want to allow the caller to pass in Drupal paths then we need to modify drupalPostAJAX() to run url() on the path for us. See attached patch for that modification, let's see if it passes tests.

I'm pretty sure the regex should work the same whether or not it's anchored with ^ at the beginning. please clarify if I'm missing something.

mfb’s picture

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
8.45 KB
5.1 KB
8.41 KB
  1. The tricky thing here is that getAbsoluteUrl() is intended to run on URLs that appear in HTML, not Drupal paths.

    The current code seems to be intended to work with both, though? If so, we shouldn't change that in this issue.

  2. +      // Return an absolute URL based on the internal browser's current URL if
    +      // it is not using the internal: scheme.
    +      $parts = parse_url($this->getUrl());
    +      if ($parts['scheme'] != 'internal') 
    

    Although the behavior where drupalSetContent() magically sets an 'internal' scheme wasn't added by this patch, I don't think it makes much sense, so if it's causing us problems, it would be better to get rid of that behavior rather than adding special checks for it. (It especially doesn't make sense to me when drupalSetContent() is called in the context of drupalPostAJAX(), since the entire point of AJAX is that you stay on the same URL after it's posted...)

  3. +        $port = isset($parts['port']) ? ':' . $parts['port'] : '';
    +        $url = $parts['scheme'] . '://' . $parts['host'] . $port;
    

    We have 'port', 'scheme', and 'host', but what about 'user' and 'pass'? The parse_url() function can theoretically return those too.

**********

The attached patch tries a different approach that deals with all the above issues. (For #3, I found it conceptually simpler to strip stuff off the end of the URL rather than try to build a new one up from scratch, so I tried it that way.)

I ran the Secure Pages tests, and this patch works as well as the previous one in terms of the number of tests it is able to make pass.

I also think I found a way to write tests for this in core that doesn't assume the server is actually running HTTPS, so I've added those to the patch too. They pass for me locally, but let's see what happens here.

David_Rothstein’s picture

So the tests passed, but with only ~30,000 assertions rather than ~34,000. That seems odd. And looking at the results, it seems like some of them just didn't run at all... I'll trigger the testbot again.

David_Rothstein’s picture

#24: 471970-24.patch queued for re-testing.

David_Rothstein’s picture

OK, 34,000 this time; that looks better. Who knows what happened with the previous testbot run...

mstef’s picture

FileSize
8.41 KB

Rerolled #24 to apply to drupal root (7.x) - needed for drush make.

Status: Needs review » Needs work
Issue tags: -SecurePages, -Needs backport to D7, -D7 stable release blocker

The last submitted patch, 471970-28.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: +SecurePages, +Needs backport to D7, +D7 stable release blocker

#24: 471970-24.patch queued for re-testing.

David_Rothstein’s picture

Note that #24 already contained a D7 patch...

I'm retesting the D8 patch to keep this at "needs review" and make sure it still passes tests.

Everett Zufelt’s picture

@David, I have some time to review this patch. Can you please tell me if there is anything in particular that I should be looking for as far as logic goes, or is it a straight code review required?

David_Rothstein’s picture

Thanks Everett!

For the most part I think it just needs a straight code review. I guess the trickiest part, though, is the tests I added... Because this functionality can be affected by the server where the tests are running on (whether or not it supports HTTPS), it was a little complicated to write tests that were actually valid in either situation. I think I succeeded but it would definitely be useful to get confirmation/review of that.

Everett Zufelt’s picture

@David

The code review was good. I'll run the tests on head on a server with ssl enabled, and then try again with ssl disabled and report back.

Everett Zufelt’s picture

Status: Needs review » Needs work

I tested this locally using MAMP Pro 2.0.5 on an SSL only site at https://drupalcore/...

Test the internal browser of the testing framework.
23 passes, 6 fails, 0 exceptions, and 6 debug messages

If you want the details please let me know.

Everett Zufelt’s picture

Test results on same setup, but with SSL disable for the virtualhost

Test the internal browser of the testing framework.
29 passes, 0 fails, 0 exceptions, and 6 debug messages

chaskype’s picture

What is the patch for Drupal 7?

grendzy’s picture

#13 should still apply to 7.x (that's the last one without the /core directories in the patch headers).

rickmanelius’s picture

Recap of where we are

  • #13 does work (and in fact is being used on a lot of D7 sites that require a stable release of secure pages)
  • @chx brought up some issues in #21 which appear to be addressed by @David_Rothstein in #24-#31.
  • The patches in #24 by @David_Rothstein pass for D7 but not for D8.
  • @Everett Zufelt reviewed the code and gave an ok on the code review
  • @Everett Zufelt also ran this through simpletest (not sure if D7 or D8) and works only with SSL disabled.

Questions:

  • Is the code review for D7, D8, or both?
  • Is this code review sufficient or are there still some concerns as per #21?
  • Does the test still require passing w/SSL enabled or are the results in #35 a deal breaker?

The next Drupal 7 release is coming up and it would be great to get this patch fixed so we can finally get a stable release of secure pages for D7 (http://drupal.org/project/securepages). I'm an intermediate level drupal developer and can help where my skill level is appropriate. If the road ahead is mainly testing and tasks (but the code is already looking good), then I can help complete this issue.

Thoughts?

Everett Zufelt’s picture

My thought is that we want to avoid future technical debt and to have the tests pass both in non-ssl and ssl environments. I am pretty certain that my review was against D8.

mpdonadio’s picture

Status: Needs work » Needs review

#13: 471970.patch queued for re-testing.

mpdonadio’s picture

Didn't mean to resubmit that patch for testing. Sorry.

miscul’s picture

mistake

coolestdude1’s picture

http://drupal.org/node/961508#comment-6177714 Same for this issue. One month no progress. Secure pages issue blocker. Drupal 7 migration blocker. Although patches work many sites are not taking the risk, we should really push on this.

webchick’s picture

This patch is stuck at "needs review." Rather than asking other people to push, since you need this functionality, how about taking action yourself, and reviewing/testing the patch? :)

sun’s picture

Status: Needs review » Needs work
Issue tags: -D7 stable release blocker

Needs re-roll against HEAD.

coolestdude1’s picture

Hi webchick, sorry about the message before. I did not mean to bug or pester anyone with this alert but simply subscribing does not move the issue up in your alerts.

I have taken your suggestion and tested the patches submitted in this ticket against D7. I currently work on D6 and D7 sites. I am not allowed to mess with the upstream D8 as of yet as at my work place I am not allowed to work on anything I can't sell to customers.

I have gotten a couple of tickets recently from customers asking if there was a stable secure side of a drupal environment and as of yet I had to tell them all no. I love drupal and my job deals primarily with drupal but I have to be honest to people. These issues are so close and I do have to give praise to the people that have put effort into them, thank you all so much <3

My testing results from using simple test on my fresh D7.x Dev branch install on my Centos box 6.2 with a self signed cert have all passed using the patch for D7 in #24. I have found that the only errors I get are coming from other core modules as I ran the simpletest module (aka testing) against all of the default configs.

I am available sporadically throughout the week as my deadlines allow and would be willing to test further. Just go ahead and give me a heads up with what you want tested but only for D7.

Currently I think the ticket is stuck on security testing, something that I am not familiar with and would probably not even be able to do. Also sun just posted asking for a reroll? Not quite sure what that means. I guess that just shows how much of a core noob I am haha. :)

Well I hope all is well and I wish everyone the best!

danielgmc’s picture

Status: Needs work » Needs review

#13: 471970.patch queued for re-testing.

sbandyopadhyay’s picture

FileSize
8.45 KB

Per sun's request (#46), this is a re-roll against HEAD of David Rothstein's patch in #24.

Perignon’s picture

I'm about to use Patch #13 on a live site as I need securepages for Drupal Commerce. I been testing tonight and thus far so good. I launch the site in less than 4 weeks so I will keep tabs on this thread. Being that D7 is a great eCommerce platform with Drupal Commerce the ability to SSL switching on pages with Securepages is a must have. I can't encrypt the entire site cause of the social media hooks of the website that come across unencrypted.

rbrownell’s picture

Hello;

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?

klonos’s picture

Does anybody know if the patch has been put into D7 core (e.g. the latest 7.15 release)?...

same answer as bellow:

...Or is even on the agenda for D7 core development?

Things are first fixed in latest branch (currently D8) - the backported to previous supported branch (D7). You can tell if a bug is meant to be fixed in the previous release by taking a look at the issue tags looking for a "needs backport to D7" tag (which this issue here does have). When an issue is actually backported, the tag is removed or the issue is marked as fixed.

Hope this answers both your questions Ryan ;)

Lloyd’s picture

#49: 471970-49.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +SecurePages, +Needs backport to D7

The last submitted patch, 471970-49.patch, failed testing.

adamt19’s picture

op: 5/26/2009

3.5 years!?

jazzdrive3’s picture

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.

webchick’s picture

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 #49 and is currently in need of a re-roll since it does not apply to 8.x anymore. If you want to push this forward, you should check out a copy of Drupal 8 from Git, apply the patch, and attempt to re-roll the patch using the instructions at http://drupal.org/patch/reroll. #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.

krishworks’s picture

First, rewinding head to replay your work on top of it...
Applying: Applying patch from http://drupal.org/node/471970#comment-5370470
Using index info to reconstruct a base tree...
A	core/modules/simpletest/drupal_web_test_case.php
A	core/modules/simpletest/simpletest.test
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): core/modules/simpletest/simpletest.test deleted in HEAD and modified in Applying patch from http://drupal.org/node/471970#comment-5370470. Version Applying patch from http://drupal.org/node/471970#comment-5370470 of core/modules/simpletest/simpletest.test left in tree.
Auto-merging core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
Failed to merge in the changes.
Patch failed at 0001 Applying patch from http://drupal.org/node/471970#comment-5370470

I tried to re-roll the patch in #24. I got this above error. Can someone shed some light on how to proceed with this?

xjm’s picture

@krishworks, the patch in #24 was created before a lot of file moves, so will not rebase well to 8.x. #49 is probably a better bet.

krishworks’s picture

Status: Needs work » Needs review
FileSize
4.92 KB

manually implemented the patch in #49 against head.

xjm’s picture

Thanks @krishworks!

Status: Needs review » Needs work

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

jazzdrive3’s picture

So what next. How do we fix the testing fails? Is that necessary before reviewing?

mgifford’s picture

The patch applies nicely.

I can't make sense of why it fails, but unit tests are brutal to figure out much of the time.

@xjm any ideas on how to work through the errors?

Would be good to fix this for so many reasons.

dcam’s picture

Status: Needs work » Needs review
Issue tags: -SecurePages, -Needs backport to D7

#60: 471970.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +SecurePages, +Needs backport to D7

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

EclipseGc’s picture

FileSize
8.43 KB

Actually rerolled the WHOLE patch David supplied in #24.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review

Needs testing.

coolestdude1’s picture

Tested using fresh install of version 8.x in the repo. Git log reports at commit hash 522188692156731724b59bcfc06207b5b0d1e3ec (Latest commit to D8 Sun Dec 3 by catch).
Patch from #67 applies cleanly. SimpleTest Browser test passes 100% with 29 passes, 0 fails, 0 exceptions, and 6 debug messages.

Running on a CentOS6.3 Box
With self signed SSL cert.
Accessing website using Https

[edit] haha 'git log' is spat out in reverse incorrect hash fixed

star-szr’s picture

#67: 471970_1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +SecurePages, +Needs backport to D7

The last submitted patch, 471970_1.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.33 KB

Rerolled.

mgifford’s picture

#73: 471970-73.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +SecurePages, +Needs backport to D7

The last submitted patch, 471970-73.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
8.34 KB

Rerolled #73.

alberto56’s picture

The patch at #28 no longer applies since to 7.x. This version works for me.

Status: Needs review » Needs work
Issue tags: -SecurePages, -Needs backport to D7

The last submitted patch, core--7.x-471970-77-base-url-for-test.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
Issue tags: +SecurePages, +Needs backport to D7

#76: 471970-76.patch queued for re-testing.

mgifford’s picture

FileSize
16.05 KB

What else needs to be done to get #76 marked RTBC for D8?

It was marked RTBC earlier, but I think @David_Rothstein's post in #24 is a better earlier reference.

I think from reviewing this that if the Bot's happy, then there is no human testing required. Am I wrong in that?

Still installs fine on simplytest.me

aberger’s picture

#13: 471970.patch queued for re-testing.

attila.fekete’s picture

FileSize
8.39 KB

Rerolled #28 to apply to Drupal root (7.x) - needed for drush make.

Status: Needs review » Needs work
Issue tags: -SecurePages, -Needs backport to D7

The last submitted patch, 471970-82.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

#76: 471970-76.patch queued for re-testing.

Lukas von Blarer’s picture

#76: 471970-76.patch queued for re-testing.

Lukas von Blarer’s picture

How can I help here? Is there testing needed? What should be reviewed? What can we do about the 3 fails?

alarcombe’s picture

FileSize
3.27 KB

Patch for 7.22

Status: Needs review » Needs work

The last submitted patch, 471970-87.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
8.36 KB
866 bytes

#1798832: Convert https to $settings was the cause of the fails. This patch passes the failed tests locally.

dcam’s picture

Status: Needs review » Needs work

I have to clarify my comment in #89 about the patch passing tests locally. It only passed tests with a non-SSL parent site.

Like #24, #89 fails local testing with an SSL-only parent site. This is because the test is forced to try and get pages from http:// URLs, but with no http:// URL available a 404 error message is returned. So all the asserts end up failing.

+    // Test that the internal browser always stores the correct URLs for the
+    // location it is pointed at. For example, the URLs should use http:// if
+    // the internal browser is visiting an http:// version of the site,
+    // regardless of whether the parent site (that is running the tests) is on
+    // http:// or https://.

Since the point of the test is for the internal browser to always fetch pages from the http:// URL, I'm not sure how to resolve this. It seems like the test might have to be rewritten.

David_Rothstein’s picture

There are API functions in core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php called httpUrl() and httpsUrl() which return a URL that can be used to fake an HTTP or HTTPS request regardless of the parent site.

Those either didn't exist the last time I worked on this patch, or I didn't know about them. But I think changing the tests here to use those would help solve the problem?

dcam’s picture

I don't think so. SessionHttpsTest also has failures when trying to run it in an SSL-only environment and it's failing when attempting to use the mocked URLs created by httpUrl() and httpsUrl(). I haven't tried running any other tests to see what might fail, but it seems like this may be a problem with other existing code.

rbrownell’s picture

Priority: Normal » Major

I am upgrading this to major because it is one of the full release blocking bugs of secure pages.

rbrownell’s picture

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.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -SecurePages, -Needs backport to D7

#89: 471970-89.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +SecurePages, +Needs backport to D7

The last submitted patch, 471970-89.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

@rbrownell This issue will be fixed in D7, after it is fixed in D8. Usually this is a fairly straight forward process, but not always. @dcam's patch looks like it needs new tests to deal with ssl only sites. I just got the bot's re-engaged to test the last patch, but I think this is going to require more work on the unit tests.

Want to help update the tests?

star-szr’s picture

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

Tagging for reroll.

dlu’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.02 KB

Reroll. Not expecting it to pass the test bot (reasons above), but patch does apply.

mikeytown2’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
FileSize
10.17 KB

Testing Drupal 7.x D7 Code. Will move back to D8 once the testbot reports back. Combines #87 & #82.

mikeytown2’s picture

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

Test passes, moving back to D8

sonicthoughts’s picture

Please see the thread on #961508: Dual http/https session cookies interfere with drupal_valid_token() and the latter comments (around #229!) This issue related to security and affects many systems - it should be considered an exception the the backport policy.

webservant316’s picture

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

Perignon’s picture

@webservant316 I had a lot of issues with securepages. I tried out securelogin and it resolved all of my issues. Worth consideration.

kingdee40’s picture

It looks like securelogin would continue to use https on all pages for an authenticated user. What are the performance impacts of doing this?

webservant316’s picture

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?

webservant316’s picture

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

webservant316’s picture

I am currently upgrading from Drupal 7.26 to 7.28 and checking back here to see if this issue is solved. I installed a patch above in order to use https://drupal.org/project/securepages. Securepages still advise...

Steps to install Secure Pages 7.x-dev
Set $conf['https'] = TRUE; in settings.php.
Apply these two patches:
#961508-71: Dual http/https session cookies interfere with drupal_valid_token()
#471970-13: DrupalWebTestCase->getAbsoluteUrl should use internal browser's base URL
Despite these issues still exist in Drupal 7, and are unlikely to be resolved, we are now working towards a stable release of Secure Pages.

How could it be that this issue would not be resolved in Drupal 7? So everyone that wants to have a mixed http/https session in Drupal 7 needs to patch core? That is hard to believe. I am missing something? Perhaps it would be better to ditch securepages and run https all the time rather than maintain a core patch in my configuration.

webservant316’s picture

Ok I reapplied this core patch to Drupal 7.28 and Securepages. All seems well. I needed to maintain mixed http/https sessions because I need to iframe a few http and https pages. Thus the parent page needs to be http and https correspondingly. How would a non PHP person even use Drupal 7?

rickmanelius’s picture

While bummed I am traveling today (and can't focus on sprints), if there is any brave soul willing to take on getting test coverage on this issue, we can finally get a secure pages mixed mode working out of the box without the need to track core patches in D7...

Or if we can get an exception to the backport rule (a reach I know), that would also be pragmatic/

jhedstrom’s picture

The patch in #100 looks reasonable to me. Are there objections to that approach, or can this be RTBC'd?

webservant316’s picture

I was using this patch in order to use securepages with mixed mode sessions. However, having no desire to maintain a core patch and since this issues does not appear to be moving forward I abandoned the effort and have moved to constant HTTPS for my entire site. If this patch ever gets added to core I will be glad to use it. The main hassle for me is that I was iframing a 3rd party HTTP and HTTPS pages on my site, so I needed to specify HTTP and HTTPS for those pages so that browsers would not complain.

mausolos’s picture

What obstacles remain before this patch is added to the next release?

jhedstrom’s picture

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

The patch in ##9 needs to be rerolled.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Reroll of a 15mo patch? I'm game :)

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
7.35 KB

@jhedstrom , I assume you meant the patch in #99 and not #9...

OK, this was done with a combo splitting out the patch into parts, hand edits to the patch to change filenames, rebase + hand merges, and just plain do-overs. It now cleanly applies to HEAD.

Deviations from the original patch:

BrowserTest::getInfo() is gone.

I made the setup and teardown functions protected to bring them in line with current coding standards.

BrowserTest::testGetAbsoluteUrl() had a drupalPost() in the old patch that I converted to drupalPostForm().

BrowserTest::testGetAbsoluteUrl() used url(), which is gone from HEAD; redid this with Url::fromRoute()->toString().

BrowserTest passes for me locally, but I don't have HTTPS on this machine, so that pass for me is more of a sanity check that it doesn't barf an exception.

I spot checked a few things that extend WebTestBase, and saw no obvious problems. I also read the patches side by side, and the patched files old and new side by side, and didn't see any obvious problems. If this actually passes, someone should do a review to see if there are any API related things that need to change (but I didn't see anything glaring).

TestBot, do your thing.

mpdonadio’s picture

Issue tags: -Needs reroll

Ok, that passed. Removing tag. Still needs to be eyeballed by someone else.

rickmanelius’s picture

Given that (at first glance based on this announcement https://twitter.com/drupal8changes/status/539580689851899905) it appears that this has been removed from Drupal core in 8.x (see #2342593: Remove mixed SSL support from core), can we remove the backport policy on this ticket and get committed to Drupal 7?

mgifford’s picture

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

That makes sense to me. We can always set it back if we're missing something.

pandroid’s picture

This is ridiculous. The securepages mixed-mode patches work, but they are not getting rolled into D7 core, so we keep having to patch core every time a release comes out. What does it take to get these committed?

mausolos’s picture

@pandroid #120

Try this:
https://www.acquia.com/blog/patching-drush-make

That said, I have to agree. What steps need to be taken to get this going?

Thanks!

jollysolutions queued 13: 471970.patch for re-testing.

mausolos’s picture

So it's been about a year since the latest patch passed review and a request was made to add to core. Are we ready to move to the next step?

mgifford’s picture

Status: Needs review » Needs work

Needs re-roll.

sonicthoughts’s picture

Just a reminder to the maintainer that over 25,000 sites are using securepages which is blocked by this issue #471970: DrupalWebTestCase->getAbsoluteUrl should use internal browser's base URL - This is really a security issue and should be escalated rather than relying on patches (which rarely survive updates.)

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
10.17 KB

Patch in #100 still seems to apply. Reupload for TestBot.

Status: Needs review » Needs work

The last submitted patch, 126: drupal7-471970-126-fix-simpletest-https-d7.patch, failed testing.

mpdonadio’s picture

DrupalAlterTestCase passes locally for me. This was the log in Jenkins:

23:45:12 exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'jenkins_default_72725.simpletest718947cache_bootstrap' doesn't exist' in /var/www/html/includes/database/database.inc:2171
23:45:12 Stack trace:
23:45:12 #0 /var/www/html/includes/database/database.inc(2171): PDOStatement->execute(Array)
23:45:12 #1 /var/www/html/includes/database/database.inc(683): DatabaseStatementBase->execute(Array, Array)
23:45:13 #2 /var/www/html/includes/database/query.inc(858): DatabaseConnection->query('DELETE FROM {ca...', Array, Array)
23:45:13 #3 /var/www/html/includes/cache.inc(542): DeleteQuery->execute()
23:45:13 #4 /var/www/html/includes/cache.inc(168): DrupalDatabaseCache->clear('hook_info', false)
23:45:13 #5 /var/www/html/includes/module.inc(724): cache_clear_all('hook_info', 'cache_bootstrap')
23:45:13 #6 /var/www/html/includes/registry.inc(96): module_implements('', false, true)
23:45:13 #7 /var/www/html/includes/bootstrap.inc(3255): _registry_update()
23:45:13 #8 /var/www/html/includes/module.inc(452): registry_update()
23:45:13 #9 /var/www/html/modules/simpletest/drupal_web_test_case.php(1489): module_enable(Array, false)
23:45:13 #10 /var/www/html/modules/simpletest/tests/common.test(996): DrupalWebTestCase->setUp('system_test', 'locale')
23:45:13 #11 /var/www/html/modules/simpletest/drupal_web_test_case.php(514): DrupalHTTPRequestTestCase->setUp()
23:45:13 #12 /var/www/html/scripts/run-tests.sh(371): DrupalTestCase->run()
23:45:13 #13 /var/www/html/scripts/run-tests.sh(22): simpletest_script_run_one_test('1', 'DrupalHTTPReque...')
23:45:16 #14 {main}FATAL PageTitleFiltering: test runner returned a non-zero error code (1).

That looks like a bot problem, so I am triggering a retest.

mpdonadio’s picture

Status: Needs work » Needs review

OK, retest of #100 / #126 came back green, so setting Needs Review.

Review away. Secure Pages users should see if this still works for them.

torgosPizza’s picture

Adding some related issues so they don't get lost in this long thread. From what I can tell #2245003: Use a random seed instead of the session_id for CSRF token generation will need to be RTBC next (if and when a patch from this issue is committed) in order to get the Secure Pages issue resolved.

mausolos’s picture

I diff'd this against what I had from months ago, and it's still exactly the same. Since no code has changed and the desired behavior seems to be consistent, I'm not sure what we're supposed to be reviewing here?

mpdonadio’s picture

#131, we just need to review that this patch still works and the approach is good, and then set RTBC or Needs Work as appropriate. As far as I can tell, this has never really been at RTBC status, despite widespread use in the wild.

mausolos’s picture

Well, consider this a +1 towards RTBC :)

David_Rothstein’s picture

I hate to ask this, but are we sure this issue is no longer relevant for Drupal 8?

As far as I recall from working on it (and from the issue title) this issue is just about a basic bug in the Simpletest browser URL handling which would apply equally well to Drupal 7 and 8. It's not directly related to mixed-mode SSL support or #2342593: Remove mixed SSL support from core at all.

I.e. if you look at the patch, it seems to me like the only place we're doing anything with Drupal 7's mixed-mode SSL support is in the tests:

   function testGetAbsoluteUrl() {
....
+    // Store the URLs of the pages we will direct the internal browser to visit
+    // during this test. Force them to be http:// URLs regardless of the
+    // environment the test is running in.
+    variable_set('https', TRUE);
+    $user_login_absolute_url = url('user/login', array('absolute' => TRUE, 'https' => FALSE));
+    $user_register_absolute_url = url('user/register', array('absolute' => TRUE, 'https' => FALSE));

And based on https://www.drupal.org/node/2384903 that would work exactly the same for Drupal 8, the only difference being that the variable_set('https', TRUE) line would just be left out.

mpdonadio’s picture

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

Here is a reroll of #116 so we can have a test run to see what happens in 8.0.x.

$ git checkout 13aef43
$ git apply drupal8.simpletest-module.471970-116.patch
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: drupal8.simpletest-module.471970-116.patch
Using index info to reconstruct a base tree...
M	core/modules/simpletest/src/Tests/BrowserTest.php
M	core/modules/simpletest/src/WebTestBase.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/simpletest/src/WebTestBase.php
CONFLICT (content): Merge conflict in core/modules/simpletest/src/WebTestBase.php
Auto-merging core/modules/simpletest/src/Tests/BrowserTest.php
Failed to merge in the changes.
Patch failed at 0001 drupal8.simpletest-module.471970-116.patch

Think this is a good hand merge. Also removed the $this->settingsSet('mixed_mode_sessions', TRUE); line.

However, IIRC, the Secure Pages issue is that the mixed-mode support provided in this patch is so that the tests in Secure Pages works properly, which do have mixed-mode cases.

mpdonadio’s picture

How about attaching the patch...

Status: Needs review » Needs work

The last submitted patch, 136: 471970-135.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kaidjohnson’s picture

kaidjohnson’s picture

Whoops... And now the reroll using the 7.x-dev tests for previous patch...

kaidjohnson’s picture

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

The last submitted patch, 139: drupal7-fix-simpletest-https-471970-139-7.x.patch, failed testing.

David_Rothstein’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuneBL’s picture

#140 apply with few warnings:

patching file modules/simpletest/drupal_web_test_case.php
Hunk #1 succeeded at 2098 (offset 26 lines).
Hunk #2 succeeded at 2784 (offset 26 lines).
Hunk #3 succeeded at 2969 (offset 26 lines).
patching file modules/simpletest/simpletest.test
patching file modules/simpletest/tests/ajax.test

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
7.84 KB

Rerolled against 8.4.x.

Status: Needs review » Needs work

The last submitted patch, 148: drupal-n471970-148.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Chris Charlton’s picture

This ticket is tagged 8.x, but the 7.x patch in #140 seems to pass testing and I wondered if it's stalled, still valid, and/or not going to get fixed?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

geoffreyr’s picture

FWIW we've been using the patch in #140 as well for a while now.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bburg’s picture

It seems like we had a 7.x patch RTBC, it never made it into 7.x core?

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Component: simpletest.module » phpunit

Triaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.

This looks like it a Phpunit issue, changing component.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

Here I have rerolled patch #148 for Drupal 8.9.x.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.