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

⭐ new: HTML locale message warning option #567

Merged
merged 8 commits into from
Apr 26, 2019

Conversation

kazupon
Copy link
Owner

@kazupon kazupon commented Apr 22, 2019

No description provided.


* **Read/Write**

Whether to allow the use locale messages of HTML formatting.
Copy link
Collaborator

@exoego exoego Apr 23, 2019

Choose a reason for hiding this comment

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

🤔 Hmm, I am afirad that I can not understand the feature from this description.

Is this feature intended to detect invalid HTML tags in locale message and raise warn/error?
If so, I suggest to name it like htmlMessageValidationLevel.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you for your reviewing!

This feature is to detect HTML tags from locale messages to provides increased security.
Another alternative, it name like allowHtmlMessageing

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, the feature itself sound very nice.

It looks like off on allow~ means allowing HTML without warning.
It is a bit weired for me, since off is supposed to disable feature and I thought it means "Do Not Allow".

I suggest

  • allow instead of `off,
  • or, name the configuration warnHtmlInMessage

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks!

allow instead of `off,
or, name the configuration warnHtmlInMessage

It sounds good. 👍

@@ -105,6 +114,9 @@ export default class VueI18n {
})
}

_checkLocaleMessage (locale: Locale, level: AllowHtmlFormattingLevel, message: LocaleMessageObject): void {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be helpful if error message suggest users to read Formatting and use component interpolation

@kazupon kazupon marked this pull request as ready for review April 23, 2019 17:00
@codecov-io
Copy link

codecov-io commented Apr 23, 2019

Codecov Report

Merging #567 into dev will increase coverage by 0.13%.
The diff coverage is 98.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #567      +/-   ##
==========================================
+ Coverage   96.25%   96.38%   +0.13%     
==========================================
  Files          10       10              
  Lines         720      775      +55     
==========================================
+ Hits          693      747      +54     
- Misses         27       28       +1
Impacted Files Coverage Δ
src/util.js 98.3% <100%> (+0.09%) ⬆️
src/index.js 98.02% <98.07%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9b9adf...008cb87. Read the comment docs.

decls/i18n.js Outdated
@@ -59,6 +59,8 @@ declare type FormattedNumberPart = {
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/formatToParts#Return_value
declare type NumberFormatToPartsResult = Array<FormattedNumberPart>;

declare type WarnHtmlInMessageLevel = 'allow' | 'warn' | 'error';
Copy link
Collaborator

@exoego exoego Apr 24, 2019

Choose a reason for hiding this comment

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

From this comment, I suggest to change enums like this

WarnHtmlInMessageLevel = 'off' | 'warn' | 'error';

Because allow for Warn~ to disable warnings is very confusing.
off for Warn~ is reasonable enum to disable warning.

Copy link
Owner Author

Choose a reason for hiding this comment

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

sorry for my miss-understanding...

src/index.js Outdated
} else if (typeof message === 'string') {
const ret = htmlTagMatcher.test(message)
if (ret) {
const msg = `Detect unsafe locale message '${message}' of keypath '${stack.join('')}' at '${locale}', suggest use component interpolation with '<i18n>'`
Copy link
Collaborator

@exoego exoego Apr 24, 2019

Choose a reason for hiding this comment

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

I suggest to add followings to give more advidses to users.

  • What is a unsafe part of message: HTML
  • Why it is unsafe: it may cause vulnerability known as XSS (technical term)
  • How to resolve: URL to How to user component interpolation
Suggested change
const msg = `Detect unsafe locale message '${message}' of keypath '${stack.join('')}' at '${locale}', suggest use component interpolation with '<i18n>'`
const msg = `Detected HTML in message '${message}' of keypath '${stack.join('')}' at '${locale}'. Consider component interpolation with '<i18n>' to avoid XSS. See https://kazupon.github.io/vue-i18n/guide/interpolation.html`

URL could be shortend by https://bitly.com/

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks a lot!

@kazupon kazupon changed the title ⭐ new: allow html formatting option ⭐ new: HTML locale message warning option Apr 26, 2019
@kazupon kazupon merged commit 4aecf03 into dev Apr 26, 2019
@kazupon kazupon deleted the feature/strict-locale-messasges branch April 26, 2019 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants