242525 - isPlainHostName() fails if host is FQDN ("localhost.")

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[benc]

263 "function isPlainHostName(host) {\n" +
264 "    return (host.search('\\\\.') == -1);\n" +
265 "}\n" +

I suspect all the domain name parsing functions of PAC have this problem. If the
host is an FQDN, like "localhost." (vs. "localhost"), then the single "." makes
the hostname look like it actually has more than two labels.

One solution might be to say: if you find a ".", but it is the last character,
that is okay.

The other might be to append a trailing dot at the top of the function, then
re-write all functions to assume they are processing properly terminated FQDN's.

Comment 1

[d]

At http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html#isPlainHostName , the function isPlainHostName is described as "True iff there is no domain name in the hostname (no dots)."

I can only see this being a problem with 'localhost'. If you had a machine named 'bill', referring to it as 'bill.' would be incorrect.

localhost info: http://www.dns.net/dnsrd/rfc/rfc1912.html#4.1%20Boot%20file

d

Comment 2

[benc]

d.

In the general case, that might be true, because using a hostname as an FQDN happens sometimes because the meaning of the hostname is inherently ambiguous.

However, for the specific case, "localhost." is a valid FQDN.

Comment 3

[George Veneel Dogga [::TheLayman]]

Hi, Can I work on this bug?
I've just setup the dev environment for Firefox.

Comment 4

[Patrick McManus [:mcmanus]]

please go ahead

Comment 5

[Alex Shatberashvili]

(In reply to George Veneel Dogga [::TheLayman] from comment #3)
> Hi, Can I work on this bug?
> I've just setup the dev environment for Firefox.

Hi, George. Are you still working on that bug?

Comment 6

[Swapnesh Kumar Sahoo [:swapneshks]]

Hi
I'd like to work on this bug.
I am a beginner and have the firefox dev env built.
I am currently looking at the function in netwerk/base/ProxyAutoConfig.cpp

Comment 7

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Add fix for hostnames ending with '.'

Comment 8

[Daniel Stenberg [:bagder]]

This actually make our function even less similar in functionality to Chrome's and that could be a concern.

Chrome doesn't seem to accept a dot in there, and makes sure the 'host' is not a numerical IPv6-address (as it doesn't use any dots) see https://chromium.googlesource.com/chromium/src/net/+/master/proxy/proxy_resolver_v8.cc#347 (C++ code)

Comment 9

[Daniel Stenberg [:bagder]]

Comment on attachment 8848121 [details] [diff] [review]
Add fix for hostnames ending with '.'

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

::: netwerk/base/ProxyAutoConfig.cpp
@@ +66,5 @@
>    ""
>    "function isPlainHostName(host) {\n"
> +  "    if (host.search('\\\\.') != -1) {\n"
> +  "        var str = host.substring(0, host.length - 1);\n"
> +  "        if (str.search('\\\\.') == -1) {\n"

I think doing search + substring + search again is a bit on the heavy side and an unnecessary amount of processing. Wouldn't a simpler and faster implementation rather just use the found column from the first search to check if it was last in the host name string?

Something like this (untested) snippet:

var col = host.search('\\\\.');

if (col != -1) {
    if (col+1 == host.length) {
        return true;
    } else {
        return false;
    }
} else {
    return true;
	
}

Comment 10

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Amend top commit with bug number when not using mq

MozReview-Commit-ID: JkHVMJkRWWZ

Comment 11

[Swapnesh Kumar Sahoo [:swapneshks]]

Comment on attachment 8850509 [details]
Amend top commit with bug number when not using mq

Please ignore this attachment. Submitted the wrong patch through bzexport.

Comment 12

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Use simple column search instead of using search+substring

I totally missed out the fact that the search method returns the index of the first occurence. This attachment contains the updated (and tested) code.

Comment 13

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove unnecessary substring method

I am really sorry and apologize for the confusion caused. As I mentioned earlier, I am a beginner and still catching up to things...

Comment 14

[Daniel Stenberg [:bagder]]

No worries, we've all been beginners. You'll get there.

Now you only submitted the final correction as a patch for me to review. Can you squash your two last commits and submit them as a single patch so that I can review the entire change as a single commit?

Comment 15

[Daniel Stenberg [:bagder]]

Comment on attachment 8850521 [details] [diff] [review]
Remove unnecessary substring method

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

(r- this, waiting for the full patch to get resubmitted)

Comment 16

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Use simple column search instead of search+substring

Attachment containing patch with squashed commits from previous attachments.

Comment 17

[Daniel Stenberg [:bagder]]

Comment on attachment 8850553 [details] [diff] [review]
Use simple column search instead of search+substring

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

This is still not the complete patch but a follow-up patch that changes your original suggestion. I'd like the full stand-alone patch in a single commit to review+ (I've seen the change now so I know it is correct and I approve of it) and then run through a try-run to verify it doesn't introduce any side-effects.

Comment 18

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Add fix for hostnames ending with '.'

Attachment with complete patch (tested).

Comment 19

[Masatoshi Kimura [:emk]]

This patch blanket permitting trailing dots instead of special casing "localhost." I don't think it is OK (see comment #1).
For example, the patched function will mis-identify "to." as a plain host name.

Comment 20

[Masatoshi Kimura [:emk]]

And honestly, I think it is too late to change the PAC function behavior (this bug was reported 13 years ago). I don't think we should change it unless other browser vendors agree the change.

Comment 21

[Masatoshi Kimura [:emk]]

I even think this bug is not valid. It is natual that isPlainHostName() returns false if host is FQDN by definition because FQDN is not a plain host name.

Comment 22

[Masatoshi Kimura [:emk]]

Comment on attachment 8850576 [details] [diff] [review]
Add fix for hostnames ending with '.'

See above comments.

Comment 23

[Daniel Stenberg [:bagder]]

(In reply to Masatoshi Kimura [:emk] from comment #19)
> This patch blanket permitting trailing dots instead of special casing
> "localhost." I don't think it is OK (see comment #1).
> For example, the patched function will mis-identify "to." as a plain host
> name.

Oh yes, very good point. I got too focused on the actual code I forgot that detail. If we should do something, we probably should only pass exactly "localhost." through... but yes, unless other browsers do this we should probably rather close this as spec-adherent.

Comment 24

[Swapnesh Kumar Sahoo [:swapneshks]]

(In reply to Masatoshi Kimura [:emk] from comment #19)
> This patch blanket permitting trailing dots instead of special casing
> "localhost." I don't think it is OK (see comment #1).
> For example, the patched function will mis-identify "to." as a plain host
> name.

That's right.. I accidentally totally forgot about the condition. Nonetheless.. judging by the above comments, I guess I should stall my work on this bug until the behavior described in this bug is desired.

Comment 25

[Daniel Stenberg [:bagder]]

Comment on attachment 8850576 [details] [diff] [review]
Add fix for hostnames ending with '.'

Given this some time and thoughts, I'm now weighing to favor PAC compatibility and stability. I don't see for example Chrome supporting a trailing dot for localhost, so introducing this support now in Firefox without a stronger motivation seems to just ask for compatibility issues going forward.

Comment 26

[Daniel Stenberg [:bagder]]

A trailing dot for localhost in a PAC is not universally accepted. Let's not do this unless it becomes universally agreed to be a good idea.