Currently, 8.0.x HEAD has seven cases where there are tests to determine if text has been escaped properly. The tests look similar to

$this->assertRaw(String::checkPlain($some_string));

While this works, it isn't the best idea. For one, it forces the developer to program to the implementation rather than the interface. It also violates a testing principle where the assertion should describe what you are testing, and not just generate a pass/fail.

AssertContentTrait should have a method for testing whether a string has been escaped or not in HTML output. Something like

  protected function assertEscaped($raw, $message = '', $group = 'Other') {
    if (!$message) {
      $message = String::format('Escaped "@raw" found', array('@raw' => $raw));
    }
    return $this->assert(strpos($this->getRawContent(), String::checkPlain($raw)) !== FALSE, $message, $group);
  }

should be sufficient.

I'll provide a patch if anyone thinks this is a good idea.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio’s picture

Status: Active » Needs review
FileSize
5.31 KB

Quick proof of concept patch for discussion.

xjm’s picture

Component: phpunit » simpletest.module
Category: Feature request » Task
Issue tags: +SafeMarkup
mpdonadio’s picture

Fixed a few bad copy/paste lines.

Added a assertNoEscaped to parallel the other versions.

Updated all instances of `$this->assertRaw(String::checkPlain($some_string));` in core:

  • BlockTest
  • BreadcrumbTest
  • LinkFieldTest
  • TwigRawTest
  • TextFieldTest
Fabianx’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

RTBC, looks great to me and is a very valuable asset.

This is major, due to improving security by ensuring developers make less mistakes during programming of security sensitive tests.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 010c4df and pushed to 8.0.x. Thanks!

  • alexpott committed 010c4df on 8.0.x
    Issue #2340785 by mpdonadio: Create proper test method for determining...
mpdonadio’s picture

Do I need to make a Change Notice for this?

cilefen’s picture

@mpdonadio That would be good.

Status: Fixed » Closed (fixed)

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

larowlan’s picture

Assigned: Unassigned » larowlan
Issue tags: +CriticalADay

patch coming

larowlan’s picture

Assigned: larowlan » Unassigned
Issue tags: -CriticalADay

meh, wrong issue