Refactor IDN code, to allow integration of new IDN sanity checks

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: usenet, Assigned: smontagu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
The current IDN code has two shortcomings which are preventing me from cleanly integrating the new script-mixing filter and UseSTDASCII enforcement code.

The problems are:
* isOnlySafeChars() is being called within core IDN code, when, since it relates to display issues only, it should instead _only_ be called in one place, within nsStandardURL::UTF8toDisplayIDN -- however, the result of this change will need to be checked carefully
* the DNS resolver code, both sync and async, does not appear to correctly handle the failure of ConvertUTF8ToACE, the failure of which should abort DNS lookups as per RFC, rather than (apparently) letting the lookup continue with the unconverted result 
* it is unclear what happens in nsStandardURL::UTF8toDisplayIDN in the case where ConvertUTF8ToACE fails, as Normalise will therefore fail, causing the code to try to return the result of ConvertUTF8ToACE, which fails, causing  nsStandardURL::UTF8toDisplayIDN itself to fail: what happens next?
* in general, error handling in all of the calling points of ConvertUTF8ToACE needs to be examined
* the logging of failures of ConvertUTF8ToACE should be removed: this should never be a "cannot happen" condition... (consider, for example, overlength IDN labels)

Needless to say, these changes should be made with as little alteration to the rest of the IDN code as possible.
(Reporter)

Updated

13 years ago
Blocks: 316444, 316727
(Reporter)

Updated

13 years ago
Blocks: 316730
Status: NEW → ASSIGNED
(Reporter)

Updated

13 years ago
OS: Linux → All
Hardware: PC → All
(Reporter)

Updated

13 years ago
Assignee: darin → usenet
Status: ASSIGNED → NEW
(Reporter)

Comment 1

13 years ago
Notes:
* isOnlySafeChars is called from exactly two places; 
in nsIDNService::Normalize()
in nsIDNService::decodeACE(). 

It probably shouldn't be in either.

On the other hand, it probably _should_ be called from nsStandardURL::UTF8toDisplayIDN().

* ConvertUTF8ToACE() is called in 7 places; 
in nsStandardURL::UTF8toDisplayIDN() [2 occurrences]
in nsIDNService::Normalize() 
in nsStandardURL::UTF8toDisplayIDN()
in nsIDNService::decodeACE()
in nsDNSService::AsyncResolve()
in nsDNSService::Resolve()
and also within the test program in TestIDN.cpp

All of these calls need to be reviewed for error handling behaviour.
(Reporter)

Comment 2

13 years ago
Further up the call tree: nsStandardURL::UTF8toDisplayIDN() is called in only one place, in nsStandardURL::NormalizeIDN(), which is in turn called:

in nsStandardURL::BuildNormalizedSpec()
in nsStandardURL::SetHost()

Comments:
* If NormalizeIDN() fails in either call case, the host string falls back to remaining as the raw input string. This is... suboptimal.
(Assignee)

Comment 3

3 years ago
Created attachment 8667751 [details] [diff] [review]
Patch v.1
Assignee: usenet → smontagu
Attachment #8667751 - Flags: review?(mcmanus)
(Assignee)

Updated

3 years ago
Blocks: 479520
Comment on attachment 8667751 [details] [diff] [review]
Patch v.1

Review of attachment 8667751 [details] [diff] [review]:
-----------------------------------------------------------------

rs=mcmanus
Attachment #8667751 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/c7933be0b031
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.