139594 - PAC2: "NULL" directive

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[benc]

There are some uses (especially for ad blocking) to have PAC decide to throw
away a URL request (sort of like the IMAP or FTP NOOP). 

I think the directive "NULL" would be ideal. Right now people do things like
PROXY to localhost on a dead port.

I've strongly objected to people doing things like hacking /etc/hosts on their
systems as a form of ad blocking, so I think any way of allowing people to get
what they want without potentially hacking the proverbial nose off their system
is a good idea. (These two mechanisms are not the same, but I'm just connecting
a general problem with a general solution).

Comment 1

[Nathan Kurz]

This seems like a good idea, so I dove into the code to figure it out.

There are two levels of enhancement one could add here.  First, could add
support for a PAC directive that would cause behaviour very similar to the
current PROXY to non-operative port on localhost trick, but without the dead end
HTTP request.  Second, one could try to make this act like the content-policy
blocking, and not show the broken image that results from the dead port option.

The first of these is very easy:  a couple of lines added to
nsProxyAutoConfig.js, and a couple of lines added to nsProtocolProxyService.cpp.
 Let the Javascript pass a new type, and have the C++ return NS_ERROR_FAILURE
upon receiving it.  I didn't like the "NULL" syntax, so I let the PAC return
"IGNORE" if it wanted to ignore the URL, like this:

        // do not load any JPEGs -- proxy them to death
        if ( shExpMatch(url, "*.jpg") ) return "IGNORE";

I did this, and it seemed to work as expected: .jpg's included by image tags
would show broken image icons (just like a dead local proxy except no wasted
HTTP request), and attempts to go directly to a .jpg URL would silently fail
(better than a the dead proxy, which gives a confusing error).  

So far, so good.  The hard part is convincing the browser that this failed load
is by design, and not really an error.  While I found that one can suppress the
broken image icon with user_pref("browser.display.show_image_placeholders",
false), it seemed wrong to be returning NS_ERROR_FAILURE for an intended
behaviour (although this is also done in some cases by NS_CheckContentLoadPolicy
in the content-policy approach).  And abusing a userpref seems sub-ideal.

So I spent time trying to come up with a smooth way to signal that the load
should not happen, but should not trigger a broken image icon.  One needs to get
a message from nsProtocolProxyService.cpp up to nsImageFrame.cpp in a way that
would not bother the other users of the proxy service.  One could either tack a
message on the URI object, or come up with a new error return value.  Using the
URI object seems oblique and bad practice, and coming up with a new
NS_ERROR_POLICY seems extreme.  But maybe it would be good, considering that it
would make the content-policy stuff clearer as well. 

So my conclusion is that this should probably be done, as it is quite simple to
implement and supplants a poorer practice (using a known to be bad local port).
I realize that there are other (and probably more appropriate) ways to
accomplish this sort of blocking, but this is a familiar approach for many users
and actually works quite well.  I'll try to implement the new error value
approach, and then post a patch.  Probably later this week, so feel free to nag
me if I haven't done it by then.

Oh, syntax:  How attached are you to "NULL"?  How do you feel about "IGNORE"?  
Or what about the alternative of coming up with a special host name that one
could use with the existing PROXY directive:  "PROXY _null_", "PROXY ignore"?

Comment 2

[benc]

(nathan, if you are going to work on this bug, you can assign ownership to
yourself and target a milestone as welll...)

From reading the PAC specs, as well as being the only person to propose PAC2,
here's my take:

PROXY <RESERVED> string should not be used because of possible colisions w/ real
hostnames. 

NULL was just the first word that came into my head. I think "ignore" or
"blocked" or something like that would be fine, after all the purpose of the
directive is to "throwaway" the URL request gracefully, so all these poor people
who are using /etc/hosts to block ads can stop administratively cutting off
their nose to save their systems from ads.

As for the implementation details you mention, I think you very much should talk
to both layout/UI and docshell module owners, because I approached my request
purely from a networking perspective. I never figure out how a blocked request
should LOOK, either to the network consumer modules, nor to the end user. 

It might be possible that they would want to add some kind of new error state,
like PAC_rejection, because there might be uses for making it distinct from a
"real" network error. (For example, I could imagine that some people would want
the rejection to happen interactively, which would mean docshell would need to
be able to distinguish between PAC blocks and real network problems).

Comment 3

[Nathan Kurz]

Attachment: Patch to allow PAC code to gracefully doom a request (PAC2)

Here's a patch in progress to allow the PAC code to deny a URL request in a
manner that does not make a request to a (hopefully) unresponsive local port
nor cause the display of broken image icons on the users screen.

Most of the patch happens in netwerk, but error handling had to be minorly
changed in "modules/libpr0n/src/imgLoader.cpp". In private email,
[e-mail] granted theoretical approval of this change. 

I've tested this patch only minorly, and only on Linux.  The comments starting
with "FIXME" are notes to myself that probably will not be in a final patch. 
But as far as I know, this patch should be safe to apply to a test system.

Comment 4

[Nathan Kurz]

Attachment: Sample PAC file showing syntax for DENY

This is a sample PAC file to show the syntax of DENY and to illustrate the
differences between DENY and proxying to a dead local port.  DENY blocks the
request without visible error, while proxying to a dead port (unless
pref("browser.display.show_image_placeholders", false)) has a broken image
icon.

Hmm.  You can probably use this attachment directly from the web as a PAC file,
but (since I'm still typing this) I haven't tried it yet. Else save to a file.

Comment 5

[Nathan Kurz]

Attachment: Second draft patch allowing PAC to deny request

Here's another patch with a couple small modifications to account for interim
changes to the other source files.  I think this is pretty much ready to go. 
Review would be appreciated.

I needed to make a tiny reversion to the patch of nsIOService.cpp for bug
#176919, but I think this is fine.  It was a giant patch, and this particular
change didn't seem to be directly relevant to it.  Worth checking, though.

Comment 6

[Darin Fisher]

Comment on attachment 112736 [details] [diff] [review]
Second draft patch allowing PAC to deny request

this patch has rotted.