Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error message from api request. #15657

Merged
merged 10 commits into from Jun 28, 2019

Add comment

  • Loading branch information...
spacedmonkey committed Jun 25, 2019
commit b4e7268be45e7e7b4e151ac630fec0fc5cefa320
@@ -103,8 +103,9 @@ export function getNotificationArgumentsForSaveFail( data ) {
messages[ edits.status ] :
__( 'Updating failed.' );

// Check if message string contains html tags. It is unsafe to inject unfiltered html here.
This conversation was marked as resolved by spacedmonkey

This comment has been minimized.

Copy link
@aduth

aduth Jun 26, 2019

Member

Technically it's not a matter of safety, at least not without other changes to inject the HTML as innerHTML. Even if there were HTML included in the notice message, it would be written as plaintext (for example, "Publishing failed. Error message: <strong>Bad things</strong> happened."). It's still not ideal from a user's perspective to have shown (at least not without stripping the tags), but not a security concern.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jun 27, 2019

Author Contributor

Do you want me to change the comment or strip the tags?

This comment has been minimized.

Copy link
@aduth

aduth Jun 27, 2019

Member

Do you want me to change the comment or strip the tags?

Per your comment at #15657 (comment) (which I tend to agree with), stripping the tags might yield some bizarre results (at least for specific types of elements; em, strong, and others may be fine).

Changing the comment to clarify this could improve. Maybe:

Suggested change
// Check if message string contains html tags. It is unsafe to inject unfiltered html here.
// Check if message string contains HTML. Notice text is currently only
// supported as plaintext, and stripping the tags may muddle the meaning.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jun 28, 2019

Author Contributor

Merge your change. 👍

if ( ! ( /<\/?[^>]*>/.test( error.message ) ) ) {

This comment has been minimized.

Copy link
@aduth

aduth May 15, 2019

Member

Is the idea with this to exclude messages with HTML? I think it may be agreeable as a first-pass improvement, but is there a fundamental reason we should want to exclude messages containing HTML?

Stripping the HTML tags seems reasonable to me, though the specifics of doing so are some combination of challenging and risky (example solution), depending if we consider the REST response to be "safe".

It might also help to provide some context to the future maintainer what's actually happening here, either with an inline code comment or extracting as a separate, helpfully-named function (e.g. stripTags), in scope or proposed as part of a relevant package (closest might be escape-html).

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey May 17, 2019

Author Contributor

There reason we don't want html tags here, as some responses may contain hyperlinks or bullet points. The content may not make sense anymore if it says for example "Click on this link" and it is no longer a link.

I didn't make the tag check into a function, as I didn't seem like it would be useful to do so. But I can, if you so wish.

This comment has been minimized.

Copy link
@aduth

aduth May 21, 2019

Member

There reason we don't want html tags here, as some responses may contain hyperlinks or bullet points. The content may not make sense anymore if it says for example "Click on this link" and it is no longer a link.

I think it's okay to be conservative here as you propose, as long as there's the fallback messaging of "Publishing failed".

I didn't make the tag check into a function, as I didn't seem like it would be useful to do so. But I can, if you so wish.

My concern here is whether it will be obvious here to a future maintainer what this logic is intended to do. Whether that's an inline code comment or function, I have no strong preference. But as implemented currently, I foresee it being not immediately obvious to its purpose.

This comment has been minimized.

Copy link
@aduth

aduth Jun 12, 2019

Member

My concern here is whether it will be obvious here to a future maintainer what this logic is intended to do. Whether that's an inline code comment or function, I have no strong preference. But as implemented currently, I foresee it being not immediately obvious to its purpose.

Can we add a code comment here?

noticeMessage = sprintf( '%1$s %2$s', noticeMessage, error.message );
noticeMessage = sprintf( __( '%1$s Error message: %2$s' ), noticeMessage, error.message );

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 1, 2019

Member

This is missing a translator comment to explain the parameters.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jul 1, 2019

Author Contributor

Should this be another PR?

This comment has been minimized.

Copy link
@swissspidy
}
return [ noticeMessage, { id: SAVE_POST_NOTICE_ID } ];
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.