Skip to content

Sharedaddy: Protect against malformed from names.#6769

Merged
dereksmart merged 2 commits intomasterfrom
sharedaddy/email-from
Mar 28, 2017
Merged

Sharedaddy: Protect against malformed from names.#6769
dereksmart merged 2 commits intomasterfrom
sharedaddy/email-from

Conversation

@georgestephanis
Copy link
Copy Markdown
Contributor

Avoid syntax errors due to unexpected characters in the from name by
base64encoding the from name if any blacklisted characters are used
or it's non-ASCII.

Avoid syntax errors due to unexpected characters in the from name by
base64encoding the from name if any non-whitelisted characters are used
or it's non-ASCII.
@georgestephanis georgestephanis added [Feature] Sharing Post sharing, sharing buttons Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. labels Mar 27, 2017
@jeherve jeherve added this to the 4.8 milestone Mar 27, 2017
@samhotchkiss samhotchkiss requested review from samhotchkiss and removed request for dereksmart March 28, 2017 03:05
@samhotchkiss samhotchkiss added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 28, 2017
rcoll
rcoll previously requested changes Mar 28, 2017
Copy link
Copy Markdown
Member

@rcoll rcoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except the syntax error.

$needs_encoding = preg_match( $name_needs_encoding_regex, $s_name ) || // If it contains any blacklisted chars,
! function_exists( 'mb_convert_encoding' ) || // Or if we can't use `mb_convert_encoding` encode everything,
mb_convert_encoding( $data['name'], 'ASCII' ) !== $s_name // Or if it's not already ASCII
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting the following fatal:

2017/03/28 12:09:48 [error] 14282#0: *231580 FastCGI sent in stderr: "PHP message: PHP Parse error: syntax error, unexpected ')' in /var/www/sometestsite.com/wp-content/plugins/jetpack/modules/sharedaddy/sharedaddy.php on line 38" while reading response header from upstream, client: 73.204.197.29, server: sometestsite.com, request: "GET / HTTP/1.1", upstream: "fastcgi://unix:/var/run/php-fpm/php-fpm.sock:", host: "sometestsite.com"

Looks like we don't need the parenthesis here.

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 28, 2017

I have fixed the syntax error and @georgestephanis if you don't mind I have split the condition into more lines to improve readability. Testing with this snippet:

add_filter( 'sharing_email_can_send', function( $data ) {
	$data['name'] = "Игорь<\r\n";
	return $data;
} );

What's interesting is that even if you add text after CR and LF symbols, the email gets sent anyway, it just comes without a parsed From header because it gets split into several lines.

Anyway, works great!

Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dereksmart dereksmart merged commit 8b58527 into master Mar 28, 2017
@dereksmart dereksmart deleted the sharedaddy/email-from branch March 28, 2017 18:00
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 28, 2017
@georgestephanis
Copy link
Copy Markdown
Contributor Author

@rcoll D'oh! This is what happens when I do a last minute readability change after testing it. Thanks for the catch!

jeherve added a commit that referenced this pull request Mar 28, 2017
samhotchkiss pushed a commit that referenced this pull request Mar 29, 2017
* Readme: remove old release and add skeleton for 4.8.

* Changelog: add #6572

* Changelog: add #6567

* Changelog: add #6542

* Changelog: add #6527

* Changelog: add #6508

* Changelog: add #6478

* Changelog: add #6477

* Changelog: add #6249

* Update stable version and remove old version from readme.

* Changelog: add 4.7.1 to changelog.

* Readme: add new contributor.

* Sync: update docblock @SInCE version.

Related: #6053

* Changelog: add release post.

* changelog: add #6053

* Changelog: add #6413

* Changelog: add #6482

* Changelog: add #6584

* Changelog add #6603

* Changelog: add #6606

* Changelog: add #6611

* Changelog: add #6635

* Changelog: add #6639

* Changelog: add #6684

* Changelog: add #6710

* Changelog: add #6711

* Changelog: add #5461

* Testing list: update Settings UI feedback prompt.

Props @MichaelArestad

* Changelog: add #6789

* Changelog: add #6778

* Changelog: add #6777

* Changelog: add #6775

* Changelog: add #6755

* Changelog: add #6731

* Changelog: add #6721

* Changelog: add #6705

* Changelog: add #6702

* Changelog: add #6671

* Changelog: add #6637

* Changelog: add #6582

* Changelog: add #6566

* Changelog: add #6555

* Changelog: add #6529

* Changelog: add #6344

* Changelog: add #5763

* Changelog: add #5503

* Changelog: update #6637 changelog.

@see 40e115c#commitcomment-21523982

* Changelog: add #6699

* Changelog: add #6632

* Changelog: add #6769

* Changelog: add #6707

* Changelog: add #6590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Sharing Post sharing, sharing buttons [Pri] High

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants