Problem/Motivation

Noticing a few places where check_plain() my CSV source names so I did some investigating and turned up a few over escaping.

I've got columns with & in their names and the CSV won't pick up those columns if the CSV doesn't have them as & which is silly.

Proposed resolution

Remove check_plain() on values going into l()

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 l_does_check_plain-2557581-2.patch3.5 KBjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: l() does check_plain automatically, avoid double escaping. » CSV column names escaped & upload field double escaping.
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new l_does_check_plain-2557581-2.patch3.5 KB

This resolves both double escaping in the upload CSV file name and in the source names + makes sure their use in the UI is escaped.

megachriz’s picture

megachriz
Primary language Dutch
commented

Patch looks good and it fixes the unnecessary escaping of column names in the CSV template.

The changes in FeedsCSVParser::sourceForm() look like they only improve readability of the code and not any problems noted in this issue. I checked with using an ampersand in a source name and noted no differences in the UI before and after applying the patch. Am I right or did I overlook something?

You are right that text that is passed through l() doesn't need to be escaped first (unless the option HTML is set to TRUE, which is not the case here).

With and without the patch I noticed no problems when doing an import of a CSV file when an ampersand was used in a column name. Did you notice any issues there or is this issue purely fixing the UI and the CSV template?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Here's a self review of the patch so you can see my intentions behind each line:

No line was changed for readability.

  1. +++ b/feeds.pages.inc
@@ -354,7 +354,7 @@ function theme_feeds_upload($variables) {
-      $summary .= l(check_plain($file->filename), $wrapper->getExternalUrl());
+      $summary .= l($file->filename, $wrapper->getExternalUrl());

    This is because l() already check_plain() the title by default. 

  2. +++ b/plugins/FeedsCSVParser.inc
@@ -124,20 +124,20 @@ class FeedsCSVParser extends FeedsParser {
-        $sources[] = '"' . check_plain($mapping['source']) . '"';
+        $sources[] = '"' . $mapping['source'] . '"';
...
-        $sources[] = check_plain($mapping['source']);
+        $sources[] = $mapping['source'];
...
-        $uniques[] = check_plain($mapping['source']);
+        $uniques[] = $mapping['source'];

@@ -215,16 +215,16 @@ class FeedsCSVParser extends FeedsParser {
-        $uniques[] = check_plain($mapping['source']);
+        $uniques[] = $mapping['source'];
...
-        $sources[] = check_plain($mapping['source']);
+        $sources[] = $mapping['source'];

    These are to ensure we are not storing pre-escaped source mappings. 

  3. +++ b/plugins/FeedsCSVParser.inc
@@ -124,20 +124,20 @@ class FeedsCSVParser extends FeedsParser {
-    $output = t('Import !csv_files with one or more of these columns: !columns.', array('!csv_files' => l(t('CSV files'), 'http://en.wikipedia.org/wiki/Comma-separated_values'), '!columns' => implode(', ', $sources)));
+    $output = t('Import !csv_files with one or more of these columns: @columns.', array('!csv_files' => l(t('CSV files'), 'http://en.wikipedia.org/wiki/Comma-separated_values'), '@columns' => implode(', ', $sources)));
...
-    $items[] = format_plural(count($uniques), 'Column <strong>!columns</strong> is mandatory and considered unique: only one item per !columns value will be created.', 'Columns <strong>!columns</strong> are mandatory and values in these columns are considered unique: only one entry per value in one of these column will be created.', array('!columns' => implode(', ', $uniques)));
+    $items[] = format_plural(count($uniques), 'Column <strong>@columns</strong> is mandatory and considered unique: only one item per @columns value will be created.', 'Columns <strong>@columns</strong> are mandatory and values in these columns are considered unique: only one entry per value in one of these column will be created.', array('@columns' => implode(', ', $uniques)));

    These lines ensure that the columns names get escaped with @ instead of !. So the markup should be the same as before, just the escaping is happening later than earlier. 

  4. +++ b/plugins/FeedsCSVParser.inc
@@ -215,16 +215,16 @@ class FeedsCSVParser extends FeedsParser {
-      if (in_array(check_plain($mapping['source']), $uniques) || in_array(check_plain($mapping['source']), $sources)) {
+      if (in_array($mapping['source'], $uniques) || in_array($mapping['source'], $sources)) {

    This makes sure we are comparing raw data with raw data.

Try this, create field mapping for CSV with & in the source column names. Then try to upload a CSV with headers that have just &. See if the fields actually import or get ignored. For me the ones with & before this patch were ignored, but if I escaped them as &amp; in the CSV (which is gross) then it picked those columns up.

megachriz’s picture

megachriz
Primary language Dutch
commented

Thanks for your self review. I can completely understand changes #1 and #4, but I still feel I might be overlooking something when I look at #2. The variables $sources and $uniques in the FeedsCSVParser::sourceForm() are only passed to t() and format_plural(). I don't see where these variables are stored for use outside of this method. They are not stored on $this and $form, for example.

As per your suggestion I tried again to import a CSV with columns that contain an ampersand in the name or are just only the ampersand.

Without the patch applied, I tried to import the following CSV file:

guid,name&address,&
1,London Town,Silver rain was falling down

This was correctly imported.

And when importing this CSV, the contents of columns that contained &amp; was not imported.

guid,name&amp;address,&amp;
1,London Town,Silver rain was falling down

With the patch applied, there is no difference.

I only see fixes in the UI and the CSV template. What am I missing?

For completeness, I would be okay with changes #2 and #3 if it was just only for readability, but I get the sense from you that there is something more which could be needing an automated test.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

@joelpittet
We talked about this at DrupalCon. We agreed on that #2 and #3 of your self review in post #3 does only cleanup, so there wasn't a "hidden" bug that need an automated test. Committed the patch in post #2.

Status: Fixed » Closed (fixed)

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