Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#159 | 471970-159.patch | 7.75 KB | ravi.shankar |
#148 | drupal-n471970-148.patch | 7.84 KB | DamienMcKenna |
#140 | drupal7-fix-simpletest-https-471970-140-7.x.patch | 10.13 KB | kaidjohnson |
#139 | drupal7-fix-simpletest-https-471970-139-7.x.patch | 10.13 KB | kaidjohnson |
#136 | 471970-135.patch | 7.76 KB | mpdonadio |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedFixed in #340283: Abstract SimpleTest browser in to its own object.
Comment #2
grendzy CreditAttribution: grendzy commentedThanks for the update! This is what I had been working on, but I like the new browser object a lot better.
Comment #3
grendzy CreditAttribution: grendzy commentedThis 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
Comment #4
grendzy CreditAttribution: grendzy commentedpatch in #2 no longer applies.
Comment #5
grendzy CreditAttribution: grendzy commentednew patch
Comment #7
mfbSubscribe. This is a big problem for tests that span across multiple schemes or domains.
Comment #8
mfbLet's see if this passes tests..
Comment #9
mfbLet'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).
Comment #11
mfbThe upgrade path tests fail because update.php isn't a Drupal path. So back to the earlier working logic..
Comment #12
grendzy CreditAttribution: grendzy commentedI'll try to review this soon.
Comment #13
grendzy CreditAttribution: grendzy commentedWorks for me. Needed a re-roll:
Comment #14
BenK CreditAttribution: BenK commentedSubscribing
Comment #15
gregglestagging.
Comment #16
grendzy CreditAttribution: grendzy commented#13: 471970.patch queued for re-testing.
Comment #17
xjmNote that this patch needs to be rerolled for the new directory structure.
Comment #18
xjmOr rather to be fixed in 8.x period to prevent a regression.
Comment #19
xjmReroll for D8 attached.
Comment #20
rickmanelius CreditAttribution: rickmanelius commentedIn an effort to move these along by providing feedback, here is a quick overview.
Comment #21
chx CreditAttribution: chx commenteddrupalPost 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.
Comment #22
mfb@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.
Comment #23
mfbComment #24
David_Rothstein CreditAttribution: David_Rothstein commentedThe current code seems to be intended to work with both, though? If so, we shouldn't change that in this issue.
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...)
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.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commented#24: 471970-24.patch queued for re-testing.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 34,000 this time; that looks better. Who knows what happened with the previous testbot run...
Comment #28
mstef CreditAttribution: mstef commentedRerolled #24 to apply to drupal root (7.x) - needed for drush make.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commented#24: 471970-24.patch queued for re-testing.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedNote 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.
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commented@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?
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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.
Comment #34
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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.
Comment #36
Everett Zufelt CreditAttribution: Everett Zufelt commentedTest 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
Comment #37
chaskype CreditAttribution: chaskype commentedWhat is the patch for Drupal 7?
Comment #38
grendzy CreditAttribution: grendzy commented#13 should still apply to 7.x (that's the last one without the /core directories in the patch headers).
Comment #39
rickmanelius CreditAttribution: rickmanelius commentedRecap of where we are
Questions:
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?
Comment #40
Everett Zufelt CreditAttribution: Everett Zufelt commentedMy 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.
Comment #41
mpdonadio#13: 471970.patch queued for re-testing.
Comment #42
mpdonadioDidn't mean to resubmit that patch for testing. Sorry.
Comment #43
miscul CreditAttribution: miscul commentedmistake
Comment #44
coolestdude1 CreditAttribution: coolestdude1 commentedhttp://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.
Comment #45
webchickThis 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? :)
Comment #46
sunNeeds re-roll against HEAD.
Comment #47
coolestdude1 CreditAttribution: coolestdude1 commentedHi 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!
Comment #48
danielgmc CreditAttribution: danielgmc commented#13: 471970.patch queued for re-testing.
Comment #49
sbandyopadhyay CreditAttribution: sbandyopadhyay commentedPer sun's request (#46), this is a re-roll against HEAD of David Rothstein's patch in #24.
Comment #50
Perignon CreditAttribution: Perignon commentedI'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.
Comment #51
rbrownellHello;
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?
Comment #52
klonossame answer as bellow:
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 ;)
Comment #53
Lloyd CreditAttribution: Lloyd commented#49: 471970-49.patch queued for re-testing.
Comment #55
adamt19 CreditAttribution: adamt19 commentedop: 5/26/2009
3.5 years!?
Comment #56
jazzdrive3 CreditAttribution: jazzdrive3 commentedWhat 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.
Comment #57
webchickIn 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.
Comment #58
krishworks CreditAttribution: krishworks commentedI tried to re-roll the patch in #24. I got this above error. Can someone shed some light on how to proceed with this?
Comment #59
xjm@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.
Comment #60
krishworks CreditAttribution: krishworks commentedmanually implemented the patch in #49 against head.
Comment #61
xjmThanks @krishworks!
Comment #63
jazzdrive3 CreditAttribution: jazzdrive3 commentedSo what next. How do we fix the testing fails? Is that necessary before reviewing?
Comment #64
mgiffordThe 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.
Comment #65
dcam CreditAttribution: dcam commented#60: 471970.patch queued for re-testing.
Comment #67
EclipseGc CreditAttribution: EclipseGc commentedActually rerolled the WHOLE patch David supplied in #24.
Eclipse
Comment #68
EclipseGc CreditAttribution: EclipseGc commentedNeeds testing.
Comment #69
coolestdude1 CreditAttribution: coolestdude1 commentedTested 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
Comment #70
star-szr#67: 471970_1.patch queued for re-testing.
Comment #72
star-szrTagging.
Comment #73
star-szrRerolled.
Comment #74
mgifford#73: 471970-73.patch queued for re-testing.
Comment #76
dcam CreditAttribution: dcam commentedRerolled #73.
Comment #77
alberto56 CreditAttribution: alberto56 commentedThe patch at #28 no longer applies since to 7.x. This version works for me.
Comment #79
dcam CreditAttribution: dcam commented#76: 471970-76.patch queued for re-testing.
Comment #80
mgiffordWhat 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
Comment #81
aberger CreditAttribution: aberger commented#13: 471970.patch queued for re-testing.
Comment #82
attila.fekete CreditAttribution: attila.fekete commentedRerolled #28 to apply to Drupal root (7.x) - needed for drush make.
Comment #84
dcam CreditAttribution: dcam commented#76: 471970-76.patch queued for re-testing.
Comment #85
Lukas von Blarer#76: 471970-76.patch queued for re-testing.
Comment #86
Lukas von BlarerHow can I help here? Is there testing needed? What should be reviewed? What can we do about the 3 fails?
Comment #87
alarcombe CreditAttribution: alarcombe commentedPatch for 7.22
Comment #89
dcam CreditAttribution: dcam commented#1798832: Convert https to $settings was the cause of the fails. This patch passes the failed tests locally.
Comment #90
dcam CreditAttribution: dcam commentedI 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.
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.
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedThere 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?
Comment #92
dcam CreditAttribution: dcam commentedI 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.
Comment #93
rbrownellI am upgrading this to major because it is one of the full release blocking bugs of secure pages.
Comment #94
rbrownellAccording 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.
Comment #95
mgifford#89: 471970-89.patch queued for re-testing.
Comment #97
mgifford@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?
Comment #98
star-szrTagging for reroll.
Comment #99
dlu CreditAttribution: dlu commentedReroll. Not expecting it to pass the test bot (reasons above), but patch does apply.
Comment #100
mikeytown2 CreditAttribution: mikeytown2 commentedTesting Drupal 7.x D7 Code. Will move back to D8 once the testbot reports back. Combines #87 & #82.
Comment #101
mikeytown2 CreditAttribution: mikeytown2 commentedTest passes, moving back to D8
Comment #102
sonicthoughts CreditAttribution: sonicthoughts commentedPlease 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.
Comment #103
webservant316 CreditAttribution: webservant316 commentedWhich is the preferred patch for Drupal 7.26 for the proper operation of securepages?
Comment #104
Perignon CreditAttribution: Perignon commented@webservant316 I had a lot of issues with securepages. I tried out securelogin and it resolved all of my issues. Worth consideration.
Comment #105
kingdee40 CreditAttribution: kingdee40 commentedIt looks like securelogin would continue to use https on all pages for an authenticated user. What are the performance impacts of doing this?
Comment #106
webservant316 CreditAttribution: webservant316 commentedWow - 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?
Comment #107
webservant316 CreditAttribution: webservant316 commentedok this got secure pages working for me - https://drupal.org/comment/8524769#comment-8524769
Comment #108
webservant316 CreditAttribution: webservant316 commentedI 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...
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.
Comment #109
webservant316 CreditAttribution: webservant316 commentedOk 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?
Comment #110
rickmanelius CreditAttribution: rickmanelius commentedWhile 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/
Comment #111
jhedstromThe patch in #100 looks reasonable to me. Are there objections to that approach, or can this be RTBC'd?
Comment #112
webservant316 CreditAttribution: webservant316 commentedI 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.
Comment #113
mausolos CreditAttribution: mausolos commentedWhat obstacles remain before this patch is added to the next release?
Comment #114
jhedstromThe patch in ##9 needs to be rerolled.
Comment #115
mpdonadioReroll of a 15mo patch? I'm game :)
Comment #116
mpdonadio@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.
Comment #117
mpdonadioOk, that passed. Removing tag. Still needs to be eyeballed by someone else.
Comment #118
rickmanelius CreditAttribution: rickmanelius commentedGiven 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?
Comment #119
mgiffordThat makes sense to me. We can always set it back if we're missing something.
Comment #120
pandroid CreditAttribution: pandroid commentedThis 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?
Comment #121
mausolos CreditAttribution: mausolos commented@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!
Comment #123
mausolos CreditAttribution: mausolos commentedSo 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?
Comment #124
mgiffordNeeds re-roll.
Comment #125
sonicthoughts CreditAttribution: sonicthoughts as a volunteer commentedJust 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.)
Comment #126
mpdonadioPatch in #100 still seems to apply. Reupload for TestBot.
Comment #128
mpdonadioDrupalAlterTestCase passes locally for me. This was the log in Jenkins:
That looks like a bot problem, so I am triggering a retest.
Comment #129
mpdonadioOK, retest of #100 / #126 came back green, so setting Needs Review.
Review away. Secure Pages users should see if this still works for them.
Comment #130
torgosPizzaAdding 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.
Comment #131
mausolos CreditAttribution: mausolos commentedI 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?
Comment #132
mpdonadio#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.
Comment #133
mausolos CreditAttribution: mausolos commentedWell, consider this a +1 towards RTBC :)
Comment #134
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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:
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.Comment #135
mpdonadioHere is a reroll of #116 so we can have a test run to see what happens in 8.0.x.
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.
Comment #136
mpdonadioHow about attaching the patch...
Comment #139
kaidjohnson CreditAttribution: kaidjohnson commentedRerolling #126 for 7.50.
Comment #140
kaidjohnson CreditAttribution: kaidjohnson commentedWhoops... And now the reroll using the 7.x-dev tests for previous patch...
Comment #141
kaidjohnson CreditAttribution: kaidjohnson commentedComment #143
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #146
DuneBL#140 apply with few warnings:
Comment #148
DamienMcKennaRerolled against 8.4.x.
Comment #150
Chris CharltonThis 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?
Comment #152
geoffreyr CreditAttribution: geoffreyr commentedFWIW we've been using the patch in #140 as well for a while now.
Comment #154
bburgIt seems like we had a 7.x patch RTBC, it never made it into 7.x core?
Comment #157
quietone CreditAttribution: quietone as a volunteer commentedTriaging 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.
Comment #158
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #159
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have rerolled patch #148 for Drupal 8.9.x.