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.

CommentFileSizeAuthor
#92 make-plain-title-242873-92.patch3.1 KBmfer
#90 make-plain-title-242873-90.patch25.52 KBmfer
#83 title-test-only-242873-83.patch2.7 KBpwolanin
#83 make-plain-title-notest-242873-83.patch27.84 KBpwolanin
#75 fu-boot-242873-75.patch1.76 KBpwolanin
#72 make-plain-title-242873-72.patch29.96 KBpwolanin
#69 make-plain-title-242873-69.patch30.18 KBpwolanin
#68 issue-242873.patch30 KBlilou
#66 make-plain-title-242873-66.patch31.29 KBpwolanin
#60 make-plain-title-242873-60.patch31.27 KBpwolanin
#58 make-plain-title-242873-58.patch31.28 KBpwolanin
#54 make-plain-title-242873-53.patch29.9 KBlilou
#52 make-plain-title-242873-52.patch31.03 KBpwolanin
#50 make-plain-title-242873-50a.patch27.34 KBpwolanin
#49 make-plain-title-242873-46.patch27.14 KBkeith.smith
#45 make-plain-title-242873-45.patch27.07 KBpwolanin
#44 make-plain-title-242873-44.patch27.15 KBpwolanin
#30 make-plain-title-242873-30.patch26.29 KBpwolanin
#28 make-plain-title-242873-28.patch26.38 KBpwolanin
#27 make-plain-title-242873-27.patch26.37 KBpwolanin
#26 make-plain-title-242873-26.patch25.79 KBpwolanin
#23 make-plain-title-242873-23.patch23.17 KBpwolanin
#20 make-plain-title-242873-20.patch23.7 KBwebchick
#16 make-plain-title-242873-16.patch22.44 KBpwolanin
#15 make-plain-title-242873-15.patch21.86 KBpwolanin
#12 make-plain-title-242873-12.patch20.73 KBpwolanin
#8 make-plain-title-242873-8.patch20.57 KBpwolanin
#4 mw.patch19.3 KBmoshe weitzman
#3 make-plain-title-242873-1.patch19.64 KBkeith.smith
#1 make-plain-title-242873.patch19.64 KBpwolanin
make-plain-title.patch1.05 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Needs work » Needs review
FileSize
19.64 KB

here's a more complete patch.

scor’s picture

I'm for it. +1. subscribing.

keith.smith’s picture

The patch in 1 had a "boolen" and an "html" in a code comment. Corrected in the attached patch.

(Please do not credit on commit.)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
19.3 KB

fixed one hunk and then tested. looks good.

Crell’s picture

Should 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.

pwolanin’s picture

@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?

Crell’s picture

OK, 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:

  • The title of the page in the page template must have HTML that is not escaped and shows the accent mark properly.
  • The title of the page in the <head><title></head> tag must have the HTML stripped, but accent mark still work.
  • If the node title shows up in a menu item, it must behave like the page title with working italics and accent marks.

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:

$vars['title'] = strip_tags(drupal_get_title(FALSE), '<em>');
$vars['page_title'] = drupal_get_title();

Much simpler, and less wacky decoding going on.

pwolanin’s picture

@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.

Crell’s picture

Let 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.

pwolanin’s picture

I 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.

moshe weitzman’s picture

We 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.

pwolanin’s picture

re-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?

cwgordon7’s picture

Status: Reviewed & tested by the community » Needs work

Tests?

pwolanin’s picture

This is really a case where most testing would need to be via unit tests rather than browser-based.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
21.86 KB

chx 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

pwolanin’s picture

missed a case in profile.pages.inc that needs html.

cwgordon7’s picture

Status: Needs review » Reviewed & tested by the community

We 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.

Crell’s picture

Reading 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.

webchick’s picture

@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:

$stored_title = $html ? $title : check_plain($title);

should be:

$stored_title = $html ? filter_xss(_admin)($title) : check_plain($title);

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
23.7 KB

confirm_form()'s drupal_set_title() needs to accept HTML too. Here's a re-roll that includes that.

webchick’s picture

Using 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);

pwolanin’s picture

@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

pwolanin’s picture

ok, here's a version with filter_xss_admin added for the confirm form.

pwolanin’s picture

Since 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.

webchick’s picture

filter_xss() on batch.inc sounds reasonable. Someone can always submit a follow-up patch to make it less restrictive if it's hurting anything.

pwolanin’s picture

ok, 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

pwolanin’s picture

slightly revised to match http://drupal.org/node/262514 and filter both the description and the question in the confirm form.

pwolanin’s picture

fix minor code-style problems in the .test code.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

pwolanin’s picture

re-roll for HEAD - just a little offset/fuzz

bjaspan’s picture

Subscribe.

@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. :-)

bjaspan’s picture

Status: Reviewed & tested by the community » Needs work

Within 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

drupal_set_title($foo, TRUE);

it is far from obvious that $foo better not contain XSS.

Instead, we should use a defined constant such as:

drupal_set_title($foo, DRUPAL_VALIDATED_HTML);

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.

bjaspan’s picture

Addenda:

1. An alternative to the defined constant is two separate API functions:

drupal_set_title($title); // will check_plain() its argument
drupal_set_title_validated_html($title); // will not check_plain its argument

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.

webchick’s picture

A 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.

webchick’s picture

Oops. 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.

catch’s picture

Having had people put bbcode in node titles expecting it to work, that sounds like a good approach.

pwolanin’s picture

I 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

webchick’s picture

Maybe, 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. :)

pwolanin’s picture

@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.

Crell’s picture

This 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.

catch’s picture

Category: feature » bug
Priority: Normal » Critical

