Posted by grendzy on May 25, 2009 at 6:52am
88 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | simpletest.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D7, SecurePages |
Issue Summary
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.
Comments
#1
Fixed in #340283: Abstract SimpleTest browser in to its own object.
#2
Thanks for the update! This is what I had been working on, but I like the new browser object a lot better.
#3
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
#4
patch in #2 no longer applies.
#5
new patch
#6
The last submitted patch, 471970.patch, failed testing.
#7
Subscribe. This is a big problem for tests that span across multiple schemes or domains.
#8
Let's see if this passes tests..
#9
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).
#10
The last submitted patch, 471970-absolute-url.patch, failed testing.
#11
The upgrade path tests fail because update.php isn't a Drupal path. So back to the earlier working logic..
#12
I'll try to review this soon.
#13
Works for me. Needed a re-roll:
#14
Subscribing
#15
tagging.
#16
#13: 471970.patch queued for re-testing.
#17
Note that this patch needs to be rerolled for the new directory structure.
#18
Or rather to be fixed in 8.x period to prevent a regression.
#19
Reroll for D8 attached.
#20
In an effort to move these along by providing feedback, here is a quick overview.
#21
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.
#22
@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.
#23
#24
The current code seems to be intended to work with both, though? If so, we shouldn't change that in this issue.
+ // 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...)
+ $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.
#25
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.
#26
#24: 471970-24.patch queued for re-testing.
#27
OK, 34,000 this time; that looks better. Who knows what happened with the previous testbot run...
#28
Rerolled #24 to apply to drupal root (7.x) - needed for drush make.
#29
The last submitted patch, 471970-28.patch, failed testing.
#30
#24: 471970-24.patch queued for re-testing.
#31
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.
#32
@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?
#33
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.
#34
@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.
#35
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.
#36
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
#37
What is the patch for Drupal 7?
#38
#13 should still apply to 7.x (that's the last one without the /core directories in the patch headers).
#39
Recap 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?
#40
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.
#41
#13: 471970.patch queued for re-testing.
#42
Didn't mean to resubmit that patch for testing. Sorry.
#43
mistake
#44
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.
#45
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? :)
#46
Needs re-roll against HEAD.
#47
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!
#48
#13: 471970.patch queued for re-testing.
#49
Per sun's request (#46), this is a re-roll against HEAD of David Rothstein's patch in #24.
#50
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.
#51
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?
#52
same 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 ;)
#53
#49: 471970-49.patch queued for re-testing.
#54
The last submitted patch, 471970-49.patch, failed testing.
#55
op: 5/26/2009
3.5 years!?
#56
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.
#57
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.
#58
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?
#59
@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.
#60
manually implemented the patch in #49 against head.
#61
Thanks @krishworks!
#62
The last submitted patch, 471970.patch, failed testing.
#63
So what next. How do we fix the testing fails? Is that necessary before reviewing?
#64
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.
#65
#60: 471970.patch queued for re-testing.
#66
The last submitted patch, 471970.patch, failed testing.
#67
Actually rerolled the WHOLE patch David supplied in #24.
Eclipse
#68
Needs testing.
#69
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
#70
#67: 471970_1.patch queued for re-testing.
#71
The last submitted patch, 471970_1.patch, failed testing.
#72
Tagging.
#73
Rerolled.
#74
#73: 471970-73.patch queued for re-testing.
#75
The last submitted patch, 471970-73.patch, failed testing.
#76
Rerolled #73.
#77
The patch at #28 no longer applies since to 7.x. This version works for me.
#78
The last submitted patch, core--7.x-471970-77-base-url-for-test.patch, failed testing.
#79
#76: 471970-76.patch queued for re-testing.
#80
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
#81
#13: 471970.patch queued for re-testing.
#82
Rerolled #28 to apply to Drupal root (7.x) - needed for drush make.
#83
The last submitted patch, 471970-82.patch, failed testing.
#84
#76: 471970-76.patch queued for re-testing.
#85
#76: 471970-76.patch queued for re-testing.
#86
How can I help here? Is there testing needed? What should be reviewed? What can we do about the 3 fails?
#87
Patch for 7.22
#88
The last submitted patch, 471970-87.patch, failed testing.
#89
#1798832: Convert https to $settings was the cause of the fails. This patch passes the failed tests locally.
#90
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.
#91
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?
#92
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.