Closed Bug 205947 Opened 21 years ago Closed 20 years ago

Proxy: MailNews does not display proxy auth dialog

Categories

(SeaMonkey :: MailNews: Message Display, defect)

Product:
SeaMonkey
Component:
MailNews: Message Display
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: grant.fengstad, Assigned: mscott)

References

Details

Attachments

(2 files, 5 obsolete files)

mailnews-proxy-auth-2.patch (aviary)
12.51 KB, patch
vlad
: review+
vlad
: superreview+
vlad
: approval-aviary+
roc
: approval1.7.5+
Details | Diff | Splinter Review
mailnews-proxy-patch-2.patch (trunk)
12.44 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030514
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030514

I am configured to use a proxy in order to access Internet based content and
services.  This proxy requires authentication upon first use.

When I open up the mail client and open an email that contains Internet based
content (ie: images, etc.), they never load.  The mail/news client does not
invoke the proxy authentication routine when attempting to load Internet based
content.

If I go to the browser and then direct a session to the Internet, I am prompted
for authentication within the browser.  Once authenticated, the mailnews client
works like a charm.

Reproducible: Always

Steps to Reproduce:
1.Open Mozilla (MailNews Client)
2.Open email that references internet based content
3.Content is not displayed
We can confirm this behaviour. We also noticed and reported the identical
behaviour in current Thunderbird builds. Unfortunately, with Thunderbird being a
standalone mail program, there is no similar workaround. 
I can also confirm that this is a regression introduced after Mozilla 1.4RC1, as
it works properly in that build. Please elevate the priority of this bug as it
disables Thunderbird from displaying HTML mail with embedded links to external
web sites. We would be pleased to assist in testing a patch for this issue
through our proxy servers.
Flagging this bug for thunderbird. 

Unfortunately I don't have access to proxy servers to try to debug this so that
makes it extremely hard! 

Any java script errors showing up in the JS console when you try this?
Assignee: sspitzer → scott
Status: UNCONFIRMED → NEW
Ever confirmed: true
No, no errors in the JS console or anywhere else.  It just never tries to do the
proxy authentication.
*** Bug 217799 has been marked as a duplicate of this bug. ***
Mozilla Thunderbird 0.5a (20040105) still exhibits this behaviour. Clicking 
the Thunderbird icon (top right) launches a proxy challenge. The default 
browser (Firebird 0.7) is launched, but returning to Thunderbird and refresing 
the message, it still doesn't display.
As expected, this MailNews client issue was not resolved when we switched from
the Sun Proxy server to Squid proxy on Linux. The Mozilla MailNews client still
does not issue a proxy challenge when e-mail with external links is received.

At the bottom of this post is a sample html message exhibiting this behaviour.

Using Mozilla 1.6, a proxy challenge can be generated by asking the web browser
to go to an external web site. Once this has been done, a refresh of the mail
message allows everything to display as expected.

==============================================================================
Sample mail message illustrating this bug:
==============================================================================

Return-path: <M8jAMwIBAQAGZLIACAAAAAAEL6/q8w@cnl08.b.tep1.com>
Received: from proxy2.komatsu.ca (proxy2.komatsu.ca [XXX.XXX.XXX.XXX])
 by messenger.komatsu.ca
 (iPlanet Messaging Server 5.2 HotFix 1.12 (built Feb 13 2003))
 with SMTP id <JS4L08PH7AKS4N02YW@messenger.komatsu.ca> for
 wbayer@ims-ms-daemon; Fri, 06 Feb 2004 11:28:44 -0500 (Eastern Standard Time)
Received: From out005.toptp.com ([38.113.202.25])
 by proxy2.komatsu.ca (WebShield SMTP v4.5 MR1a P0803.345)
 ;	id 1076084922979; Fri, 06 Feb 2004 11:28:42 -0500
Date: Fri, 06 Feb 2004 08:13:31 -0800
From: FurnitureFan <Newsletter@FurnitureFan.com>
Subject: Furniture Newsletter
To: wbayer@komatsu.ca
Errors-to: M8jAMwIBAQAGZLIACAAAAAAEL6/q8w@cnl08.b.tep1.com
Reply-to: Newsletter@FurnitureFan.com
Message-id: <1911963319-1463792638-1076084043@cnl08.b.tep1.com>
MIME-version: 1.0
Content-type: multipart/alternative; boundary=1249438995.1463793406.1076084022
Original-recipient: rfc822;wbayer@komatsu.ca

--1249438995.1463793406.1076084022
Content-Type: text/plain

FurnitureFan.com
The Furniture Directory

