Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
for 5.x and 6.x we repeatedly have seen possible XSS holes since devs don't realize they need to sanitize before passing to drupal_set_title (unlike, l(), for example). So, to help prevent this being a security problem, lets jsut check_plain() be default like l() does.
Here's a minimal patch - needs work, since probably most of the uses in core need to be alter to avoid double escaping.
Comment | File | Size | Author |
---|---|---|---|
#92 | make-plain-title-242873-92.patch | 3.1 KB | mfer |
#90 | make-plain-title-242873-90.patch | 25.52 KB | mfer |
#83 | title-test-only-242873-83.patch | 2.7 KB | pwolanin |
#83 | make-plain-title-notest-242873-83.patch | 27.84 KB | pwolanin |
#75 | fu-boot-242873-75.patch | 1.76 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedhere's a more complete patch.
Comment #2
scor CreditAttribution: scor commentedI'm for it. +1. subscribing.
Comment #3
keith.smith CreditAttribution: keith.smith commentedThe patch in 1 had a "boolen" and an "html" in a code comment. Corrected in the attached patch.
(Please do not credit on commit.)
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedfixed one hunk and then tested. looks good.
Comment #5
Crell CreditAttribution: Crell commentedShould this check be on set or get? I'm all for making it more flexible than it is now, where to have HTML in a page title you have to do all sorts of wacky un/re-escaping (and my clients always want to put em or strong or something into a page title), but filtering on set rather than get strikes me as "filter on input" rather than Drupal's policy of "filter on output, in a context-sensitive way". After all, HTML may be allowable in the title in the page itself, but in the
<title>
tag, you want to strip_tags() because anything else is invalid.Comment #6
pwolanin CreditAttribution: pwolanin commented@Crell - I have no idea how your suggestion would work - is there something you want to do that would be difficult with the proposed code?
Also, I think in this case it makes more sense to make the choice on "set". For example, I may know that I've already used theme('placeholder') so the title got check_plain() except for the wrapping &l;em> tags. How woul the output part know which tags to allow or what strings might be safe?
Comment #7
Crell CreditAttribution: Crell commentedOK, here's a situation I run into frequently: User creates a node with title "Review of Some painting with an accent mark". Because "Some painting..." is a proper name of a painting, it needs to be italicized. (And no, the client never budges on that. :-) ). Therefore, the following needs to happen:
<head><title></head>
tag must have the HTML stripped, but accent mark still work.Right now the only way to do that is by calling drupal_get_title() in _phptemplate_variables() or the page preprocess function, running htmlspecialchars_decode() on it, then trying to refilter it myself and split it into two variables. And apostrophies always get b0rked when I do that, too. :-)
By moving the filtering to 'get', that can be replaced with:
Much simpler, and less wacky decoding going on.
Comment #8
pwolanin CreditAttribution: pwolanin commented@Crell - unless I'm missing the point, what you want would not be very usable for the average developer.
Here's a version that lets you force html on either set or get. Seems a little inelegant, but at least I think it would satisfy Crell's concern.
Comment #9
Crell CreditAttribution: Crell commentedLet me try and shorten it:
User enters a node title of
"My thoughts on <em>The Hitchhikers Guide To The Galaxy</em>"
. That's not at all a fringe requirement. Make that work without breaking horribly. I am flexible as to how that happens. :-) I just worry that set-based filtering will continue to break horribly, just as it does now.Comment #10
pwolanin CreditAttribution: pwolanin commentedI want to make this change to enhance security against XSS. The default behavior needs to be very secure (e.g. check_plain). I think the patch in #4 is a good step forward, and it should be committed and then we can investigate further.
@Crell - Sounds like what you want is a patch for node module to filter the title based on the node format rather than with check_plain()? That would be really independent of this proposed change.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedWe put in a check_plain() assumption in titles because people were putting bold and font size large other crap so they up more prominently in 'recent forum post' listings and so on. I agree with Crell that this assumption is no good but it is in various parts of Drupal and this isn't the issue for fixing it. Please use http://drupal.org/node/82751. This issue remains RTBC.
Comment #12
pwolanin CreditAttribution: pwolanin commentedre-rolled #8 against HEAD. Minor change to simplify drupal_set_title() - I think we only need a single $html param.
@moshe & Crell what do you think about this version that allows you to specify unfiltered html either on input or retrieval of the title?
Comment #13
cwgordon7 CreditAttribution: cwgordon7 commentedTests?
Comment #14
pwolanin CreditAttribution: pwolanin commentedThis is really a case where most testing would need to be via unit tests rather than browser-based.
Comment #15
pwolanin CreditAttribution: pwolanin commentedchx pointed out to me that search.test is already doing unit tests via a class that extends DrupalWebTestCase, so this patch adds a test that does the same for these functions, plus check that a node title is handled as expected.
Also, chx and webchick convinced me in IRC to put drupal_get_title() and drupal_set_title() back to the simpler (more comprehensible) form that moshe's set as RTBC in #4
Comment #16
pwolanin CreditAttribution: pwolanin commentedmissed a case in profile.pages.inc that needs html.
Comment #17
cwgordon7 CreditAttribution: cwgordon7 commentedWe talked a bit in IRC, and we agreed that the tests for this could remain extending DrupalWebTestCase as DrupalUnitTestCase is currently in a strange sort of jelly limbo where it neither really exists nor ceases to exist and nothing can really be done with it because no one really knows where it is or what it is meant for or how it is supposed to work. So tests check.
Comment #18
Crell CreditAttribution: Crell commentedReading the patch, I am not clear on how I can allow users to enter entities or HTML in a node title without writing a custom hack around it in the template layer. I am also still not convinced that drupal_set_title() is the place to specify filtering.
Leaving as RTBC, though, since it appears that I am in the minority here.
Comment #19
webchick@Crell that's true, but node titles are currently check_plain()ed, so this patch just keeps with the default behaviour. It would be trivial to submit a follow-up patch to #82751: Allow some HTML tags in node titles that includes drupal_set_title($title, TRUE); to solve your problem for node titles.
I like the latest patch's approach. I see the arguments about this violating our "filter on output" rule, but I think the ease this affords developers, along with not having potentially weird interactions between drupal_set_title() and drupal_get_title()'s HTML flags as was in previous incarnations of this patch.
My only concern is whether:
should be:
But I wasn't able to exploit this in some cursory checking I did with <script> tags in a user's "Interests" field in profile module.
Comment #20
webchickconfirm_form()'s drupal_set_title() needs to accept HTML too. Here's a re-roll that includes that.
Comment #21
webchickUsing grep between a fresh HEAD install and one with this patch, the following don't look like they were touched by this patch, so might be worth a second glance just to make sure things are working as expected. The behaviour seemed the same to me on both the clean and patched install on all of them, except the only one I wasn't able to verify was the batch.inc stuff.
./includes/batch.inc:75: drupal_set_title($current_set['title']);
./includes/batch.inc:118: drupal_set_title($current_set['title']);
./modules/help/help.admin.inc:26: drupal_set_title($module['name']);
./modules/node/node.pages.inc:14: drupal_set_title($node->title);
./modules/taxonomy/taxonomy.pages.inc:29: drupal_set_title($title);
Comment #22
pwolanin CreditAttribution: pwolanin commented@webchick - for the cofirm form, should that be passed through filter_xss_admin()? It seems entirely possible that some contrib modules could be passing user input as part of the message... If that seems like a good idea, that little piece would be trivial to backport to 6.x
Comment #23
pwolanin CreditAttribution: pwolanin commentedok, here's a version with filter_xss_admin added for the confirm form.
Comment #24
pwolanin CreditAttribution: pwolanin commentedSince the title passed into batch is of unknown origin and could have HTML, we could wrap it with filter_xss() to pass some basic tags but strip script?
the node.pages.inc line 14 in D6 got check_plain() as SA-2008-18, and HEAD never got the patch apparently. This patch should resolve that hole.
in taxonomy.pages.inc, there is a check_plain() on the line above which can be removed.
Comment #25
webchickfilter_xss() on batch.inc sounds reasonable. Someone can always submit a follow-up patch to make it less restrictive if it's hurting anything.
Comment #26
pwolanin CreditAttribution: pwolanin commentedok, here's an updated patch.
Created a separate issue for a patch for the confirm form and batch_set() for D6: http://drupal.org/node/262514
Comment #27
pwolanin CreditAttribution: pwolanin commentedslightly revised to match http://drupal.org/node/262514 and filter both the description and the question in the confirm form.
Comment #28
pwolanin CreditAttribution: pwolanin commentedfix minor code-style problems in the .test code.
Comment #29
webchickI think this is ready to go. I clicked around to all the usual suspects, and then ran a bunch of the tests too. I originally got failures in node revisions and search, but I think this has to do with static variable wonkiness, because when I went back and ran those two tests individually, they passed fine.
Comment #30
pwolanin CreditAttribution: pwolanin commentedre-roll for HEAD - just a little offset/fuzz
Comment #31
bjaspan CreditAttribution: bjaspan commentedSubscribe.
@Crell, I have an excellent solution to the "HTML in titles" problem which unfortunately is too small to fit in this margin. Tomorrow, though, it won't be. :-)
Comment #32
bjaspan CreditAttribution: bjaspan commentedWithin the existing approach that Drupal node titles are constrained/assumed to be plain text, I like the general solution of making drupal_set_title() use check_plain() by default with an override. I disagree with crell's position that doing this in set_title() instead of get_title() constitutes "filtering on input." "Input" is when the title comes from the user and is stored in the db. From a module developer's point of view, drupal_set_title() is output. From a theme developer's point of view, drupal_get_title() is assumed to be safe just like $field_foo[0]['view'] is assumed to be safe. So it is consistent to do it in set_title().
What I do not like is using the highly ambiguous TRUE as the ticket to writing insecure code. Using a literal constant as a positional function argument is poor API design and poor Developer Experience (in fact, I sense a new DX Files post coming on). It requires programmers to remember that argument exists when writing code and requires them to understand what it means when reviewing code. When a non-Drupal-guru looks at
it is far from obvious that $foo better not contain XSS.
Instead, we should use a defined constant such as:
Now it is clear. Anyone who sees that knows what $foo is expected to contain. I'm not sure that DRUPAL_VALIDATED_HTML is the right constant name to use; suggestions encouraged.
An important detail is that DRUPAL_VALIDATED_HTML (or whatever we call it) must be something different than TRUE or 1 or '1' and must be compared with ===, not ==, so that drupal_set_title($foo, TRUE) not only is wrong but will not work and will fail safe. Only the expected defined constant argument will achieve the desired result.
Yes, there are other places in Drupal (many of them) where we use TRUE/FALSE as positional arguments like this. They should all be fixed, but we gotta start somewhere, and this particular issue addresses XSS security bugs so is a fine place to start.
Comment #33
bjaspan CreditAttribution: bjaspan commentedAddenda:
1. An alternative to the defined constant is two separate API functions:
This is a bigger change from our current API; I still suggest the defined constant instead of TRUE.
2. @crell: For my solution to formatted titles, see http://drupal.org/node/82751#comment-876871.
Comment #34
webchickA named constant is definitely preferable to $html = TRUE, because there are actually at least two different things I can think of that you could mean by $html = TRUE:
1. HTML that you don't trust at all. For example, a typical node title, which is usually the very definition of untrusted user-generated input.
2. HTML that you can probably trust. For example, the node title of a node that can only be created by administrative users.
It's also only one function for developers to learn.
But, I think what might be even easier, is to just pass along a $format argument instead that's just passed along to check_markup(), which would default to -1 / FILTER_FORMAT_PLAIN_TEXT or something which means "Don't use a format, just use check_plain().". Developers can then re-use their knowledge of how check_markup works here.
Comment #35
webchickOops. I forgot to mention, this would also let you use format like BBCode in the title, which might be all some users might know, esp. on forum systems.
Comment #36
catchHaving had people put bbcode in node titles expecting it to work, that sounds like a good approach.
Comment #37
pwolanin CreditAttribution: pwolanin commentedI feel uncomfortable about using check_markup and it's codes - seems like you could readily end up in an insecure situation if (for example) the default format is full HTML
Comment #38
webchickMaybe, but it's more secure than what the patch is currently doing, which is just passing along $html straight out to the browser, with zero filtering. There's no reason you can't do exactly what you're doing here (filter_xss_admin first then passing to drupal_set_title($title, FILTER_FORMAT_DEFAULT)) if you're severely concerned that the default might be insecure.
We also need to make it more clear to people not to choose Full HTML by default, but that's another patch. :)
Comment #39
pwolanin CreditAttribution: pwolanin commented@webchick - then potentially we are running filter_xss twice on the same string (which seems silly for performance reasons)?
Still, I can see the advantages in terms of site customization.
Comment #40
Crell CreditAttribution: Crell commentedThis sounds like a much more promising direction. Ideally we would also be able to set which input format to use for node tiles, with a default of check_plain(). (I am absolutely fine with that being contrib-only, as long as it is possible.)
Remember, though, that the title gets used twice: Once in the title tag, and once in the page body. The former absolutely never supports HTML. The latter may under some circumstances. So there still needs to be not a check_plain() but a strip_tags() applied to it at some point for use in the page header.
Comment #41
catchSince this is so easy to get wrong I'm changing the status.
Comment #42
pwolanin CreditAttribution: pwolanin commentedBarry and I were reasonably happy with
ALREADY_SANITIZED
as the define.Comment #43
pwolanin CreditAttribution: pwolanin commentedTalking to Heine in IRC - he is unhappy with sanitizing in the batch and confirm form code when we have no particular information about where the strings are coming from. He suggests in these cases just updating the doxygen to make it clear that these strings are expected to be sanitized by the originating module.
Comment #44
pwolanin CreditAttribution: pwolanin commenteda new patch - the main change is the define and no longer doing filtering for confirm form and batch (just doxygen changes)
Comment #45
pwolanin CreditAttribution: pwolanin commentedclean up for doxygen > 80 chars and rename param from $filter to $input based on catch's suggestion.
Comment #46
catchI'm not anywhere I can test at the moment but visually this looks good to me - including the constants and $input.
Comment #47
abdul87 CreditAttribution: abdul87 commentedThanks crell :)
Comment #48
catchI finally got time to test this - running through obvious simpletests goes fine, and as before the function signature etc. all looks good to me. I think this can be set back to RTBC.
Comment #49
keith.smith CreditAttribution: keith.smith commentedI'm leaving this at RTBC because according to a diff between the previous patch and mine, I changed only the following lines:
- //Create the node with HTML in the title
+ // Create the node with HTML in the title.
Comment #50
pwolanin CreditAttribution: pwolanin commentedre-roll to remove offset and fuzz.
Comment #51
pwolanin CreditAttribution: pwolanin commentedwith the patch, all tests pass except for known failures:
Site-wide contact form: 120 passes, 4 fails, 0 exceptions
Core filters: 48 passes, 12 fails, 0 exceptions
Poll create: 27 passes, 7 fails, 0 exceptions
XML-RPC validator 0 passes, 0 fails, 3 exceptions
Comment #52
pwolanin CreditAttribution: pwolanin commentedWorked though and re-checked this patch again. Added ALREADY_SANITIZED to a few simple t() strings. Since they were not getting check_plain before, the intent of this patch was not to alter that behavior.
Ran all tests - all pass except the known fails in filters (68 passes, 12 fails, 0 exceptions). New test added by the patch passes, of course.
Comment #53
catchAll tests pass, patch looks good. I agree that we can look at those few strings in a follow up patch if necessary.
Comment #54
lilou CreditAttribution: lilou commentedPatch partially apply ... re-roll to CVS HEAD.
Comment #55
c960657 CreditAttribution: c960657 commentedThere are three types of text in play here:
I think the naming of the constant NOT_SANITIZED and the documentation for the $input argument to drupal_set_title() indicate that $title is actually HTML that will be sanitized, not that it is actually plain text that is converted to HTML (plain text cannot contain dangerous stuff and thus doesn't need to be "sanitized").
I suggest renaming NOT_SANITIZED and ALREADY_SANITIZED to PLAIN_TEXT and HTML_ALREADY_SANITIZED to make it clear that they don't refer to two different flavours of HTML but rather to two different text encodings.
Comment #56
stella CreditAttribution: stella commentedsubscribing
Comment #57
pwolanin CreditAttribution: pwolanin commented@c960657 - I don't care what the string is supposed to contain. I.e. it might be expected to be plain text but have injected script.
Anyhow, it seems we need to clairfy the doxygen. Also, Barry had a pending patch to fix one node title call in HEAD which might have done in and would necessitate a re-roll?
Comment #58
pwolanin CreditAttribution: pwolanin commenteddue to : http://drupal.org/node/268706 and DB patch, a re-roll and doxygen cleanup.
Comment #59
stella CreditAttribution: stella commentedThe lines within the function "testTitleTags" in system.test are indented by 3 spaces instead of by 2.
This was spotted thanks to the new Coder module patch review functionality. It does throw up other warnings, but they're false positives - one is due to string concatenation standards change which is not in Coder yet, and the other one is a bug in Coder which I'm working on (it only happens when a patch begins with a comment and causes /*/ line warnings).
Cheers,
Stella
Comment #60
pwolanin CreditAttribution: pwolanin commentedre-roll to fix spacing (brought to you by a long layover in Heathrow...)
Comment #61
stella CreditAttribution: stella commentedChecks out fine against Coder's checks.
Comment #62
c960657 CreditAttribution: c960657 commentedPlain text can - by definition - not contain script.
I may not have explained my objection clearly enough. My point is just that calling check_plain() on a string isn't really "sanitizing" the string but rather converting it from plain text to HTML. On the other hand, filter_xss() takes a string that is already HTML and then sanitizes it and returns (another) string that is also HTML. I think there is an important distinction between text-to-HTML conversion and sanitization of HTML, and I just want this to be clear in the wording of the documentation and the naming of the constants.
What I am suggesting is something like this:
Comment #63
pwolanin CreditAttribution: pwolanin commentedpatch in #60 still applies cleanly (with offset). All tests pass: 5408 passes, 0 fails, 0 exceptions
Comment #64
catchNo longer applies, but I had another read through and it still looks rtbc to me.
Comment #65
c960657 CreditAttribution: c960657 commentedFWIW, the terms "Restricted HTML" and "Unrestricted HTML" are suggested in #245052: Rename input formats to reflect their purpose.. They don't cover exactly the same concepts, but they are closely related.
Comment #66
pwolanin CreditAttribution: pwolanin commentedok, seems like just a whitespace change made the new test class hunk not apply. re-rolled.
Comment #67
catchApplies cleanly and breaks no tests. I asked pwolanin in irc about whether we trust translators, and apparently last word was 'yes we do' (titles defined in code with t() are treated as already sanitised but could contain HTML later on). If we decide later on that translators can't be trusted, we could always change those five or six calls in a followup patch. That issue is also dealt with separately here: #276111: Validate translation strings on input/import
So, RTBC.
Comment #68
lilou CreditAttribution: lilou commentedPatch partially failed :
modules\taxonomy\taxonomy.admin.inc (Cannot apply hunk @@ 7 )
Re-roll against CVS HEAD, and convert all
drupal_set_title(check_plain())
todrupal_set_title()
.Back to CNR.
Comment #69
pwolanin CreditAttribution: pwolanin commented@lilou - thanks for the re-roll.
changing the constants per discussion w/ webchick, sun, and chx.
Comment #70
sunI'd say RTBC after running _all_ tests once more.
Comment #71
pwolanin CreditAttribution: pwolanin commentedRan all tests: 6949 passes, 0 fails, 0 exceptions
Comment #72
pwolanin CreditAttribution: pwolanin commentedtruncated TITLE_ from the constants, move them to common.inc per webchick.
Comment #73
webchickOk.
a) I've reviewed this several times, and think we caught all the weird bugs by now (I hope ;))
b) It has the thumbs-up from several prominent members of the security team.
c) It protects developers/themers from accidentally making silly mistakes.
d) The naming of the constant you need to pass in is obscure enough that you need to REALLY think about it before you do it. ;)
e) It paves the way for doing similar with with l() or other places that people frequently mess up.
f) AND! All tests pass. :)
Therefore, committed to HEAD. Thanks guys!!
Please update the upgrade docs. Probably in the theme upgrade docs too?
Comment #74
pwolanin CreditAttribution: pwolanin commenteddocs updated: http://drupal.org/node/224333#drupal_set_title
Comment #75
pwolanin CreditAttribution: pwolanin commentedoops - we wanted those constants in bootstrap.inc, not common.inc
Comment #76
webchickThanks. :)
Comment #77
webchickHey, Peter... I'm really sorry, but I had to roll this patch back. Tonight we were talking about how when running ALL tests (for whatever reason not when running all but one test, as discovered by mfer) on certain systems (mine and mfer's at least) results in a huge PDOException string that locks up the browser. After 2+ hours of trying to figure out what caused it, we finally discovered it was this patch (thanks to dww for the CVS help).
This is really going to suck, because Peter can't reproduce it on his machine. :P But I can't commit the patch as-is until we figure out what the deal is.
Here's an excerpt:
PDOException: UPDATE {batch} SET token = ?, batch = ? WHERE bid = ? - Array ( [0] => 8927c603a1f11b6247fafb1ad91c8dd2 [1] => a:10:{s:4:"sets";a:1:{i:0;a:12:{s:7:"sandbox";a:0:{}s:7:"results";a:0:{}s:7:"success";b:0;s:5:"title";s:19:"Running SimpleTests";s:10:"operations";a:1:{i:0;a:2:{i:0;s:27:"_simpletest_batch_operation";i:1;a:2:{i:0;a:126:{i:0;s:15:"AddFeedTestCase";i:1;s:18:"UpdateFeedTestCase";i:2;s:18:"RemoveFeedTestCase";i:3;s:22:"UpdateFeedItemTestCase";i:4;s:22:"RemoveFeedItemTestCase";i:5;s:26:"CategorizeFeedItemTestCase";i:6;s:18:"ImportOPMLTestCase";i:7;s:13:"BlockTestCase";i:8;s:20:"NonDefaultBlockAdmin";i:9;s:12:"BlogTestCase";i:10;s:15:"BlogAPITestCase";i:11;s:12:"BookTestCase";i:12;s:26:"BootstrapIPAddressTestCase";i:13;s:26:"BootstrapPageCacheTestCase";i:14;s:25:"BootstrapVariableTestCase";i:15;s:15:"CacheSavingCase";i:16;s:14:"CacheClearCase";i:17;s:20:"CommentInterfaceTest";i:18;s:15:"CommentNodePage";i:19;s:16:"CommentAnonymous";i:20;s:19:"CommentApprovalTest";i:21;s:23:"ContactSitewideTestCase";i:22;s:23:"ContactPersonalTestCase";i:23;s:26:"DatabaseConnectionTestCase";i:24;s:21:"DatabaseFetchTestCase";i:25;s:22:"DatabaseFetch2TestCase";i:26;s:22:"DatabaseInsertTestCase";i:27;s:25:"DatabaseInsertLOBTestCase";i:28;s:30:"DatabaseInsertDefaultsTestCase";i:29;s:22:"DatabaseUpdateTestCase";i:30;s:29:"DatabaseUpdateComplexTestCase";i:31;s:25:"DatabaseUpdateLOBTestCase";i:32;s:22:"DatabaseDeleteTestCase";i:33;s:21:"DatabaseMergeTestCase";i:34;s:22:"DatabaseSelectTestCase";i:35;s:29:"DatabaseSelectOrderedTestCase";i:36;s:29:"DatabaseSelectComplexTestCase";i:37;s:23:"DatabaseTaggingTestCase";i:38;s:21:"DatabaseAlterTestCase";i:39;s:22:"DatabaseAlter2TestCase";i:40;s:26:"DatabaseRegressionTestCase";i:41;s:13:"DBLogTestCase";i:42;s:17:"FileValidatorTest";i:43;s:25:"FileUnmanagedSaveDataTest";i:44;s:18:"FileSaveUploadTest";i:45;s:17:"FileDirectoryTest";i:46;s:23:"FileUnmanagedDeleteTest";i:47;s:21:"FileUnmanagedMoveTest";i:48;s:21:"FileUnmanagedCopyTest";i:49;s:14:"FileDeleteTest";i:50;s:12:"FileMoveTest";i:51;s:12:"FileCopyTest";i:52;s:12:"FileLoadTest";i:53;s:12:"FileSaveTest";i:54;s:16:"FileValidateTest";i:55;s:17:"FileSetStatusTest";i:56;s:16:"FileSaveDataTest";i:57;s:19:"FilterAdminTestCase";i:58;s:14:"FilterTestCase";i:59;s:13:"FormsTestCase";i:60;s:13:"ForumTestCase";i:61;s:12:"HelpTestCase";i:62;s:14:"LocaleTestCase";i:63;s:12:"MenuTestCase";i:64;s:15:"MenuIncTestCase";i:65;s:21:"NodeRevisionsTestCase";i:66;s:18:"NodeTeaserTestCase";i:67;s:16:"PageEditTestCase";i:68;s:19:"PagePreviewTestCase";i:69;s:20:"PageCreationTestCase";i:70;s:16:"PageViewTestCase";i:71;s:20:"NodeTitleXSSTestCase";i:72;s:12:"PathTestCase";i:73;s:17:"PHPFitlerTestCase";i:74;s:17:"PHPAccessTestCase";i:75;s:18:"PollCreateTestCase";i:76;s:16:"PollVoteTestCase";i:77;s:17:"ProfileTestFields";i:78;s:17:"ProfileTestSelect";i:79;s:15:"ProfileTestDate";i:80;s:18:"ProfileTestWeights";i:81;s:23:"ProfileTestAutocomplete";i:82;s:19:"SearchMatchTestCase";i:83;s:14:"SearchBikeShed";i:84;s:24:"SearchAdvancedSearchForm";i:85;s:21:"SearchRankingTestCase";i:86;s:15:"SessionTestCase";i:87;s:18:"SimpleTestTestCase";i:88;s:31:"StatisticsBlockVisitorsTestCase";i:89;s:14:"SyslogTestCase";i:90;s:28:"ActionsConfigurationTestCase";i:91;s:24:"CommonFormatSizeTestCase";i:92;s:26:"DrupalTagsHandlingTestCase";i:93;s:25:"DrupalHTTPRequestTestCase";i:94;s:24:"DrupalSetContentTestCase";i:95;s:25:"RegistryParseFileTestCase";i:96;s:26:"RegistryParseFilesTestCase";i:97;s:25:"EnableDisableCoreTestCase";i:98;s:25:"IPAddressBlockingTestCase";i:99;s:15:"CronRunTestCase";i:100;s:21:"AdminOverviewTestCase";i:101;s:20:"AdminMetaTagTestCase";i:102;s:20:"AccessDeniedTestCase";i:103;s:20:"PageNotFoundTestCase";i:104;s:18:"PageTitleFiltering";i:105;s:30:"TaxonomyVocabularyLoadTestCase";i:106;s:35:"TaxonomyVocabularyFunctionsTestCase";i:107;s:29:"TaxonomyTermFunctionsTestCase";i:108;s:26:"TaxonomyTermSingleTestCase";i:109;s:28:"TaxonomyTermMultipleTestCase";i:110;s:21:"TaxonomyAdminTestCase";i:111;s:27:"TaxonomyTestNodeApiTestCase";i:112;s:16:"TermEditTestCase";i:113;s:11:"TrackerTest";i:114;s:19:"TranslationTestCase";i:115;s:22:"TriggerContentTestCase";i:116;s:14:"UploadTestCase";i:117;s:24:"UserRegistrationTestCase";i:118;s:22:"UserValidationTestCase";i:119;s:18:"UserDeleteTestCase";i:120;s:19:"UserPictureTestCase";i:121;s:23:"UserPermissionsTestCase";i:122;s:17:"UserAdminTestCase";i:123;s:24:"UserAutocompleteTestCase";i:124;s:27:"XMLRPCValidator1IncTestCase";i:125;s:22:"XMLRPCMessagesTestCase";}i:1;s:1:"3";}}}s:8:"finished";s:26:"_simpletest_batch_finished";s:8:"redirect";s:19:"admin/build/testing";s:16:"progress_message";s:17:"Processing tests.";s:3:"css";a:1:{i:0;s:33:"modules/simpletest/simpletest.css";}s:12:"init_message";s:65:"SimpleTest is initializing... 126 test cases will run.
";s:13:"error_message";s:22:"An error has occurred.";s:5:"total";i:1;}}s:4:"form";a:23:{s:5:"tests";a:20:{s:5:"#type";s:8:"fieldset";s:6:"#title";s:5:"Tests";s:12:"#description";s:60:"Select the tests you would like to run, and click Run tests.";s:5:"table";a:46:{s:6:"#theme";s:21:"simpletest_test_table";s:10:"Aggregator";a:16:{s:15:"AddFeedTestCase";a:22:{s:5:"#type";s:8:"checkbox";s:6:"#title";s:22:"Add feed functionality";s:14:"#default_value";i:0;s:12:"#description";s:14:"Add feed test.";s:5:"#post";a:130:{s:15:"AddFeedTestCase";s:1:"1";s:18:"UpdateFeedTestCase";s:1:"1";s:18:"RemoveFeedTestCase";s:1:"1";s:22:"UpdateFeedItemTestCase";s:1:"1";s:22:"RemoveFeedItemTestCase";s:1:"1";s:26:"CategorizeFeedItemTestCase";s:1:"1";s:18:"ImportOPMLTestCase";s:1:"1";s:13:"BlockTestCase";s:1:"1";s:20:"NonDefaultBlockAdmin";s:1:"1";s:12:"BlogTestCase";s:1:"1";s:15:"BlogAPITestCase";s:1:"1";s:12:"BookTestCase";s:1:"1";s:26:"BootstrapIPAddressTestCase";s:1:"1";s:26:"BootstrapPageCacheTestCase";s:1:"1";s:25:"BootstrapVariableTestCase";s:1:"1";s:15:"CacheSavingCase";s:1:"1";s:14:"CacheClearCase";s:1:"1";s:20:"CommentInterfaceTest";s:1:"1";s:15:"CommentNodePage";s:1:"1";s:16:"CommentAnonymous";s:1:"1";s:19:"CommentApprovalTest";s:1:"1";s:23:"ContactSitewideTestCase";s:1:"1";s:23:"ContactPersonalTestCase";s:1:"1";s:26:"DatabaseConnectionTestCase";s:1:"1";s:21:"DatabaseFetchTestCase";s:1:"1";s:22:"DatabaseFetch2TestCase";s:1:"1";s:22:"DatabaseInsertTestCase";s:1:"1";s:25:"DatabaseInsertLOBTestCase";s:1:"1";s:30:"DatabaseInsertDefaultsTestCase";s:1:"1";s:22:"DatabaseUpdateTestCase";s:1:"1";s:29:"DatabaseUpdateComplexTestCase";s:1:"1";s:25:"DatabaseUpdateLOBTestCase";s:1:"1";s:22:"DatabaseDeleteTestCase";s:1:"1";s:21:"DatabaseMergeTestCase";s:1:"1";s:22:"DatabaseSelectTestCase";s:1:"1";s:29:"DatabaseSelectOrderedTestCase";s:1:"1";s:29:"DatabaseSelectComplexTestCase";s:1:"1";s:23:"DatabaseTaggingTestCase";s:1:"1";s:21:"DatabaseAlterTestCase";s:1:"1";s:22:"DatabaseAlter2TestCase";s:1:"1";s:26:"DatabaseRegressionTestCase";s:1:"1";s:13:"DBLogTestCase";s:1:"1";s:17:"FileValidatorTest";s:1:"1";s:25:"FileUnmanagedSaveDataTest";s:1:"1";s:18:"FileSaveUploadTest";s:1:"1";s:17:"FileDirectoryTest";s:1:"1";s:23:"FileUnmanagedDeleteTest";s:1:"1";s:21:"FileUnmanagedMoveTest";s:1:"1";s:21:"FileUnmanagedCopyTest";s:1:"1";s:14:"FileDeleteTest";s:1:"1";s:12:"FileMoveTest";s:1:"1";s:12:"FileCopyTest";s:1:"1";s:12:"FileLoadTest";s:1:"1";s:12:"FileSaveTest";s:1:"1";s:16:"FileValidateTest";s:1:"1";s:17:"FileSetStatusTest";s:1:"1";s:16:"FileSaveDataTest";s:1:"1";s:19:"FilterAdminTestCase";s:1:"1";s:14:"FilterTestCase";s:1:"1";s:13:"FormsTestCase";s:1:"1";s:13:"ForumTestCase";s:1:"1";s:12:"HelpTestCase";s:1:"1";s:14:"LocaleTestCase";s:1:"1";s:12:"MenuTestCase";s:1:"1";s:15:"MenuIncTestCase";s:1:"1";s:21:"NodeRevisionsTestCase";s:1:"1";s:18:"NodeTeaserTestCase";s:1:"1";s:16:"PageEditTestCase";s:1:"1";s:19:"PagePreviewTestCase";s:1:"1";s:20:"PageCreationTestCase";s:1:"1";s:16:"PageViewTestCase";s:1:"1";s:20:"NodeTitleXSSTestCase";s:1:"1";s:12:"PathTestCase";s:1:"1";s:17:"PHPFitlerTestCase";s:1:"1";s:17:"PHPAccessTestCase";s:1:"1";s:18:"PollCreateTestCase";s:1:"1";s:16:"PollVoteTestCase";s:1:"1";s:17:"ProfileTestFields";s:1:"1";s:17:"ProfileTestSelect";s:1:"1";s:15:"ProfileTestDate";s:1:"1";s:18:"ProfileTestWeights";s:1:"1";s:23:"ProfileTestAutocomplete";s:1:"1";s:19:"SearchMatchTestCase";s:1:"1";s:14:"SearchBikeShed";s:1:"1";s:24:"SearchAdvancedSearchForm";s:1:"1";s:21:"SearchRankingTestCase";s:1:"1";s:15:"SessionTestCase";s:1:"1";s:18:"SimpleTestTestCase";s:1:"1";s:31:"StatisticsBlockVisitorsTestCase";s:1:"1";s:14:"SyslogTestCase";s:1:"1";s:28:"ActionsConfigurationTestCase";s:1:"1";s:24:"CommonFormatSizeTestCase";s:1:"1";s:26:"DrupalTagsHandlingTestCase";s:1:"1";s:25:"DrupalHTTPRequestTestCase";s:1:"1";s:24:"DrupalSetContentTestCase";s:1:"1";s:25:"RegistryParseFileTestCase";s:1:"1";s:26:"RegistryParseFilesTestCase";s:1:"1";s:25:"EnableDisableCoreTestCase";s:1:"1";s:25:"IPAddressBlockingTestCase";s:1:"1";s:15:"CronRunTestCase";s:1:"1";s:21:"AdminOverviewTestCase";s:1:"1";s:20:"AdminMetaTagTestCase";s:1:"1";s:20:"AccessDeniedTestCase";s:1:"1";s:20:"PageNotFoundTestCase";s:1:"1";s:18:"PageTitleFiltering";s:1:"1";s:30:"TaxonomyVocabularyLoadTestCase";s:1:"1";s:35:"TaxonomyVocabularyFunctionsTestCase";s:1:"1";s:29:"TaxonomyTermFunctionsTestCase";s:1:"1";s:26:"TaxonomyTermSingleTestCase";s:1:"1";s:28:"TaxonomyTermMultipleTestCase";s:1:"1";s:21:"TaxonomyAdminTestCase";s:1:"1";s:27:"TaxonomyTestNodeApiTestCase";s:1:"1";s:16:"TermEditTestCase";s:1:"1";s:11:"TrackerTest";s:1:"1";s:19:"TranslationTestCase";s:1:"1";s:22:"TriggerContentTestCase";s:1:"1";s:14:"UploadTestCase";s:1:"1";s:24:"UserRegistrationTestCase";s:1:"1";s:22:"UserValidationTestCase";s:1:"1";s:18:"UserDeleteTestCase";s:1:"1";s:19:"UserPictureTestCase";s:1:"1";s:23:"UserPermissionsTestCase";s:1:"1";s:17:"UserAdminTestCase";s:1:"1";s:24:"UserAutocompleteTestCase";s:1:"1";s:27:"XMLRPCValidator1IncTestCase";s:1:"1";s:22:"XMLRPCMessagesTestCase";s:1:"1";s:2:"op";s:9:"Run tests";s:13:"form_build_id";s:37:"form-a847b921573c96a3f5f428e835a7e3d8";s:10:"form_token";s:32:"1d3be5e5ad29f8b02043811e5ce3c7e0";s:7:"form_id";s:20:"simpletest_test_form";}s:11:"#programmed";b:0;s:5:"#tree";b:0;s:8:"#parents";a:1:{i:0;s:15:"AddFeedTestCase";}s:14:"#array_parents";a:4:{i:0;s:5:"tests";i:1;s:5:"table";i:2;s:10:"Aggregator";i:3;s:15:"AddFeedTestCase";}s:7:"#weight";i:0;s:10:"#processed";b:1;s:11:"#attributes";a:0:{}s:9:"#required";b:0;s:6:"#input";b:1;s:13:"#return_value";i:1;s:8:"#process";a:1:{i:0;s:17:"form_process_ahah";}s:5:"#name";s:15:"AddFeedTestCase";
...
etc.
Comment #78
webchickmfer did some sleuthing and I believe managed to track it down to the new test that this patch adds. I can try debugging it some tomorrow when I am not at the brink of exhaustion. :P
Comment #79
Dave ReidNeed another rollback of common.inc (http://drupal.org/cvs?commit=145866). The constants for this patch are still there.
Comment #80
pwolanin CreditAttribution: pwolanin commentedrats - i wonder if this is due to some difference in the starting conditions - i.e. which modules do you have enabled when yo go to run the tests?
Should I re-roll the patch without the test part?
Comment #81
pwolanin CreditAttribution: pwolanin commentedAlso, are you running the tests from the browser of the CLI interface?
I've done a fresh checkout, fresh install!?! Since we're all using MAMP 1.7.1 or 1.7.2, this is even odder. I have all core modules enabled except PHP and syslog. Nothing in the patch is affecting any query directly.
Just trying to think about what could be different. Simpletest creates files when it's installed?
Comment #82
pwolanin CreditAttribution: pwolanin commentedThe query throwing the error would sem to be in form.inc in function batch_process()
Comment #83
pwolanin CreditAttribution: pwolanin commentedhere's a patch without the test (which also moves the constants to bootstrap.inc from common.inc) and a separate patch with the test.
Comment #84
mfer CreditAttribution: mfer commentedI did some more test under MAMP 1.7.2 and am still having the white screen of death when I try to run all tests at the same time. When I try to just run one test shy of all tests there aren't any issues. It doesn't matter which one I choose not to run. These tests are run with no optimization or opcode cache.
I've tested this with all core modules enabled and just those that come enabled out of the box. Same result both ways. As far as the tables go, the simpletest table will have no rows added to it while a row is added to simpletest_test_id with the id for this run. The error that sows up in my php_errors.log reads:
I modified the test so that it contains only a getInfo method and a method for the test with $this->assertTrue(TRUE, 'a test');. Still, the white screen of death and the same error.
To make things even more strange, I tried taking an existing known test, copying it, renaming it's class, and I get the WSD. It seems that adding a test to system.test no matter how I do it causes a WSD when I try to run all tests.
Comment #85
Dave ReidI guess I'm in the case where I can't duplicate this error. Go figure. My tests always run different from everyone else.
Comment #86
mfer CreditAttribution: mfer commentedI don't feel nearly as bad. I get the same error on a xampp install under winXP. Although, it is off my codebase that was patched under OSX.
Comment #87
pwolanin CreditAttribution: pwolanin commented@mfer - ah, maybe a clue? Does this happen for *any* new class you add to any test file? Maybe it's a registry issue?
Have you tried submitting the modules page between applying the patch and running tests?
Comment #88
mfer CreditAttribution: mfer commented@pwolanin - It may be some form of registry issue. I've submitted the modules page between applying the patch and trying to run the tests. I've, also, tried clearing the cache_registry tables. Still get the WSD. I went so far as to do a clean install after applying the patch and still get WSD.
Comment #89
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis was a simpletest issue, please see #320374: Simpletest now fails with max_allowed_packet = 1M.
Comment #90
mfer CreditAttribution: mfer commentedThe attached patch rolls the test and patch into one file. The patch in #83 removed the cruft that webchick rolled back earlier. This patch removed that un-needed change.
This should be RTBC, again.
Comment #91
webchickOk. Re-Committed. :) Thanks for all the hard-core sleuthing, guys!
Comment #92
mfer CreditAttribution: mfer commentedThe patch in #90 was missing the hunks from batch.inc, form.inc, and path.inc that was in the earlier patches. oops. The attached patch adds them. If you run the tests before adding it you'll find a node and system test that fail.
Comment #93
webchickD'oh. :P Thanks. I didn't have time to run the test suite again, so hopefully this fixes it.
See you guys next week!
Comment #94
mfer CreditAttribution: mfer commented@webchick enjoy your vacation. I did run the entire test suite after applying that patch and everything passed. I attempted to be thorough after that mishap.
Comment #95
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.