Page MenuHomePhabricator

Fix character escaping throughout the extension files
Closed, ResolvedPublic

Description

Review of 631882e3779 of Newsletter extension (Jan 29, 2017)

  • "NewsletterTablePager.php" line 96 - Run htmlspecialchars after you truncate, not before. In this particular case, the worst that could happen would be for an entity reference to get cut off in the middle, but nonetheless escaping should always be the last thing you do.
  • NewsletterDiffEngine - The messages for the h4 headers are not escaped.
  • "SpecialNewsletter.php" line 142 - newsletter-subtitlelinks-foo message is not escaped when it is the active link.
  • "SpecialNewsletter.php" line 207 - Double escaping newsletter-do-unsubscribe.
  • "SpecialNewsletter.php" line 220 - Double escaping newsletter-do-subscribe.
  • "NewsletterContent.php" line 253 - Double escaping in the OOUI button labels (5 instances)
  • "NewsletterTablePager.php" line 33 - getFieldNames() has double escaping.

Details

Related Gerrit Patches:
mediawiki/extensions/Newsletter : masterRemoving double escaping from getFieldNames()
mediawiki/extensions/Newsletter : masterRemoving double escaping of OOUI labels
mediawiki/extensions/Newsletter : masterRemoving double escaping of (un)subscribe message
mediawiki/extensions/Newsletter : masterEscaping newsletter-subtitlelinks-foo message
mediawiki/extensions/Newsletter : masterh4 messages not escaped in NewsletterDiffEngine
mediawiki/extensions/Newsletter : masterEscaping newsletter-subtitlelinks-foo message
mediawiki/extensions/Newsletter : masterConverting $values to HTML entity after truncating

Event Timeline

i want to try to fix them as its my first time contribution how to get source code for this help me getting it here tony

i want to try to fix them as its my first time contribution how to get source code for this help me getting it here tony

Welcome @mvmohitverma54! Please see https://www.mediawiki.org/wiki/How_to_become_a_MediaWiki_hacker for general questions (like how to get the code) which are not specifically related to the topic of this task. (And using punctuation is also welcome - makes things easier to read! :) ) Thanks a lot! Looking forward to your patch!

Change 339963 had a related patch set uploaded (by D3r1ck01):
Converting $values to HTML entity after truncating

https://gerrit.wikimedia.org/r/339963

Change 339963 merged by jenkins-bot:
Converting $values to HTML entity after truncating

https://gerrit.wikimedia.org/r/339963

D3r1ck01 updated the task description. (Show Details)Feb 26 2017, 8:27 PM

Change 339967 had a related patch set uploaded (by D3r1ck01):
Escaping newsletter-subtitlelinks-foo message

https://gerrit.wikimedia.org/r/339967

Change 339967 abandoned by D3r1ck01:
Escaping newsletter-subtitlelinks-foo message

Reason:
Will submit a different PS instead.

https://gerrit.wikimedia.org/r/339967

Change 340054 had a related patch set uploaded (by D3r1ck01):
h4 messages not escaped in NewsletterDiffEngine

https://gerrit.wikimedia.org/r/340054

D3r1ck01 claimed this task.Feb 28 2017, 1:39 PM

Taking this one but anyone is free to still do any task in the list if its undone since its a many-in-one task.

@Bawolff : I was just wondering why we would need ->escape() here

$output .= Html::element( 'h4', [],
					$this->msg( 'newsletter-diff-descheader' )->text() );

as its just an extension message - without any extra params. Probably I am missing something ?

Change 340314 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
Escaping newsletter-subtitlelinks-foo message

https://gerrit.wikimedia.org/r/340314

D3r1ck01 updated the task description. (Show Details)Feb 28 2017, 2:47 PM
D3r1ck01 updated the task description. (Show Details)
D3r1ck01 updated the task description. (Show Details)

@Bawolff, I don't think 'default' => $this->msg( 'newsletter-do-unsubscribe' )->escaped(), is double escaped and I have tested and I am sure. As per this task: "SpecialNewsletter.php" line 207 - Double escaping newsletter-do-unsubscribe. Have a second look and let me know :)

@Bawolff, I don't think 'default' => $this->msg( 'newsletter-do-unsubscribe' )->escaped(), is double escaped and I have tested and I am sure. As per this task: "SpecialNewsletter.php" line 207 - Double escaping newsletter-do-unsubscribe. Have a second look and let me know :)

For this case, my assumption is that HTMLForm::factory() might be doing the magic of escaping.

@Bawolff, I don't think 'default' => $this->msg( 'newsletter-do-unsubscribe' )->escaped(), is double escaped and I have tested and I am sure. As per this task: "SpecialNewsletter.php" line 207 - Double escaping newsletter-do-unsubscribe. Have a second look and let me know :)

For this case, my assumption is that HTMLForm::factory() might be doing the magic of escaping.

@01tonythomas, when I remove escaped() and replace with text(), its no longer escaped but when escaped() is allowed, the string is actually escaped. So what magic does HTMLForm::factory() do that doesn't affect text() or are my missing something?

Sorry folks I think I made a mistake here. The h4 does indeed appear to already be properly escaped (since Html::element escapes things).

Change 340054 abandoned by D3r1ck01:
h4 messages not escaped in NewsletterDiffEngine

Reason:
Abandoning PS since its no longer needed.

https://gerrit.wikimedia.org/r/340054

D3r1ck01 updated the task description. (Show Details)Feb 28 2017, 11:21 PM

Sorry folks I think I made a mistake here. The h4 does indeed appear to already be properly escaped (since Html::element escapes things).

Ok @Bawolff. I just marked it as done. Thanks for your feedback :)

D3r1ck01 updated the task description. (Show Details)Feb 28 2017, 11:47 PM
D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 8:19 PM

Change 340314 merged by jenkins-bot:
Escaping newsletter-subtitlelinks-foo message

https://gerrit.wikimedia.org/r/340314

Change 340566 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/Newsletter] Removing double escaping of (un)subscribe message

https://gerrit.wikimedia.org/r/340566

D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 8:56 PM
D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 9:15 PM

Change 340566 merged by D3r1ck01:
[mediawiki/extensions/Newsletter] Removing double escaping of (un)subscribe message

https://gerrit.wikimedia.org/r/340566

Change 340604 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/Newsletter] Double escaping of OOUI labels

https://gerrit.wikimedia.org/r/340604

D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 9:31 PM

Change 340604 merged by D3r1ck01:
[mediawiki/extensions/Newsletter] Removing double escaping of OOUI labels

https://gerrit.wikimedia.org/r/340604

D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 9:56 PM

Change 340650 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/Newsletter] Removing double escaping from getFieldNames()

https://gerrit.wikimedia.org/r/340650

D3r1ck01 updated the task description. (Show Details)Mar 1 2017, 10:22 PM
Bawolff closed this task as Resolved.Mar 1 2017, 10:23 PM

Looks good.

Change 340650 merged by D3r1ck01:
[mediawiki/extensions/Newsletter] Removing double escaping from getFieldNames()

https://gerrit.wikimedia.org/r/340650

Change 340801 had a related patch set uploaded (by Paladox):
[operations/puppet] Gerrit: Fix bot so that it checks against *-name and *-username

https://gerrit.wikimedia.org/r/340801

Qgil awarded a token.Mar 13 2017, 4:54 PM