If you cannot view this message, please visit:
http://cnl08.c.tep1.com/maabU98aa4aGXa6Xxhne/

Romancing Your bedroom
This Valentines Day rekindle the magic and escape your every day life. Create a
seductive environment by transforming your space with any of these elegant
hand-crafted items from Oakwood Interiors.

See more bedroom
http://cnl08.c.tep1.com/maabU98aa4aGYa6Xxhne/

Oakwood Interiors - Alder Rose Mansion Bed
http://cnl08.c.tep1.com/maabU98aa4aGZa6Xxhne/

Build your dreams piece by piece 
with the Alder Rose Mansion Collection from Oakwood Interiors. American made of
solid wood, this collection will stand the test of time with its quality and
exceptional design. 

Alder Rose Vanity
http://cnl08.c.tep1.com/maabU98aa4aG0a6Xxhne/

Alder Rose Night Stand
http://cnl08.c.tep1.com/maabU98aa4aG1a6Xxhne/

====================================================================
Update your profile here:
http://cnl08.u.tep1.com/survey/?a2i7O3.a6Xxhn.d2JheWVy

Unsubscribe here:
http://cnl08.u.tep1.com/survey/?a2i7O3.a6Xxhn.d2JheWVy.u

Delivered by Topica Email Publisher, http://www.email-publisher.com/

--1249438995.1463793406.1076084022
Content-Type: text/html
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<title>FurnitureFan Consumer Newsletter</title>
<meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Diso-8859-=
1">
<style type=3D"text/css">
<!--
.mnav {


	font-family: Arial, Helvetica, sans-serif;
	font-size: 12px;
	font-weight: bold;
	font-variant: normal;
	color: #333333;
	padding: 8px;
	text-align: left;
	font-style: normal;
	line-height: normal;
	word-spacing: 7px;
	border-top: 1px;
	border-right: 1px;
	border-bottom: 1px;
	border-left: 1px;
	background-color: #F3F1E4;
}
a:link {
	color: #666666;
	text-decoration: none;
}
a:visited {
	color: #666666;
	text-decoration: none;
}
a:hover {
	color: #666666;
	text-decoration: underline;
}
a:active {
	color: #666666;
	text-decoration: none;
}
.greenbox {
	border: 1px solid #D1CCA4;
}
.T1 {
	font-family: Arial, Helvetica, sans-serif;
	font-size: 10pt;
	color: #333333;
	line-height: 20px;
}
.t2 {
	font-family: Arial, Helvetica, sans-serif;
	font-size: 9.5pt;
	color: #333333;
	width: 220px;
	text-align: left;
	padding: 3px;
}
.t3 {
	font-family: Arial, Helvetica, sans-serif;
	font-size: 9pt;
	color: #333333;
}
.t4 {
	font-family: Arial, Helvetica, sans-serif;
	font-size: 8.6pt;
	color: #303030;
	padding: 5px;
	font-weight: bold;
	text-align: center;
}
.white-top {
	border-top-width: 1px;
	border-top-style: solid;
	border-top-color: #FFFFFF;
}
-->
</style>
</head>

<body leftmargin=3D"0" topmargin=3D"0">
<table width=3D"630" border=3D"0" align=3D"center" cellpadding=3D"0" cellsp=
acing=3D"0">
  <tr>=20
    <td><FONT face=3D"Arial, Helvetica, sans-serif" size=3D1><a href=3D"htt=
p://cnl08.c.tep1.com/maabU98aa4aGy=
a6Xxhne/">If=20
      you cannot view this message, please visit: http://www.furniturefan.c=
om/ECP/jumppage.aspx?linkid=3D3608</a><br>
      To continue receiving your FurnitureFan Newsletter without interrupti=
on,=20
      please add =93Newsletter@FurnitureFan.com=94 to <br>
      your email Address Book.</FONT></td>
  </tr>
  <tr>=20
    <td><a href=3D"http://cnl08.c.tep1.com/maabU98aa4aGz=
a6Xxhne/"><img src=3D"http://www2.furniturefan.com/ecp/cn/0013/images/Furni=
tureFan.gif" width=3D"180" height=3D"34" border=3D"0"></a></td>
  </tr>
  <tr>=20
    <td class=3D"mnav"><div align=3D"center"><a href=3D"http://cnl08.c.tep1=
.com/maabU98aa4aGA=
a6Xxhne/">Living=20
        Room</a> <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c=
.tep1.com/maabU98aa4aGB=
a6Xxhne/">Dining=20
        Room</a> <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c=
