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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
15 years ago
2 years ago

People

(Reporter: benc, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog][good first bug][proxy])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

15 years ago
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

13 years ago
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
(Reporter)

Comment 2

11 years ago
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.
Whiteboard: [necko-backlog][good first bug]
Hi, Can I work on this bug?
I've just setup the dev environment for Firefox.
please go ahead

Updated

2 years ago
Whiteboard: [necko-backlog][good first bug] → [necko-backlog][good first bug][proxy]
(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?
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
Assignee: benc → nobody
QA Contact: benc
Created attachment 8848121 [details] [diff] [review]
Add fix for hostnames ending with '.'
Attachment #8848121 - Flags: review?(mcmanus)
Attachment #8848121 - Flags: review?(mcmanus) → review?(daniel)
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 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;
	
}
Attachment #8848121 - Flags: review?(daniel) → review-
Created attachment 8850509 [details]
Amend top commit with bug number when not using mq

MozReview-Commit-ID: JkHVMJkRWWZ
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.
Attachment #8850509 - Attachment is obsolete: true
Attachment #8850509 - Attachment is patch: false
Created attachment 8850519 [details] [diff] [review]
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.
Attachment #8850519 - Flags: review?(daniel)
Attachment #8850519 - Flags: review?(daniel) → review-
Created attachment 8850521 [details] [diff] [review]
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...
Attachment #8850521 - Flags: review?(daniel)
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 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)
Attachment #8850521 - Flags: review?(daniel) → review-
Created attachment 8850553 [details] [diff] [review]
Use simple column search instead of search+substring

Attachment containing patch with squashed commits from previous attachments.
Attachment #8850553 - Flags: review?(daniel)
Attachment #8850519 - Attachment is obsolete: true
Attachment #8850521 - Attachment is obsolete: true
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.
Attachment #8850553 - Flags: review?(daniel) → review-
Created attachment 8850576 [details] [diff] [review]
Add fix for hostnames ending with '.'

Attachment with complete patch (tested).
Attachment #8850576 - Flags: review?(daniel)
Attachment #8848121 - Attachment is obsolete: true
Attachment #8850553 - Attachment is obsolete: true
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.
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.
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 on attachment 8850576 [details] [diff] [review]
Add fix for hostnames ending with '.'

See above comments.
Attachment #8850576 - Flags: feedback-
(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.
(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 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.
Attachment #8850576 - Flags: review?(daniel) → review-
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.