check_field (Dangerous tags in content) should be revision-aware [#2734619]

Currently the security_review_check_field check just identifies which entity-type/id has "Dangerous tags in content" even while it searches through revisions for these tags. This can make it very difficult to identify and clean content, especially when the problematic tags are in one of many old revisions.

This check should identify which revision ids have the dangerous tags, and whether those are the current revision or an old revision. As well, it should note the date of the revision and name of the revision user as those are what the /node/xxx/revisions page presents for the user to click on when browsing revisions.

I'll submit a patch shortly that addresses this issue.

Comment	File	Size	Author
#10	security_review-check_field_revision_aware-2734619-8.patch	8.52 KB	adamfranco
#7	security_review-check_field_revision_aware-2734619-7.patch	8.24 KB	adamfranco
#6	security_review-check_field_revision_aware-2734619-6.patch	8.24 KB	adamfranco
#4	security_review-check_field_revision_aware-2734619-4.patch	7.86 KB	adamfranco
#2	security_review-check_field_revision_aware-2734619-2.patch	6.55 KB	adamfranco

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

26 May 2016 at 22:01

adamfranco created an issue. See original summary.

Comment #2

adamfranco CreditAttribution: adamfranco commented 26 May 2016 at 22:09

Status:

Active

» Needs review

File	Size
security_review-check_field_revision_aware-2734619-2.patch	6.55 KB

This patch resolves this issue and provides both HTML and drush output.

Comment #3

26 May 2016 at 22:11

Status:

Needs review

» Needs work

The last submitted patch, 2: security_review-check_field_revision_aware-2734619-2.patch, failed testing.

Comment #4

adamfranco CreditAttribution: adamfranco commented 27 May 2016 at 18:22

File	Size
security_review-check_field_revision_aware-2734619-4.patch	7.86 KB

I've now reworked the patch to make the integration with the entity module optional. If entity is enabled, the revision timestamp and author will be printed in the results, if not, it will still list the revision ids, but just not the extra details. This also allows the tests to pass.

With entity enabled:

Dangerous tags were found in submitted content (fields).                                [error]
	The following items potentially have dangerous tags.  
	Javascript found in the body field of 1099, "About us":
		/node/1099/edit 
		Non-current revision 113566 from Mon, 04/05/2010 - 2:17pm by John Doe
	Javascript found in the body field of 1262, "Become a Facebook Fan":
		/node/1262/edit
		Current revision 155882 from Thu, 08/16/2012 - 12:50pm by Jane Smith
		Non-current revision 155879 from Thu, 08/16/2012 - 12:37pm by Jane Smith
		Non-current revision 155880 from Thu, 08/16/2012 - 12:39pm by Jane Smith
		Non-current revision 155881 from Thu, 08/16/2012 - 12:48pm by Jane Smith

Without entity enabled:

Dangerous tags were found in submitted content (fields).                                [error]
	The following items potentially have dangerous tags.  
	Javascript found in the body field of 1099, "About us":
		/node/1099/edit 
		Non-current revision 113566 from unknown by unknown, enable the entity module for details.
	Javascript found in the body field of 1262, "Become a Facebook Fan":
		/node/1262/edit
		Current revision 155882 from unknown by unknown, enable the entity module for details.
		Non-current revision 155879 from unknown by unknown, enable the entity module for details.
		Non-current revision 155880 from unknown by unknown, enable the entity module for details.
		Non-current revision 155881 from unknown by unknown, enable the entity module for details.

Comment #5

adamfranco CreditAttribution: adamfranco commented 27 May 2016 at 18:22

Status:

Needs work

» Needs review

Comment #6

adamfranco CreditAttribution: adamfranco commented 27 May 2016 at 19:20

File	Size
security_review-check_field_revision_aware-2734619-6.patch	8.24 KB

One more version of the patch here, this time with style fixes suggested by Coder-Review and preventing overwrites of the "current_version" flag by followup check to the revision table.

Comment #7

adamfranco CreditAttribution: adamfranco commented 2 June 2016 at 15:35

File	Size
security_review-check_field_revision_aware-2734619-7.patch	8.24 KB

Attached is a very minor tweak to the HTML details to make the revision-id not part of the link, allowing it to be more easily copy-pasted.

Comment #8

2 June 2016 at 15:40

Status:

Needs review

» Needs work

The last submitted patch, 7: security_review-check_field_revision_aware-2734619-7.patch, failed testing.

Comment #9

adamfranco CreditAttribution: adamfranco commented 2 June 2016 at 18:04

Note, current test-failures are unrelated to the content of #7 and are caused by Drupal's addition of an "administer fields" permission. Once security_revew-2741289 is in place, the tests should run fine.

Comment #10

adamfranco CreditAttribution: adamfranco commented 2 June 2016 at 18:03

Status:

Needs work

» Needs review

File	Size
security_review-check_field_revision_aware-2734619-8.patch	8.52 KB

Just in case it's of interest, here is an alternative to #7 that provides a direct "delete" link for non-current revisions. I found this extra feature very useful in my own cleanup of sites with lots of Javascript in old revisions, but it does have the downside of potentially implying that deleting the revision is proper resolution. While deleting may be a good resolution, an alternative is a direct DB query that removes the offending JS and leaves the rest of the revision content.

Either this patch or #6 are hopefully sufficient to resolve this issue and are equivalent aside from the delete-link.

Comment #11

kenahoo CreditAttribution: kenahoo as a volunteer commented 16 November 2017 at 05:33

I'd love it if this patch, or one like it, could be applied. I couldn't figure out why editing content to remove PHP tags wasn't getting the `security_review` checks to pass, and after some sleuthing I discovered it's because old revisions are still in the database.