.tep1.com/maabU98aa4aGC=
a6Xxhne/">Bedroom</a>=20
        <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c.tep1.com=
/maabU98aa4aGD=
a6Xxhne/">Entertainment</a>=20
        <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c.tep1.com=
/maabU98aa4aGE=
a6Xxhne/">Kids</a>=20
        <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c.tep1.com=
/maabU98aa4aGF=
a6Xxhne/">Office</a>=20
        <font color=3D"#972D38">|</font> <a href=3D"http://cnl08.c.tep1.com=
/maabU98aa4aGG=
a6Xxhne/">Accent=20
        Furniture</a></div></td>
  </tr>
  <tr>=20
    <td height=3D"5" bgcolor=3D"#FFFFFF">&nbsp;</td>
  </tr>
  <tr>=20
    <td class=3D"greenbox"><table width=3D"630" border=3D"0" cellspacing=
=3D"3" cellpadding=3D"0">
        <tr>=20
          <td width=3D"276" valign=3D"top" bgcolor=3D"E7E2C8"> <table width=
=3D"276" border=3D"0" cellspacing=3D"0" cellpadding=3D"0">
              <tr>=20
                <td><img src=3D"http://www.furniturefan.com/ecp/cn/0016/ima=
ges/bnr.gif" width=3D"276" height=3D"94"></td>
              </tr>
              <tr>=20
                <td style=3D"padding-left: 15px; padding-right:15px; paddin=
g-top:4px; padding-bottom:15px"><div class=3D"t1">=20
                    <p>This Valentines Day rekindle the magic and escape yo=
ur=20
                      every day life. Create a seductive environment by tra=
nsforming=20
                      your space with any of these elegant hand-crafted ite=
ms=20
                      from <strong>Oakwood Interiors</strong>.</p>
                  </div></td>
              </tr>
              <tr>=20
                <td style=3D"padding-top:4px; padding-bottom:15px; padding-=
left:5px;"><a href=3D"http://cnl08.c.tep1.com/maabU98aa4aGH=
a6Xxhne/"><img src=3D"http://www.furniturefan.com/ecp/cn/0016/images/furnit=
ure.gif" width=3D"259" height=3D"59" border=3D"0"></a></td>
              </tr>
            </table></td>
          <td align=3D"left" valign=3D"top"> <div align=3D"left">=20
              <table width=3D"350" height=3D"100%" border=3D"0" cellpadding=
=3D"0" cellspacing=3D"2">
                <tr>=20
                  <td><a href=3D"http://cnl08.c.tep1.com/maabU98aa4aGJ=
a6Xxhne/"><img src=3D"http://www.furniturefan.com/ecp/cn/0016/images/oakwoo=
d_bed.jpg" width=3D"350" height=3D"340" border=3D"0"></a></td>
                </tr>
              </table>
            </div></td>
        </tr>
      </table></td>
  </tr>
  <tr>=20
    <td height=3D"5"></td>
  </tr>
  <tr>=20
    <td bgcolor=3D"972D38" class=3D"greenbox"><table width=3D"630" border=
=3D"0" cellspacing=3D"0" cellpadding=3D"0">
        <tr>=20
          <td width=3D"117"><a href=3D"http://cnl08.c.tep1.com/maabU98aa4aG=
L=
a6Xxhne/"><img src=3D"http://www.furniturefan.com/ecp/cn/0016/images/oakwoo=
d_vanity2.jpg" width=3D"117" height=3D"150" border=3D"0"></a></td>
          <td width=3D"125" align=3D"right"><a href=3D"http://cnl08.c.tep1.=
com/maabU98aa4aGO=
a6Xxhne/"><img src=3D"http://www.furniturefan.com/ecp/cn/0016/images/oakwoo=
d_chest3.jpg" width=3D"124" height=3D"150" border=3D"0"></a></td>
          <td class=3D"T1" style=3D"padding-left:15px;"><font color=3D"E7E2=
C8"><img src=3D"http://www.furniturefan.com/ecp/cn/0016/images/furniture_fa=
n7.gif" width=3D"335" height=3D"30"><br>
            with the Alder Rose Mansion Collection from Oakwood Interiors. =
American=20
            made of solid wood, this collection will stand the test of time=
 with=20
            its quality and exceptional design.</font></td>
        </tr>
      </table></td>
  </tr>
  <tr>
    <td><div align=3D"center"><font color=3D"#333333" size=3D"1" face=3D"Ar=
ial, Helvetica, sans-serif">FurnitureFan.com=20
        142 North Rd., Sudbury, MA 01776</font></div></td>
  </tr>

