Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Proxy autodiscovery doesn't check DHCP (option 252)

NEW
Assigned to
(Needinfo from 2 people)

Status

()

Core
Networking: HTTP
11 years ago
6 days ago

People

(Reporter: Fran Boon, Assigned: polly.shaw, NeedInfo)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

proxy autodiscovery can theoretically use either DNS or DHCP.
DNS is working perfectly for me.
DHCP is also working great for IE, but not for current Firefox (1.5.0.7).
Wireshark suggests that it isn't even checking for this feature (it looks for WPAD via DNS & WINS broadcast, but no DHCP INFORM which is what IE uses)

Reproducible: Always

Steps to Reproduce:
1.Configure Network for DHCP auto-proxy detection
2.Configure Firefox to use Auto proxy detection
3.Launch Firefox

Actual Results:  
No access to Internet (if direct access is blocked)

Expected Results:  
Proxy access configured according to PAC file

DHCP is setting the 252 option to:
http://x.x.x.x:8080/wpad.dat

(x.x.x.x is the IP of the Proxy server, port 80 is used by the proxy service, hence using 8080)

Updated

11 years ago
Component: Preferences → Networking: HTTP
Product: Firefox → Core
Version: unspecified → 1.8 Branch

Updated

11 years ago
QA Contact: preferences → networking.http

Comment 1

11 years ago
There is some code in attachment 121707 [details] [diff] [review] (see bug 28998), but it was never checked in.

Updated

10 years ago
Duplicate of this bug: 350759
Duplicate of this bug: 440547
Bug 444071 seems to be similar issue to this bug, and the bug opener of Bug 444071 says following in Bug 444071 Comment #8.
> using "Work offline", immediately followed by "Work online" indeed solves the problem!