Since this is so easy to get wrong I'm changing the status.

pwolanin’s picture

Barry and I were reasonably happy with ALREADY_SANITIZED as the define.

pwolanin’s picture

Talking 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
27.15 KB

a new patch - the main change is the define and no longer doing filtering for confirm form and batch (just doxygen changes)

pwolanin’s picture

clean up for doxygen > 80 chars and rename param from $filter to $input based on catch's suggestion.

catch’s picture

I'm not anywhere I can test at the moment but visually this looks good to me - including the constants and $input.

abdul87’s picture

Thanks crell :)

catch’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

keith.smith’s picture

I'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.

pwolanin’s picture

re-roll to remove offset and fuzz.

pwolanin’s picture

with 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

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
31.03 KB

Worked 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

All tests pass, patch looks good. I agree that we can look at those few strings in a follow up patch if necessary.

lilou’s picture

Patch partially apply ... re-roll to CVS HEAD.

c960657’s picture

There are three types of text in play here:

  • Plain text (NOT_SANITIZED)
  • Sanitized HTML (ALREADY_SANITIZED)
  • Unsanitized HTML (not supported by drupal_set_title() - should be sanitized by the caller)

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.

stella’s picture

subscribing

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

@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?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
31.28 KB

due to : http://drupal.org/node/268706 and DB patch, a re-roll and doxygen cleanup.

stella’s picture

Status: Needs review » Needs work

The 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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
31.27 KB

re-roll to fix spacing (brought to you by a long layover in Heathrow...)

stella’s picture

Checks out fine against Coder's checks.

c960657’s picture

@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.

Plain 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:

/**
 * Set the title of the current page, for display on the page and in the title bar.
 *
 * @param $title
 *   Optional string value to assign to the page title; or if set to NULL
 *   (default), leaves the current title unchanged. The string is treated as plain text,
 *   unless the $input parameter is set to HTML_ALREADY_SANITIZED.
 * @param $input
 *   Optional flag - specifies the encoding of $title. 
 *   Specify HTML_ALREADY_SANITIZED if $title is HTML with any possibly dangerous code
 *   stripped using e.g. filter_xss().
 *   Specify PLAIN_TEXT if $title is plain text, i.e. it is escaped using check_plain() before
 *   being written to the HTML page.
 *
 * @return
 *   The updated title of the current page encoded as HTML.
 */
function drupal_set_title($title = NULL, $input = PLAIN_TEXT) {
pwolanin’s picture

patch in #60 still applies cleanly (with offset). All tests pass: 5408 passes, 0 fails, 0 exceptions

catch’s picture

Status: Needs review » Needs work

No longer applies, but I had another read through and it still looks rtbc to me.

c960657’s picture

Status: Needs review » Needs work

FWIW, 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
31.29 KB

ok, seems like just a whitespace change made the new test class hunk not apply. re-rolled.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Applies 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.

lilou’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
30 KB

Patch partially failed : modules\taxonomy\taxonomy.admin.inc (Cannot apply hunk @@ 7 )

Re-roll against CVS HEAD, and convert all drupal_set_title(check_plain()) to drupal_set_title().

Back to CNR.

pwolanin’s picture

@lilou - thanks for the re-roll.

changing the constants per discussion w/ webchick, sun, and chx.

sun’s picture

I'd say RTBC after running _all_ tests once more.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Ran all tests: 6949 passes, 0 fails, 0 exceptions

pwolanin’s picture

truncated TITLE_ from the constants, move them to common.inc per webchick.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ok.

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?

pwolanin’s picture

Status: Needs work » Fixed
pwolanin’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
1.76 KB

oops - we wanted those constants in bootstrap.inc, not common.inc

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. :)

webchick’s picture

Status: Fixed » Needs work

Hey, 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.

webchick’s picture

mfer 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

Dave Reid’s picture

Need another rollback of common.inc (http://drupal.org/cvs?commit=145866). The constants for this patch are still there.

pwolanin’s picture

rats - 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?

pwolanin’s picture

Also, 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?

pwolanin’s picture

The query throwing the error would sem to be in form.inc in function batch_process()

      // Actually store the batch data and the token generated form the batch id.
      db_query("UPDATE {batch} SET token = '%s', batch = '%s' WHERE bid = %d", drupal_get_token($batch['id']), serialize($batch), $batch['id']);
pwolanin’s picture

Status: Needs work » Needs review
FileSize
27.84 KB
2.7 KB

here's a patch without the test (which also moves the constants to bootstrap.inc from common.inc) and a separate patch with the test.

mfer’s picture

I 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:

[12-Oct-2008 12:28:06] PHP Fatal error:  Uncaught exception 'PDOException' with message 'UPDATE {batch} SET token = ?, batch = ? WHERE bid = ? - 
Array
(
    [0] => 4078d78c39544e6fd499175fdcf614ad
    [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:"Comment in /Users/mfarina/workspace/drupal/includes/database/database.inc on line 365

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.

Dave Reid’s picture

I guess I'm in the case where I can't duplicate this error. Go figure. My tests always run different from everyone else.

mfer’s picture

I 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.

pwolanin’s picture

@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?

mfer’s picture

@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.

Damien Tournoud’s picture

This was a simpletest issue, please see #320374: Simpletest now fails with max_allowed_packet = 1M.

mfer’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
25.52 KB

The 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. Re-Committed. :) Thanks for all the hard-core sleuthing, guys!

mfer’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
3.1 KB

The 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

D'oh. :P Thanks. I didn't have time to run the test suite again, so hopefully this fixes it.

See you guys next week!

mfer’s picture

@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.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.