</table>
<img src=3D"http://www.furniturefan.com/ECP/jumppage.aspx?linkid=3D3593">=
=20
<!-- 0x0fa9bafd8b87adf4ab0a121bfdabe341000 -->
<img src=3D"http://cnl08.c.tep1.com/naabU98aa4aGQ=
a6Xxhne/" width=3D1 height=3D1>
<!-- 0x012390fa6570193887fff02398eeaa99283 -->


<!-- Begin Footer -->
<P>
<CENTER>
        <FONT STYLE=3D"font: 10px Verdana, Sans-Serif;" FACE=3D"Verdana, Sa=
ns-Serif" SIZE=3D"-1">
         <A HREF=3D"http://cnl08.u.tep1.com/survey/?a2i7O3.=
a6Xxhn.=
d2JheWVy">Update your profile</A> or
         <A HREF=3D"http://cnl08.u.tep1.com/survey/?a2i7O3.=
a6Xxhn.=
d2JheWVy.u">unsubscribe</A> here.
        </FONT>
</CENTER>

<CENTER><FONT STYLE=3D"font: 10px verdana, sans-serif;" FACE=3D"Verdana, Sa=
ns-Serif" SIZE=3D"-1">
Delivered by <A HREF=3D"http://www.email-publisher.com/">Topica Email Publi=
sher</a>

</font></center>
</body>
</html>

--1249438995.1463793406.1076084022--

==============================================================================

We are willing to test a patch once available.

Regards,

Bill



Darin do you remember any changes to our proxy code going in after 1.4 RC1?

I'll start searching the 1.4 branch checkins since we should have branched
before RC1 was released.
Scott: the proxy servers I used to use got RIF'd, so if you want me to look at
this, I can scheme about configuring something for mozilla.org.

Grant: can you see if password manager functions as a workaround? This might
solve your problem, it also might help understand what is going on.
QA Contact: esther → benc
Summary: MailNews Client does not prompt for proxy authentication upon first load → Proxy: MailNews does not display proxy auth dialog
Yikes!  Is this bug still a problem?  Sounds pretty major.
(In reply to comment #10)
> Yikes!  Is this bug still a problem?  Sounds pretty major.

I confirmed this problem persists using the Windows Mozilla Thunderbird 0.5+
(20040312) build on Windows 2000, both with the "Automatic proxy configurarion
URL" selected under Advanced Options or using manual proxy preferences.

We have both the Linux Squid and Netscape 3.6 proxy servers. 

Also seen on occasion (however difficult to replicate): The first time an
external URL link in an e-mail message is clicked, Firefox 0.8 (also configured
with the "Automatic proxy configurarion URL" option) is started, however no
proxy challenge appears. Clicking the link a second time does result in the
proxy challenge being initiated.

We would like to assist in resolving this bug. Please advise how we can help.


Darin: I think you are going to need to decide if this needs to be fixed. In the
context of 1.7, integrated products like Mozilla have a build in work-around.

Scott: I do not closely track Thunderbird bugs, but I would strongly recommend
fixing this before your next Thunderbird release.
Flags: blocking1.7?
Flags: blocking1.7? → blocking1.7+
this bug was caused by the security drill:

http://bugzilla.mozilla.org/show_bug.cgi?id=51631

auth dialogs are disabled in all mail message windows because spammers would use
it to force up a user/pwd dialog and get the user's name and password.
Here's where the docshell refuses to return an auth prompter if auth is turned
off for this docshell instance:

http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#386

as you can see at this point in time we have no idea what kind of auth request
we are being asked for (legit proxy or other). 


*** Bug 221772 has been marked as a duplicate of this bug. ***
Question for Darin :)

In fixing the big security bug, an attribute was added to nsIDocShell for
blocking secure auth prompts. Instead of fixing all of the callers that try to
get a nsIAuthPrompter from the docshell to bring up auth UI, it looks like we
hide this in nsIDocShell::GetInterface. Failing to return the auth prompt if
this flag has been set on the docshell. 

Of course GetInterface has no way of knowing what the auth prompt is going to be
used for. In the case of a legitimate proxy request, we may want to allow it. 

Seems to me we could solve this a couple of ways:

1) Always return a nsIAuthPrompter, put the burden on the caller to check and
see if they should be allowed to use the returned auth prompter. My fear here is
that we make it too easy for the security hole to show back up for new code.

2) I wonder if we could have a special way for the http channel to still get the
auth prompter even if the flag is set on the docshell object and the http
channel (and just this channel) could check to see if we are doing proxy auth
before actually using the auth prompt. 