To Fran Boon(bug opener): (same question as Bug 444071 Comment #6)
Can following be a workaround when the problem occurs?
  "Work offline", then return to "Work online" again.

Comment 5

9 years ago
Jo: As I recall, the WPAD implementation ignored DHCP.

Should we work on implementing that here? If so, lets make this NEW.

Then we need to find out if the DHCP implementation is OS-specific. If so, then we should make this a meta bug, and then create one bug per-OS implementation.

Fran: can you confirm the DHCP setting is changing (are you using ipconfig /all)?
Summary: Proxy autodiscovery doesn't check DHCP → Proxy autodiscovery doesn't check DHCP (option 252)

Comment 6

9 years ago
Hi,

I have same problem. Can somebody repair it?
In our company is it problem and colleagues must manual setup web proxy server, but after that doesnt take in matter internal address exceptions...

Please look on it!
Thank you so much!

Comment 7

9 years ago
(In reply to comment #6)
> I have same problem. Can somebody repair it?
> In our company is it problem and colleagues must manual setup web proxy server,
> but after that doesnt take in matter internal address exceptions...

The DHCP option will return the location of the PAC-file : your colleagues should use that same PAC file when configuring their proxy-server, in the field 'Automatic proxy configuration URL', *not* the ipaddress of the proxy-server in the manual configuration. The PAC-file also contains those internal address exceptions.

If the PAC-file can be stored at the URL <http://wpad.example.com/wpad.dat> (replacing example.com with the domain of your company), then you can also use the 'Auto-detect proxy settings' field, and it's even easier (it's also supported by Internet Explorer). The DHCP option is mainly used when the PAC-file can't be found at that particular URL, and the user needs some help in the automatic configuration. Unless the whole URL is configured manually ofcourse.

If you do not have a PAC-file at all, then you can't use the 'Automatic' or 'Auto-detect' settings at all, and neither can you use the DHCP option.

Comment 8

9 years ago
Thanks for answer!

I will try it, but it is not so much usable for all peoples... in every location they must change their proxy settings (yeah i know that exist proxy "changers"..)

Is really big problem to add this feature in Firefox?

Comment 9

9 years ago
btw: another problem is with Toolbars (icq, google, yahoo, so on...)

They dont accept configuration scripts or manual configuration...

Comment 10

8 years ago
This is the same problem I have. I can successfully use WPAD and auto detect in IE using DHCP, but not in Firefox. Can this be looked at? Auto proxy should support both methods.

Comment 11

7 years ago
Base on the advice from Jo Hermans comment in above, I have tried this and put in comments in Google Chrome bug's report as it is a similar problem. https://code.google.com/p/chromium/issues/detail?id=18575#c5

Comment 12

7 years ago
At my company we use a multi-site, hierarchical web proxy model primarily for two purposes; first, to route users to a geographically local proxy for performance and bandwidth usage considerations, and second to even the load across our internet-connected web proxy devices. To accomplish this we utilize location-aware load balancing to direct users to a local web server to retrieve a location-specific version of the WPAD configuration file. This is a fairly complex configuration to maintain across >10 widely dispersed locations, and is flawed since the load balancing solution is limited in its ability to determine user locations (it is fairly broad). The proxy devices service both end-user web access and application web access, as well as B2B socks-based traffic. Legacy configuration on application servers make it difficult to reconfigure these servers to segregate user and application traffic on the proxy devices. We would like to use DHCP to replace our DNS-based solution for locating the WPAD configuration file, as all user desktops are configured to use DHCP. DHCP scopes are easily mapped to specific geographic locations which would enable more granular routing and load balancing. Unfortunately we cannot move forward with this solution because of this bug (and the same bug in Chrome). We would greatly appreciate this bug being resolved in a not-too-distant release. Thank you.

Comment 13

6 years ago
after more than 4 years still no answer about future implementation!

would be nice to see the feature to get option 252 into firefox - DNS is sometimes tricky and not possible to server!

Comment 14

6 years ago
It is disappointing to see that there is no update on this issue! I am now force to block Firefox on all office PC (all 100+ of them) due to the fact that we are deploying proxy that is being deployed via DHCP. Sad so sad!

Comment 15

6 years ago
Same here. Have to block 150+ Firefox users because the DNS-way of delivering WPAD is not currently possible in my company and Firefox still can't get wpad via DHCP.

Comment 16

6 years ago
Bumping up the status, which is not a promise that it would be fixed (I don't have anything to say over that). I noticed that Chrome has finally implemented support for this (Windows only !!!). The hard part is to support it on all platforms.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 17

6 years ago
(In reply to fyrch from comment #15)
> Same here. Have to block 150+ Firefox users because the DNS-way of
> delivering WPAD is not currently possible in my company and Firefox still
> can't get wpad via DHCP.

The same for us. We can't use dns way because of company restriction.

Comment 18

6 years ago
Still no progress on this issue ? Resolving this issue is crucial for corporate users.

Updated

6 years ago
Blocks: 720362

Updated

6 years ago
Blocks: 350759

Comment 19

5 years ago
I'm too lazy to file a new bug and I just wanted to mention that after getting the PAC settings in network1, putting the computer in standby then waking it up in a PAC-free network2, firefox insists on using the other networks' proxy (which is unreachable, thus displaying the message that it cannot connect to proxy), even if I explicitly perform a dhcp release/renew. It only resumes working if I restart it.
firefox 12 beta, windows xp sp3

Comment 20

5 years ago
firefox version 11 and 12.0 does not receive Proxy auto-config file by DHCP 252

Comment 21

5 years ago
(In reply to bibl2008 from comment #20)
> firefox version 11 and 12.0 does not receive Proxy auto-config file by DHCP
> 252

What about version 13? Does anyone know if Mozilla is about to fix it?
Using proxy through auto-config file by DHCP is the only reasonable solution for companies using DirectAccess.

Comment 22

5 years ago
I'm using 14 beta and it still doesn't implement DHCP for WPAD.

This is a particular issue for us because our company is split between multiple geographical locations each with its own proxy server, but using a single DNS name for the internal domain. This makes using wpad.domain.com more difficult.

However, we already have the proxy info configured via DHCP for use by IE and Chrome.

If Firefox would support DHCP for proxy auto-detect, it would allow the browser to integrate into existing networks far easier and without having to manually configure each one to point to the .pac file.

Comment 23

5 years ago
Could anyone confirm version 17.0 (ESR) has fixed the lack of support for DHCP option 252?

Comment 24

5 years ago
No patch has been checked in (or even proposed), so how could it have been fixed ?
Equivalent Chromium bug (now fixed, on windows at least) is https://code.google.com/p/chromium/issues/detail?id=18575

Comment 26

5 years ago
(In reply to Daniel Veditz [:dveditz] from comment #25)
> Equivalent Chromium bug (now fixed, on windows at least) is
> https://code.google.com/p/chromium/issues/detail?id=18575

Seems like Firefox is the only important browser left that still won't support this feature. Is there a plan to finally assign this to someone?
This would be crucial to rollout Firefox in corporate networks!

Updated

4 years ago
OS: Windows XP → All
Hardware: x86 → All
Version: 1.8 Branch → Trunk

Comment 27

4 years ago
Any chances that this bug got some attention in near future?

Comment 28

4 years ago
The is a great increase in users demands for this feature inside vpn private networks. DNS Query for WPAD should not be the first inly thing to do for AutoDetect Proxy settings. Sometimes DNS Query for WPAD is not possible, because the pcs are not is a domain, or dns do not know the entry, or simply some routers does not relay the request

DHCP Comming first, before DNS, Firefox should check for option 252, in the dhcp settings, by requesting windows the DhcpRequestParams command. If windows has not requested the option252, a dhcpinform should be submitted to get the option. 

This is really important for trusted dhcp servers in private secured networks.

If of course firefox do not get the option 252 from windows, it should fallback to DNS WPAD METHOD.

But first we would like Firefox to implement correctly the DHCP WPAD METHOD

Thanks

Comment 29

4 years ago
I have to say 'bye-bye' to firefox. I switched the browser of over 300 PCs in my corporate environment to Google Chrome and very satisfied with the outcome. 
I'm tired of the undefined waiting for this bug to be settled.

Comment 30

4 years ago
When this 7 years old bug will be fixed ? I use DHCP WPAD in a school environment with more than 500 computers, I would be glad to have this feature working on Firefox...

Comment 31

4 years ago
we exploit a network of ~1000 computers, the most part from which has firefox. It's really painfull for me, as software administrator, to maintain dns-wpad.
Please, please, fix it BUG and add dhcp-wpad method!

Comment 32

4 years ago
As another large corporation with tens thousands of computers, this is stopping us from implementing much more elegant and efficient solutions.  We are considering de-supporting firefox for our user base because of this...  PLEASE fix, or at least get some traction!  

Thanks.
Flags: needinfo?(nobody)

Comment 33

4 years ago
Firefox is not usable in corporate environment, forget about it and use IE
Flags: needinfo?(nobody)

Comment 34

4 years ago
(In reply to comment #31 and #32)
There is 2 solutions to meet your request : Either you paid an expert to path Firefox or your change the organization of your corporate network. ;) If no company pays to change Firefox, then Firefox will not be able to use in business.
Option 252 is for reserved private use:

http://www.iana.org/assignments/bootp-dhcp-parameters/bootp-dhcp-parameters.xhtml

It's Microsoft choice, http://superuser.com/questions/382964/dhcp-option-252-what-is-it

Unfortunatelly, DHCP hasn't an standard option for indicating de proxy.

DNS discovery must be used if you want to find the proxy:

http://findproxyforurl.com/wpad-introduction/
Whiteboard: [necko-backlog]
(Assignee)

Comment 36

8 months ago
I am thinking of working on this as it would help me out a lot. The link at the bottom of Josep's post now mentions DHCP.
(Assignee)

Comment 37

2 months ago
Created attachment 8867355 [details] [diff] [review]
diff.txt

The approach I have tried follows the pattern of SystemProxySettings - I created an interface called nsIDHCPClient, implemented it for Windows, and injected the class into nsPACMan. I've used the PAC Thread for querying DHCP.

I need guidance on testing this because I haven't written any automated tests.
Attachment #8867355 - Flags: feedback?(zxcvbnm)
(Assignee)

Updated

2 months ago
Attachment #8867355 - Flags: feedback?(zxcvbnm) → feedback?(daniel)

Updated

2 months ago
Attachment #8867355 - Flags: feedback?(mcmanus)
(Assignee)

Comment 38

2 months ago
Created attachment 8867481 [details] [diff] [review]
diff.txt

I updated the patch because the previous one was in the wrong direction (i.e. how to undo my changes to make it the same as trunk.)
Assignee: nobody → polly.shaw
Attachment #8867355 - Attachment is obsolete: true
Attachment #8867355 - Flags: feedback?(mcmanus)
Attachment #8867355 - Flags: feedback?(daniel)
(Assignee)

Updated

2 months ago
Attachment #8867481 - Flags: feedback?(mcmanus)
Attachment #8867481 - Flags: feedback?(daniel)
Comment on attachment 8867481 [details] [diff] [review]
diff.txt

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

Thanks for your contribution and work on this patch!

It seems to be the right direction and is certainly a very good start, but I'll admit I got a bit distracted when I noticed it had a fairly large amount of white space changes that should be avoided and would make the patch smaller and easier to review if they wouldn't be there. The other main concern I have is the memory copies done at a few places without properly checking that the data fits in the destination buffer.

::: netwerk/base/nsIDHCPClient.idl
@@ +10,5 @@
> + */
> +[scriptable, uuid(aee75dc0-be1a-46b9-9e0c-31a6899c175c)]
> +interface nsIDHCPClient : nsISupports
> +{    
> +    

Please avoid adding trailing white space.

::: netwerk/base/nsPACMan.cpp
@@ +223,5 @@
>      mSetupPACData.Assign(text, datalen);
>      mSetupPACURI = pacURI;
>    }
>  
> +  void ConfigureWPAD(){

Nit: the placement of the open brace when declaring a function/method should be in the first column. This mistake occurs on several places.

@@ +423,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +nsPACMan::LoadPACFromURI(const nsCString &spec, bool forceReload, bool calledFromScheduledReload)

"called from" ? Isn't the option simply "scheduled reload" ?

What's the forceReload option for and how do these two differ from each other?

@@ +466,5 @@
> +nsresult
> +nsPACMan::GetPACFromDHCP(nsCString &pacSpec)
> +{
> +    if (mDHCPClient){
> +        return mDHCPClient->GetOption(252, pacSpec);

252 like this is very magic. I'd prefer a define/const or a very clear comment explaining what this value comes from and means.

@@ +480,5 @@
> +    GetPACFromDHCP(mPACThreadURISpec);
> +
> +    if (mPACThreadURISpec.IsEmpty()) {
> +        mPACThreadURISpec.AssignLiteral(WPAD_URL);
> +    }

I can see how a LOG() could be great here for future debugging and understanding users' situations, probably for both branches in this conditional.

@@ +486,5 @@
> +}
> +
> +void nsPACMan::CopyPACURIFromPACThread(){
> +    MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
> +    mPACURISpec.Assign(mPACThreadURISpec);

Is this a thread-safe manipulation?

@@ +768,5 @@
>      // Even if the PAC file could not be parsed, we did succeed in loading the
>      // data for it.
>      mLoadFailureCount = 0;
>    } else {
> +    

trailing white space

::: netwerk/base/nsProtocolProxyService.cpp
@@ +1026,4 @@
>      nsresult rv;
>      if (mSystemProxySettings &&
> +        NS_SUCCEEDED(mSystemProxySettings->GetMainThreadOnly(&mainThreadOnlyForSystemProxySettings)) &&
> +        !mainThreadOnlyForSystemProxySettings) {

This name change seems unrelated, and I think the new much longer name makes the code harder to read. If you think we need to explain that this section is only for system proxy settings, then add a comment.

::: netwerk/base/nsProtocolProxyService.h
@@ +300,5 @@
>      nsresult SetupPACThread();
>      nsresult ResetPACThread();
>      nsresult ReloadNetworkPAC();
>  
> +

white space only change!

::: toolkit/system/windowsDHCPClient/DHCPUtils.cpp
@@ +6,5 @@
> +#include <memory>  
> +#include "mozilla\Logging.h"
> +
> +
> +#define WORKING_BUFFER_SIZE 15000

Perhaps a mention what buffer this is for?

@@ +7,5 @@
> +#include "mozilla\Logging.h"
> +
> +
> +#define WORKING_BUFFER_SIZE 15000
> +#define MAX_TRIES 3

Perhaps a mention what max tries is for?

@@ +14,5 @@
> +#pragma comment(lib, "dhcpcsvc.lib" )
> +
> +mozilla::LazyLogModule gDhcpLog("dhcp");
> +
> +bool IsCurrentAndHasDHCP(PIP_ADAPTER_ADDRESSES pAddresses) {

nit: wrong open brace placements for function declarations in this file too. You should also put the return type on the line above the name. This goes for several other function declarations.

@@ +38,5 @@
> +
> +  PIP_ADAPTER_ADDRESSES pCurrAddresses = nullptr;
> +  
> +
> +  memset(networkAdapterName, 0, NETWORK_ADAPTER_NAME_LENGTH + 1);

Why memset the whole buffer? Isn't it enough to zero the first byte?

@@ +63,5 @@
> +    }
> +
> +    iterations++;
> +
> +  } while ((dwRetVal == ERROR_BUFFER_OVERFLOW) && (iterations < MAX_TRIES));

Can this reasonably ever loop more than one extra lap? Why is MAX_TRIES 3?

Also, the check for "dwRetVal == ERROR_BUFFER_OVERFLOW" seems superfluous in the while() expression since that will always match due to the check above.

@@ +72,5 @@
> +        bool found = false;
> +        pCurrAddresses = pAddresses;
> +        while (pCurrAddresses) {        
> +          if (IsCurrentAndHasDHCP(pCurrAddresses)) {
> +            memcpy(networkAdapterName, pCurrAddresses->AdapterName, strlen(pCurrAddresses->AdapterName) + 1);

scary memcpy. Can this be done in a way that makes it more certain that the destination has room for the copied data? I suggested the method also accepts a target name buffer size and then add a run-time check that the data fits.

@@ +103,5 @@
> +    uint8_t option
> +)
> +{
> +  DWORD dwRetVal, dwSize;
> +  CHAR TmpBuffer[100]; // option result is limted to 256 characters; not sure how much else is needed for

100 or 256. Which is it? Is it supposed to be 0x100 ?

@@ +137,5 @@
> +  );
> +
> +  switch (dwRetVal){
> +    case NO_ERROR:
> +      if (DhcpRequestedOptionParams.nBytesData==0)

nit: add spaces around '=='

@@ +138,5 @@
> +
> +  switch (dwRetVal){
> +    case NO_ERROR:
> +      if (DhcpRequestedOptionParams.nBytesData==0)
> +        return NS_ERROR_NOT_AVAILABLE;

place add { braces }

@@ +143,5 @@
> +
> +      CopyMemory(
> +        pszOptionValueBuf, DhcpRequestedOptionParams.Data,
> +        DhcpRequestedOptionParams.nBytesData
> +      );

Why the funny indentation here? Is there a reason this uses CopyMemory() while you used memcpy() before?

There's no precaution done that the copied data actually fits in the provided target buffer. I propose you pass in a buffer size as well for this purpose.

::: toolkit/system/windowsDHCPClient/DHCPUtils.h
@@ +9,5 @@
> +nsresult GetActiveDHCPNetworkAdapterName(char* networkAdapterName);
> +
> +nsresult RetrieveOption(
> +    LPWSTR   pszAdapterName,
> +    char    pszHostNameBuf[257],

Please explain the chosen maximum size in a comment.

@@ +10,5 @@
> +
> +nsresult RetrieveOption(
> +    LPWSTR   pszAdapterName,
> +    char    pszHostNameBuf[257],
> +  uint8_t option

nit: wrong indentation

::: toolkit/system/windowsDHCPClient/nsWindowsDHCPClient.cpp
@@ +48,5 @@
> +  
> +  char optionValue[MAX_OPTION_LENGTH + 1];
> +  size_t retVal; 
> +  wchar_t wideNetworkAdapterName[(NETWORK_ADAPTER_NAME_LENGTH + 1) * 2];
> +  mbstowcs_s(&retVal, wideNetworkAdapterName, strlen(networkAdapterName) * 2, networkAdapterName, strlen(networkAdapterName));

Please make precautions that this can't overflow the target buffer.

::: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp
@@ +23,5 @@
>  class nsWindowsSystemProxySettings final : public nsISystemProxySettings
>  {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSISYSTEMPROXYSETTINGS

Pretty please do not do (white space) changes to unrelated code!

@@ +75,5 @@
> +    InternetGetConnectedStateExW(&connFlags, connName,
> +                                     mozilla::ArrayLength(connName), 0);
> +  } MOZ_SEH_EXCEPT(EXCEPTION_EXECUTE_HANDLER) {
> +      return NS_ERROR_FAILURE;
> +  }

The indent levels in this code turned funny. They are 4 spaces since before for each level so please keep to that! Also, this is more unrelated white space changes that make this patch larger and harder to review.
Attachment #8867481 - Flags: feedback?(daniel) → feedback-
Comment on attachment 8867481 [details] [diff] [review]
diff.txt

this is daniel's area of expertise, so I'm just going to defer..
Attachment #8867481 - Flags: feedback?(mcmanus)
(Assignee)

Comment 41

13 days ago
Created attachment 8884649 [details] [diff] [review]
diff.txt

Hi Badger - I've addressed your concerns from the last review (I think), made an effort with complying with the coding guide, and written some gtests. Also I put braces round the single-line conditionals & whiles in nsProtocolProxyService.cpp but if this makes reviewing more difficult I can submit a patch without these changes. (There are also some logic changes in this class, so I can see that it might obscure them.)
Attachment #8867481 - Attachment is obsolete: true
Attachment #8884649 - Flags: review?(daniel)
(Assignee)

Comment 42

13 days ago
Created attachment 8884670 [details] [diff] [review]
diff.txt

On reviewing the patch in the browser, I noticed that I had removed a couple of lines from nsProtocolProxyService.cpp which I shouldn't, also that I needed to move a comment from this class to nsPACMan.

also sorry for calling you badger rather than bagder in the last comment.
Attachment #8884649 - Attachment is obsolete: true
Attachment #8884649 - Flags: review?(daniel)
Attachment #8884670 - Flags: review?(daniel)
(Assignee)

Comment 43

13 days ago
Created attachment 8884679 [details] [diff] [review]
diff.txt

fixed error in reload conditional in nsProtocolProxyService. Also removed a local fix for a problem in windows.configure that was causing the build to fail on my machine (I haven't looked into this deeply enough to work out whether this is likely to be affecting anyone else.)
Attachment #8884670 - Attachment is obsolete: true
Attachment #8884670 - Flags: review?(daniel)
Attachment #8884679 - Flags: review?(daniel)
(Assignee)

Comment 44

11 days ago
review ping?
Flags: needinfo?(daniel)
Flags: needinfo?(Daniel.Stenberg)
(Assignee)

Comment 45

10 days ago
review ping? Sorry to keep on needling but I wonder if Daniel is away, so I am asking Patrick.
(Assignee)

Comment 46

10 days ago
review ping? Sorry to keep on needling but I wonder if Daniel is away, so I am asking Patrick.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 47

7 days ago
Created attachment 8886823 [details] [diff] [review]
diff.txt

Updated to use std::vector for char arrays rather than manually allocating memory.
Attachment #8884679 - Attachment is obsolete: true
Attachment #8884679 - Flags: review?(daniel)
Attachment #8886823 - Flags: review?(daniel)
polly, if this were a short patch I would try and review it myself.. but there's quite a bit of pac specific code in there and daniel is maintaining that right now.. so I think its in the best interest of firefox to wait for him to return

thanks for the patch and your patience
Flags: needinfo?(mcmanus)
You need to log in before you can comment on or make changes to this bug.