Closed
Bug 205947
Opened 22 years ago
Closed 21 years ago
Proxy: MailNews does not display proxy auth dialog
Categories
(SeaMonkey :: MailNews: Message Display, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: grant.fengstad, Assigned: mscott)
References
Details
Attachments
(2 files, 5 obsolete files)
|
12.51 KB,
patch
|
vlad
:
review+
vlad
:
superreview+
vlad
:
approval-aviary+
roc
:
approval1.7.5+
|
Details | Diff | Splinter Review |
|
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.
|
Assignee | |
Comment 3•22 years ago
|
||
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
|
||
Comment 4•22 years ago
|
||
No, no errors in the JS console or anywhere else. It just never tries to do the
proxy authentication.
|
||
Comment 5•22 years ago
|
||
*** 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"> </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
|
|
Assignee | |
Comment 8•21 years ago
|
||
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
|
||
Comment 10•21 years ago
|
||
Yikes! Is this bug still a problem? Sounds pretty major.
|
||
Comment 11•21 years ago
|
||
(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.
|
||
Comment 12•21 years ago
|
||
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?
|
||
Updated•21 years ago
|
Flags: blocking1.7? → blocking1.7+
|
|
Assignee | |
Comment 13•21 years ago
|
||
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.
|
|
Assignee | |
Comment 14•21 years ago
|
||
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).
|
|
Assignee | |
Comment 15•21 years ago
|
||
*** Bug 221772 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 16•21 years ago
|
||
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?
|
|
Assignee | |
Comment 17•21 years ago
|
||
*** Bug 241846 has been marked as a duplicate of this bug. ***
|
|
||
Comment 18•21 years ago
|
||
Echoing comment #12, I would strongly recommend fixing this before your next
Thunderbird release.
|
|
||
Comment 19•21 years ago
|
||
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.
|
||
Comment 20•21 years ago
|
||
This is a blocker for a major French account on Thunderbird. Mozilla Europe
would *love* to see this bug fixed.
|
||
Comment 21•21 years ago
|
||
It is also blocking Glocalnet in Sweden.
|
|
||
Comment 22•21 years ago
|
||
Doesn't look like this is gonna happen for 1.7.
Flags: blocking1.7+ → blocking1.7-
|
||
Comment 23•21 years ago
|
||
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+
|
|
||
Comment 24•21 years ago
|
||
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.
|
||
Comment 25•21 years ago
|
||
Hmm, changing nsIAuthPrompt sounds like it wouldn't be possible to backport to
the long-lived stable branch.
|
|
||
Comment 26•21 years ago
|
||
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.
|
||
Comment 27•21 years ago
|
||
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.
|
||
Updated•21 years ago
|
Flags: blocking1.8a4?
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)
|
|
||
Comment 30•21 years ago
|
||
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 32•21 years ago
|
||
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...
|
|
||
Comment 33•21 years ago
|
||
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.
|
|
||
Comment 35•21 years ago
|
||
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.
|
|
Assignee | |
Comment 36•21 years ago
|
||
this is going to be a very welcome fix for the aviary branch!
|
|
||
Comment 37•21 years ago
|
||
Take a hard line and reject 407 when proxy server not configured. This is
consistent with IE.
Attachment #157339 -
Attachment is obsolete: true
|
|
||
Comment 38•21 years ago
|
||
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-
|
|
||
Comment 39•21 years ago
|
||
(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
|
||
Comment 40•21 years ago
|
||
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. :)
|
|
Assignee | |
Comment 42•21 years ago
|
||
*** Bug 221772 has been marked as a duplicate of this bug. ***
|
|
||
Comment 43•21 years ago
|
||
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 45•21 years ago
|
||
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+
|
|
||
Comment 46•21 years ago
|
||
> 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.
|
|
||
Comment 47•21 years ago
|
||
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.
Updated patch, with comments incorporated and darin's 407 patch rolled in.
Attachment #157248 -
Attachment is obsolete: true
Attachment #157543 -
Flags: superreview?(shaver)
Attachment #157543 -
Flags: review?(darin)
|
|
||
Comment 49•21 years ago
|
||
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+
|
||
Comment 50•21 years ago
|
||
> nit: use |if (a) x=y; else x=z;| instead?
Nit on nit: use x = (a) ? y : z; instead, instead? ;-)
/be
|
|
||
Comment 51•21 years ago
|
||
*** Bug 248617 has been marked as a duplicate of this bug. ***
|
|
||
Comment 52•21 years ago
|
||
*** Bug 257636 has been marked as a duplicate of this bug. ***
|
||
Comment 53•21 years ago
|
||
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 54•21 years ago
|
||
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+
suspenders added, patch for 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+
same patch as above, in trunk flavour
Attachment #157680 -
Flags: approval1.7.x?
in on trunk and aviary; let me know if it's wanted for 1.7 branch.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
|
|
||
Comment 61•21 years ago
|
||
> 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+
|
|
||
Updated•21 years ago
|
Flags: blocking1.8a4?
landed on 1.7 branch
|
||
Updated•21 years ago
|
Product: Browser → Seamonkey
|
||
Comment 63•20 years ago
|
||
*** 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.
Description
•