Closed Bug 321491 Opened 14 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: usenet, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 316444, 316727
Blocks: 316730
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Assignee: darin → usenet
Status: ASSIGNED → NEW
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.
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.
Attached patch Patch v.1Splinter Review
Assignee: usenet → smontagu
Attachment #8667751 - Flags: review?(mcmanus)
Blocks: IDNA2008
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.