3) Remove nsIAuthPrompter from nsIDocShell::GetInterface. Make an explicit
method for getting the auth prompt from a docshell. Force all callers to pass in
a boolean flag for proxy which determines if we return an auth prompter or not.

Any other ideas? 
*** Bug 241846 has been marked as a duplicate of this bug. ***
Echoing comment #12, I would strongly recommend fixing this before your next
Thunderbird release.
Scott: as we discussed before, you could consider supporting proxy via a fixed
pref format. Take a look at Instant Messenger module of Netscape 7 in the prefs.

Also, this bug seems to require the same changes as bug 112179, so maybe the
combined advantages could make a case of implementing this.


This is a blocker for a major French account on Thunderbird. Mozilla Europe
would *love* to see this bug fixed.
It is also blocking Glocalnet in Sweden.
Doesn't look like this is gonna happen for 1.7.
Flags: blocking1.7+ → blocking1.7-
looks like this is going to need some redesign to avoid security problems and
still provide what it needed.  we are going to have to put this off for 1.7 and
try for 1.8, with the hope that a patch might be able to be back ported for
1.7.1 and tbird 1.0...
Flags: blocking1.8a2+
A major problem is that the nsIAuthPrompt implementation does not have
sufficient context to determine whether or not to show a prompt.  I think the
solution to this bug lies in improving the nsIAuthPrompt API so that it has
sufficient context.  In this case, that means telling the prompt whether or not
it is being invoked for proxy auth.  (I can think of other bits of information
that would be valuable for other bugs too... like the degree of password
protection.)

If nsIAuthPrompt were modified so that necko could pass more context to the
prompter, then we would just need to modify the prompter implementation that is
used by mailnews.
Hmm, changing nsIAuthPrompt sounds like it wouldn't be possible to backport to
the long-lived stable branch.
well, we could also invent nsIAuthPrompt2.  necko could try to use
nsIAuthPrompt2 and fallback to using nsIAuthPrompt if it cannot get a
nsIAuthPrompt2 implementation.  we could continue disabling support for
nsIAuthPrompt in mailnews.
Seems to me this is also blocking RSS support in Thunderbird.
When adding a rss feed, Thunderbird will try to validate the provided URL, it
never finishes.
Flags: blocking1.8a4?
Attached patch mailnews-proxy-auth-0.patch (obsolete) — — Splinter Review 
How's this for an attempt?

I added a nsIAuthPromptRequestor interface that takes flags indicating what
type of auth request is wanted (well, just 0 or 1 for now, other flags in the
future as necessary).  nsHttpChannel then tries to get this interface first and
use it to get an nsIAuthPrompt with flags; otherwise it falls back to just
doing a GetInterface.

The only problem I see is I don't see how this approach (or any other)
maintains the lockdown which caused this problem in the first place --
nsHttpChannel decides whether to tell AuthPromptRequester it wants a proxy auth
dialog based on its is proxyAuth flag, which is set if the HTTP response code
is 407 as opposed to 401.  This means that all a malicious site would have to
do would be to return a 407 instead of a 401, and HttpChannel would have no way
of knowing whether it came from a legitimate proxy or not.  Not sure how to fix
this or even if we can; we can maybe ensure that there was a proxy configured
and that the auth request is coming from there, but that'll fail in the
(admittedly slightly contrived) case of a transparent http proxy that wants
auth.
Comment on attachment 157248 [details] [diff] [review]
mailnews-proxy-auth-0.patch

Darin, could you take a look at this?  Or please suggest another reviewer if
you're not the best person :)  See comments above for patch notes.
Attachment #157248 - Flags: review?(darin)
Vlad: this patch makes sense to me.  I haven't done a thorough review yet, but I
too am concerned about the 407 from origin server case.  Currently, we prevent
exposing cached proxy credentials when a 407 occurs over a non-proxy connection.
 We just treat the 407 as coming from the origin server instead of from the
proxy server.  That affects the password manager key used, which affects what is
prefilled in the auth prompt.  But, we preserve the fact that this is a proxy
auth attempt.  We should probably set proxyAuth to false inside
nsHttpChannel::GetCredentialsForChallenge at the right place to be consistent.

I'll need to read the RFCs again to be sure, but I think we can probably assume
that 407 can only rightfully be generated by an explicit proxy server. 
Transparent proxies are probably not permitted to generate that error code,
although folks may be abusing that restriction in the real world.
Something like this?  As long as any legitimate proxy eats any 407's that it
receives itself from a site, this should be fine -- does the spec require that?
Comment on attachment 157248 [details] [diff] [review]
mailnews-proxy-auth-0.patch

