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.

Sign up for GitHub

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

Jump to bottom

src: do not ignore IDNA conversion error #11549

Closed
wants to merge 3 commits into from
Closed

src: do not ignore IDNA conversion error #11549

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6fef08
src: do not ignore IDNA conversion error
TimothyGu Feb 25, 2017
f4b0403
test: more comprehensive IDNA test cases
TimothyGu Feb 25, 2017
d9a8369
doc: document WHATWG IDNA methods' error handling
TimothyGu Feb 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
src: do not ignore IDNA conversion error 
Old behavior can be restored using a special `lenient` mode.
  • Loading branch information
@TimothyGu
TimothyGu committed Feb 28, 2017
commit a6fef08d551520c503a93e5fa714f041ee5eaa6a
5 changes: 4 additions & 1 deletion lib/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,10 @@ Url.prototype.parse = function(url, parseQueryString, slashesDenoteHost) {
// It only converts parts of the domain name that
// have non-ASCII characters, i.e. it doesn't matter if
// you call it with a domain that already is ASCII-only.
this.hostname = toASCII(this.hostname);

// Use lenient mode (`true`) to try to support even non-compliant
// URLs.
this.hostname = toASCII(this.hostname, true);
}

var p = this.port ? ':' + this.port : '';
Expand Down
20 changes: 14 additions & 6 deletions src/node_i18n.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,8 @@ bool InitializeICUDirectory(const std::string& path) {

int32_t ToUnicode(MaybeStackBuffer<char>* buf,
const char* input,
size_t length) {
size_t length,
bool lenient) {
UErrorCode status = U_ZERO_ERROR;
uint32_t options = UIDNA_DEFAULT;
options |= UIDNA_NONTRANSITIONAL_TO_UNICODE;
Expand All @@ -435,7 +436,7 @@ int32_t ToUnicode(MaybeStackBuffer<char>* buf,
&status);
}

if (U_FAILURE(status)) {
if (U_FAILURE(status) || (!lenient && info.errors != 0)) {
len = -1;
buf->SetLength(0);
} else {
Expand All @@ -448,7 +449,8 @@ int32_t ToUnicode(MaybeStackBuffer<char>* buf,

int32_t ToASCII(MaybeStackBuffer<char>* buf,
const char* input,
size_t length) {
size_t length,
bool lenient) {
UErrorCode status = U_ZERO_ERROR;
uint32_t options = UIDNA_DEFAULT;
options |= UIDNA_NONTRANSITIONAL_TO_ASCII;
Expand All @@ -473,7 +475,7 @@ int32_t ToASCII(MaybeStackBuffer<char>* buf,
&status);
}

if (U_FAILURE(status)) {
if (U_FAILURE(status) || (!lenient && info.errors != 0)) {
len = -1;
buf->SetLength(0);
} else {
Expand All @@ -489,8 +491,11 @@ static void ToUnicode(const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value val(env->isolate(), args[0]);
// optional arg
bool lenient = args[1]->BooleanValue(env->context()).FromJust();

MaybeStackBuffer<char> buf;
int32_t len = ToUnicode(&buf, *val, val.length());
int32_t len = ToUnicode(&buf, *val, val.length(), lenient);

if (len < 0) {
return env->ThrowError("Cannot convert name to Unicode");
Copy link
Member

Choose a reason for hiding this comment

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

Is this error part of any non-experimental API? Could we change it to Cannot decode name as Punycode? (basically the same question I also posted below).

Copy link
Member Author

Choose a reason for hiding this comment

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

No; in fact the toUnicode JS function isn't used in the code base at all. Maybe we should just remove this method?

/cc @jasnell

Copy link
Member

Choose a reason for hiding this comment

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

If it's not used, it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Remove which function specifically? The `i18n::ToUnicode' function is definitely used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell, the exposed process.binding('icu').toUnicode() JS function.

Expand All @@ -508,8 +513,11 @@ static void ToASCII(const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value val(env->isolate(), args[0]);
// optional arg
bool lenient = args[1]->BooleanValue(env->context()).FromJust();

MaybeStackBuffer<char> buf;
int32_t len = ToASCII(&buf, *val, val.length());
int32_t len = ToASCII(&buf, *val, val.length(), lenient);

if (len < 0) {
return env->ThrowError("Cannot convert name to ASCII");
Copy link
Member

Choose a reason for hiding this comment

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

Is this error part of any non-experimental API? Could we change it to Cannot encode name to ASCII as Punycode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes for toASCII

> url.parse(`http://${'é'.repeat(230)}.com/`)
Error: Cannot convert name to ASCII

Expand Down
6 changes: 4 additions & 2 deletions src/node_i18n.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ bool InitializeICUDirectory(const std::string& path);

int32_t ToASCII(MaybeStackBuffer<char>* buf,
const char* input,
size_t length);
size_t length,
bool lenient = false);
int32_t ToUnicode(MaybeStackBuffer<char>* buf,
const char* input,
size_t length);
size_t length,
bool lenient = false);

} // namespace i18n
} // namespace node
Expand Down