Original report:

Potential problem: FAPI element '#description' only accept filtered text. Hence, check_plain() missing from Line # 85 of modules/aggregator/aggregator.processor.inc

Adding a patch for the same.

----

Problem: No description is shown on the "Default processor settings" fieldset.

Solution: ensure the same description appears in D7 as in D8.

CommentFileSizeAuthor
#13 aggregator before.png147.55 KBstefan.r
#13 aggregator after.png146.84 KBstefan.r
#13 aggregator d8.png168.13 KBstefan.r
#12 aggregator-Fixed_Potential_problem_wrong_hook_hook_aggregator_process_rather_than_hook_aggregator_process_info_is_invoked-2774725-12-7.x.patch1.31 KBsandip27
#6 aggregator-Fixed_Potential_problem_Missing_check_plain_and_corrected_function_call-2774725-6-7.x.patch1.75 KBsandip27
#2 aggregator-Fixed_Potential_problem_Missing_check_plain-2774725-2-7.x.patch1.07 KBsandip27

Comments

sandip27 created an issue. See original summary.

sandip27’s picture

sandip27 commented
StatusFileSize
new aggregator-Fixed_Potential_problem_Missing_check_plain-2774725-2-7.x.patch1.07 KB
sandip27’s picture

sandip27 commented
Status: Active » Needs review
sandip27’s picture

sandip27 commented
Issue summary: View changes
stefan.r’s picture

stefan.r commented

It looks to me like

$info = module_invoke('aggregator', 'aggregator_process', 'info');

was supposed to say:

$info = module_invoke('aggregator', 'aggregator_process_info')

...which returns a simple hardcoded string in $info['description']. No need to check_plain that.

sandip27’s picture

sandip27 commented
StatusFileSize
new aggregator-Fixed_Potential_problem_Missing_check_plain_and_corrected_function_call-2774725-6-7.x.patch1.75 KB

@stefan.r, apparently you were correct, however, it still needs check_plain as per Drupal Coding Standards. Hence, providing a updated patch. Please review.

Thanks

Status: Needs review » Needs work

The last submitted patch, 6: aggregator-Fixed_Potential_problem_Missing_check_plain_and_corrected_function_call-2774725-6-7.x.patch, failed testing.

stefan.r’s picture

stefan.r commented

Hmm where did you get that information (that all #descriptions need a check_plain)?

+++ b/modules/aggregator/aggregator.processor.inc
@@ -72,7 +72,7 @@ function aggregator_aggregator_remove($feed) {
+    $info = module_invoke('aggregator', 'aggregator_process');

aggregator_process_info

sandip27’s picture

sandip27 commented

As per one of helper module "coder" mentioned in here, https://www.drupal.org/coding-standards#helpermod It says, One should either use check_plain(), filter_xss() or similar to ensure the $variable is fully sanitized.

Thanks and appreciate your comments about the same.

David_Rothstein’s picture

David_Rothstein commented
Title: Potential problem: Missing check_plain() from FAPI element '#description'. » Potential problem: Missing check_plain() from FAPI element '#description', and wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

Since this comes from a hook implementation (not from user input) I would think we don't want a check_plain() here since it would risk double-escaping. The example at https://api.drupal.org/api/drupal/modules!aggregator!aggregator.api.php/... shows that the description is translated text, so if there is user input involved it would be the developer's responsibility to sanitize that via normal t()-placeholders (@variable, %variable, etc). I guess it wouldn't hurt putting a note about that in the hook documentation though.

stefan.r’s picture

stefan.r commented
Title: Potential problem: Missing check_plain() from FAPI element '#description', and wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info()) » Potential problem: wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

@sandip in my opinion the coder module is wrong in this particular case -- there is nothing to sanitize, since it's a string wrapped in t() without any user input: t('Creates lightweight records from feed items.'),

sandip27’s picture

sandip27 commented
Status: Needs work » Needs review
StatusFileSize
new aggregator-Fixed_Potential_problem_wrong_hook_hook_aggregator_process_rather_than_hook_aggregator_process_info_is_invoked-2774725-12-7.x.patch1.31 KB

Thank you @David and @Stefan. Please find attached updated patch to fix the call to correct hook.

stefan.r’s picture

stefan.r commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new aggregator d8.png168.13 KB
new aggregator after.png146.84 KB
new aggregator before.png147.55 KB

It's a small UI change, looks OK to me. I guess this has never actually been right in D7, but it has already been fixed in D8.

Screenshots for reference:

Before:

After:

Drupal 8:

stefan.r’s picture

stefan.r commented
Assigned: sandip27 » Unassigned
fabianx’s picture

fabianx commented
Issue tags: +Pending Drupal 7 commit

Looks great to me and is ready for commit.

My only question is:

Was the string translated even though it did not show up before or is this technically a new string?

stefan.r’s picture

stefan.r commented
Title: Potential problem: wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info()) » Wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

  • stefan.r committed 0c770aa on 7.x 
    Issue #2774725 by sandip27: Wrong hook is invoked (...
stefan.r’s picture

stefan.r commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x, thanks!

David_Rothstein’s picture

David_Rothstein commented

Was the string translated even though it did not show up before or is this technically a new string?

It might be a new string on Drupal sites, but it should have been on localize.drupal.org all along I think.

Status: Fixed » Closed (fixed)

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