if you do want to go down a route like this, I would instead change
nsIAuthPrompt to add a "boolean isProxyAuth" param, or something like that.
that interface is suboptimal anyway, with requiring to pass the entire dialog
text to it...
If we add the isProxyAuth param to nsIAuthPrompt then perhaps we don't need
nsIAuthPromptRequestor.  I assumed the point of nsIAuthPromptRequestor was to
avoid having to change nsIAuthPrompt.  Is that not right?
(In reply to comment #33)
> If we add the isProxyAuth param to nsIAuthPrompt then perhaps we don't need
> nsIAuthPromptRequestor.  I assumed the point of nsIAuthPromptRequestor was to
> avoid having to change nsIAuthPrompt.  Is that not right?

That's correct (I actually typed up a reply earlier this morning, but just now
notice that the bugzilla tab is sitting at the password prompt..)

It seems cleaner to follow the current practice of not giving out a
nsIAuthPrompt at all if the user shouldn't be prompted; hence the Requestor,
with added flags.  Otherwise, the decision whether to display a prompt or not
has to be moved into a nsIAuthPrompt implementation.
At some point, I do wish to provide a better nsIAuthPrompt.  We should give the
prompt the knowledge that it is prompting for proxy auth vs. normal auth, etc. 
But, there are many other things that should be included in a reworking of such
an interface.  For now, I'm happy with nsIAuthPromptRequestor.  It could be a
low risk addition to the stable branches too.
this is going to be a very welcome fix for the aviary branch!
Take a hard line and reject 407 when proxy server not configured.  This is
consistent with IE.
Attachment #157339 - Attachment is obsolete: true
Comment on attachment 157248 [details] [diff] [review]
mailnews-proxy-auth-0.patch

>+[scriptable, uuid(129d3bd5-8a26-4b0b-b8a0-19fdea029196)]
>+interface nsIAuthPromptRequestor : nsISupports
>+{
>+    /* Normal (non-proxy) prompt request. */
>+    const PRUint32 PROMPT_NORMAL        = 0;
>+
>+    /* Proxy auth request. */
>+    const PRUint32 PROMPT_PROXY         = 1;
>+
>+    /**
>+     * Request a nsIAuthPrompt interface for the given flags;
>+     * throws NS_ERROR_FAILURE if no prompt is allowed or
>+     * available for the given reason.
>+     */
>+    nsIAuthPrompt requestAuthPrompt(in PRUint32 aPromptReason);
>+};

please create a new IDL file for this interface, and provide a
summary comment of some sort.  use doxygen style commenting for
params and exceptions.

should this be named nsIAuthPromptProvider instead?  oh, i guess
nsIAuthPromptRequestor corresponds to nsIInterfaceRequestor, so
maybe getAuthPrompt to correspond with getInterface?  whatever ;-)


>Index: docshell/base/nsDocShell.cpp


>+nsDocShell::RequestAuthPrompt (PRUint32 aPromptReason, nsIAuthPrompt **aResult)

GNU whitespace alert!  "when in rome, stay in rome" :-)


>+{
>+    // a priority prompt request will override a false mAllowAuth setting
>+    PRBool priorityPrompt = PR_FALSE;
>+
>+    if (aPromptReason & nsIAuthPromptRequestor::PROMPT_PROXY)
>+        priorityPrompt = PR_TRUE;

maybe this:

  priorityPrompt = aPromptReason & nsIAuthPromptRequestor::PROMPT_PROXY;


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>@@ -2304,22 +2304,37 @@ nsHttpChannel::PromptForIdentity(const c

>+        if (proxyAuth)
>+            rv = authPromptRequestor->RequestAuthPrompt (nsIAuthPromptRequestor::PROMPT_PROXY, getter_AddRefs(authPrompt));
>+        else
>+            rv = authPromptRequestor->RequestAuthPrompt (nsIAuthPromptRequestor::PROMPT_NORMAL, getter_AddRefs(authPrompt));

nit: please fix whitespace and put a linebreak somewhere in those long lines.


>+
>+        if (NS_FAILED(rv))
>+            return NS_ERROR_NO_INTERFACE;

no need to assign or check rv really.  the !authPrompt check down below is good
enough, no?  also, how about making only one call to RequestAuthPrompt?  store
the prompt type in a variable, and then make only one call.  help the compiler
optimize a bit ;-)


>+    } else {
>+        GetCallback(NS_GET_IID(nsIAuthPrompt), getter_AddRefs(authPrompt));
>+    }

please keep braces consistent with the rest of this file, which means:

    }
    else
	GetCallback(NS_GET_IID(nsIAuthPrompt), getter_AddRefs(authPrompt));
Attachment #157248 - Flags: review?(darin) → review-
(In reply to comment #33)
> I assumed the point of nsIAuthPromptRequestor was to
> avoid having to change nsIAuthPrompt.  Is that not right?
(In reply to comment #34)
> It seems cleaner to follow the current practice of not giving out a
> nsIAuthPrompt at all if the user shouldn't be prompted; 

those are good points. so, nevermind comment 32. :)

Index: netwerk/base/public/nsIAuthPrompt.idl
+     * throws NS_ERROR_FAILURE if no prompt is allowed or
+     * available for the given reason.

if no prompt is available, maybe NS_ERROR_NOT_AVAILABLE would be better? :)

Index: docshell/base/nsDocShell.cpp
+    if (aPromptReason & nsIAuthPromptRequestor::PROMPT_PROXY)

hm... wouldn't a reason be more like an enum, so that == would be a better
comparison?

+nsDocShell::RequestAuthPrompt (PRUint32 aPromptReason, nsIAuthPrompt **aResult)
...
+        *aResult = nsnull;
+        return NS_ERROR_FAILURE;

shouldn't need to null out out params in failure cases
Blocks: 221772
nsIInterfaceRequestor is not an interface to emulate in form, function or
spirit.  (The requestor is the _caller_, not the object on which you're calling
getInterface; likewise here.  Nobody wants nsIAuthPromptRequestee, though -- we
really care that it provides it, not that we can ask for it.)
(In reply to comment #40)
> nsIInterfaceRequestor is not an interface to emulate in form, function or
> spirit.  (The requestor is the _caller_, not the object on which you're calling
> getInterface; likewise here.  Nobody wants nsIAuthPromptRequestee, though -- we
> really care that it provides it, not that we can ask for it.)

So, nsIAuthPromptProvider?  Maybe nsIAuthMeHarder?  I'm open to suggestions. :)

*** Bug 221772 has been marked as a duplicate of this bug. ***
Comment on attachment 157455 [details] [diff] [review]
v2 patch for nsHttpChannel::ProcessAuthentication


>-    if (proxyAuth)
>+    if (proxyAuth) {
>+        // only allow a proxy challenge if we have a proxy server configured.
>+        // otherwise, we could inadvertantly expose the user's proxy
>+        // credentials to an origin server.  We could attempt to proceed as
>+        // if we had received a 401 from the server, but why risk flirting
>+        // with trouble?  IE similarly rejects 407s when a proxy server is
>+        // not configured, so there's no reason not to do the same.
>+        if (!mConnectionInfo->UsingHttpProxy()) {
>+            LOG(("rejecting 407 when proxy server not configured!\n"));
>+            return NS_ERROR_UNEXPECTED;
>+        }
>         challenges = mResponseHead->PeekHeader(nsHttp::Proxy_Authenticate);
>+    }
>     else
>         challenges = mResponseHead->PeekHeader(nsHttp::WWW_Authenticate);

Brace the else to match the then?  (Or is that not this Rome's style?)

>-    if (proxyAuth && mConnectionInfo->UsingHttpProxy()) {
>+    if (proxyAuth) {
>         host = mConnectionInfo->ProxyHost();
>         port = mConnectionInfo->ProxyPort();
>         ident = &mProxyIdent;
>         scheme.AssignLiteral("http");
>     }

How would you feel about an assertion there?  I'll see your belt, and raise you
some suspenders!
Attachment #157455 - Flags: superreview+
(In reply to comment #43)
> (From update of attachment 157455 [details] [diff] [review])
> 
> >-    if (proxyAuth)
> >+    if (proxyAuth) {
> >+        // only allow a proxy challenge if we have a proxy server configured.
> >+        // otherwise, we could inadvertantly expose the user's proxy
> >+        // credentials to an origin server.  We could attempt to proceed as
> >+        // if we had received a 401 from the server, but why risk flirting
> >+        // with trouble?  IE similarly rejects 407s when a proxy server is
> >+        // not configured, so there's no reason not to do the same.
> >+        if (!mConnectionInfo->UsingHttpProxy()) {
> >+            LOG(("rejecting 407 when proxy server not configured!\n"));
> >+            return NS_ERROR_UNEXPECTED;
> >+        }
> >         challenges = mResponseHead->PeekHeader(nsHttp::Proxy_Authenticate);
> >+    }
> >     else
> >         challenges = mResponseHead->PeekHeader(nsHttp::WWW_Authenticate);
> 
> Brace the else to match the then?  (Or is that not this Rome's style?)

No; the Romans are filthy heathens, it looks like.

> >-    if (proxyAuth && mConnectionInfo->UsingHttpProxy()) {
> >+    if (proxyAuth) {
> >         host = mConnectionInfo->ProxyHost();
> >         port = mConnectionInfo->ProxyPort();
> >         ident = &mProxyIdent;
> >         scheme.AssignLiteral("http");
> >     }
> 
> How would you feel about an assertion there?  I'll see your belt, and raise you
> some suspenders!

Sounds good to me; I've rolled in darin's patch into the next one I'll post and
can make this change (or I can unroll it, if you'd prefer to r them separately,
but..)
Comment on attachment 157455 [details] [diff] [review]
v2 patch for nsHttpChannel::ProcessAuthentication

r=me, but doesn't the return NS_ERROR_UNEXPECTED; possibly trigger the
superfluous auth warning, which would not really be the right thing here?
Attachment #157455 - Flags: review+
> r=me, but doesn't the return NS_ERROR_UNEXPECTED; possibly trigger the
> superfluous auth warning, which would not really be the right thing here?

of course, that only applies if the URL contains a userpass.
vlad: combining the patches sounds good to me (less to manage when it comes time
to land this on the branches).  shaver's assertion "suspender" sounds reasonable
too.
Attached patch mailnews-proxy-auth-1.patch (obsolete) — — Splinter Review 
Updated patch, with comments incorporated and darin's 407 patch rolled in.
Attachment #157543 - Flags: superreview?(shaver)
Attachment #157543 - Flags: review?(darin)
Comment on attachment 157543 [details] [diff] [review]
mailnews-proxy-auth-1.patch

>Index: netwerk/base/public/nsIAuthPromptProvider.idl

nit: how about a descriptive comment at the top of this file?


>+     * throws NS_ERROR_NOT_AVAILABLE if no prompt is allowed or
>+     * available for the given reason.

nit: use @throws instead?


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+        PRUint32 promptReason = nsIAuthPromptProvider::PROMPT_NORMAL;
>+        if (proxyAuth)
>+            promptReason = nsIAuthPromptProvider::PROMPT_PROXY;

nit: use |if (a) x=y; else x=z;| instead?


r=darin
Attachment #157543 - Flags: review?(darin) → review+
> nit: use |if (a) x=y; else x=z;| instead?

Nit on nit: use x = (a) ? y : z; instead, instead? ;-)

/be
*** Bug 248617 has been marked as a duplicate of this bug. ***
*** Bug 257636 has been marked as a duplicate of this bug. ***
Easy workaround for those who suffer - is to press reply on that HTML message, 
Composer window does display proxy auth dialog.
After that all gets to normall behavior untill mail client restart.
Maybe this info would help you solve this?
Comment on attachment 157543 [details] [diff] [review]
mailnews-proxy-auth-1.patch

I still want those suspenders, punk.

sr=shaver

aviary+ on the basis of mscott's excitement above!
Attachment #157543 - Flags: superreview?(shaver)
Attachment #157543 - Flags: superreview+
Attachment #157543 - Flags: approval-aviary+
Attachment #157455 - Attachment is obsolete: true
Attachment #157543 - Attachment is obsolete: true
Comment on attachment 157679 [details] [diff] [review]
mailnews-proxy-auth-2.patch (aviary)

Carrying flags forward.. r=darin, sr=shaver, a=shaver/mscott
Attachment #157679 - Flags: superreview+
Attachment #157679 - Flags: review+
Attachment #157679 - Flags: approval-aviary+
I meant this patch, really (didn't fix the nits in the last one!)
Attachment #157679 - Attachment is obsolete: true
Comment on attachment 157680 [details] [diff] [review]
mailnews-proxy-auth-2.patch (aviary)

(Real) patch for aviary; carrying flags forward
r=darin,sr=shaver,a=shaver/mscott
Attachment #157680 - Flags: superreview+
Attachment #157680 - Flags: review+
Attachment #157680 - Flags: approval-aviary+
in on trunk and aviary; let me know if it's wanted for 1.7 branch.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
> in on trunk and aviary; let me know if it's wanted for 1.7 branch.

gecko changes so yeah.. probably something that should be ported to the 1.7
branch for consistency with the aviary branch.  moreover, the necko patch
tightens up security a bit, which is probably reason enough to backport this patch.
Attachment #157680 - Flags: approval1.7.x? → approval1.7.x+
Flags: blocking1.8a4?
Blocks: 267367
Product: Browser → Seamonkey
*** Bug 232856 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: