autoconfig vulnerable to active MITM attacks for all domains (including the ones in ISPDB)
Categories
(Thunderbird :: Account Manager, enhancement)
Tracking
(Not tracked)
People
(Reporter: tagnaq, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(12 files, 77 obsolete files)
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review-
BenB
:
feedback-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review-
BenB
:
feedback-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Comment 5•11 years ago
|
||
Updated•11 years ago
|
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
Comment 20•9 years ago
|
||
Comment 21•9 years ago
|
||
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Comment 24•9 years ago
|
||
Comment 25•9 years ago
|
||
Comment 26•9 years ago
|
||
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
Comment 30•9 years ago
|
||
Comment 31•9 years ago
|
||
Comment 32•9 years ago
|
||
Comment 33•8 years ago
|
||
Updated•8 years ago
|
Comment 34•8 years ago
|
||
Comment 35•8 years ago
|
||
Comment 36•8 years ago
|
||
Comment 37•8 years ago
|
||
Comment 38•8 years ago
|
||
Comment 39•8 years ago
|
||
Comment 40•8 years ago
|
||
Comment 41•8 years ago
|
||
Comment 42•8 years ago
|
||
Comment 43•8 years ago
|
||
Comment 44•8 years ago
|
||
Comment 45•8 years ago
|
||
Comment 46•8 years ago
|
||
Comment 47•8 years ago
|
||
Comment 48•8 years ago
|
||
Comment 49•8 years ago
|
||
Comment 50•8 years ago
|
||
Comment 51•8 years ago
|
||
Comment 52•8 years ago
|
||
Comment 54•8 years ago
|
||
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Comment 55•8 years ago
|
||
Comment 56•8 years ago
|
||
Comment 57•8 years ago
|
||
Comment 58•8 years ago
|
||
Comment 59•8 years ago
|
||
Comment 60•8 years ago
|
||
Comment 61•8 years ago
|
||
Comment 62•8 years ago
|
||
Comment 63•8 years ago
|
||
Comment 64•8 years ago
|
||
Comment 65•8 years ago
|
||
Comment 66•8 years ago
|
||
Comment 67•8 years ago
|
||
Comment 68•8 years ago
|
||
Comment 69•8 years ago
|
||
Comment 70•8 years ago
|
||
Comment 71•8 years ago
|
||
Comment 72•8 years ago
|
||
Comment 73•8 years ago
|
||
Comment 74•8 years ago
|
||
Comment 75•8 years ago
|
||
Comment 76•8 years ago
|
||
Comment 77•8 years ago
|
||
Comment 78•8 years ago
|
||
Comment 79•8 years ago
|
||
Comment 80•8 years ago
|
||
Comment 81•8 years ago
|
||
Comment 82•8 years ago
|
||
Comment 83•8 years ago
|
||
Comment 84•8 years ago
|
||
Comment 85•8 years ago
|
||
Comment 86•8 years ago
|
||
Comment 87•8 years ago
|
||
Comment 88•8 years ago
|
||
Comment 89•8 years ago
|
||
Comment 90•8 years ago
|
||
Comment 91•8 years ago
|
||
Comment 92•8 years ago
|
||
Comment 93•8 years ago
|
||
Comment 94•8 years ago
|
||
Comment 95•8 years ago
|
||
Comment 96•8 years ago
|
||
Comment 97•8 years ago
|
||
Comment 98•8 years ago
|
||
Comment 99•8 years ago
|
||
Comment 100•8 years ago
|
||
Comment 101•8 years ago
|
||
Comment 102•8 years ago
|
||
Comment 103•8 years ago
|
||
Updated•8 years ago
|
Comment 104•8 years ago
|
||
Comment 105•8 years ago
|
||
Comment 106•8 years ago
|
||
Comment 107•8 years ago
|
||
Comment 108•8 years ago
|
||
Comment 109•8 years ago
|
||
Comment 110•8 years ago
|
||
Comment 111•8 years ago
|
||
Comment 112•7 years ago
|
||
Comment 113•7 years ago
|
||
Comment 114•7 years ago
|
||
Comment 115•7 years ago
|
||
Comment 116•7 years ago
|
||
Comment 117•7 years ago
|
||
Comment 118•7 years ago
|
||
Comment 119•7 years ago
|
||
Comment 120•7 years ago
|
||
Comment 121•7 years ago
|
||
Comment 122•7 years ago
|
||
Comment 123•7 years ago
|
||
Comment 124•7 years ago
|
||
Updated•7 years ago
|
Comment 125•7 years ago
|
||
Comment 126•7 years ago
|
||
Comment 127•7 years ago
|
||
Comment 128•7 years ago
|
||
Comment 129•7 years ago
|
||
Comment 130•7 years ago
|
||
Comment 131•7 years ago
|
||
Comment 132•7 years ago
|
||
Comment 133•7 years ago
|
||
Comment 134•7 years ago
|
||
Comment 135•7 years ago
|
||
Comment 136•7 years ago
|
||
Comment 137•7 years ago
|
||
Comment 138•7 years ago
|
||
Comment 139•7 years ago
|
||
Comment 140•7 years ago
|
||
Comment 141•7 years ago
|
||
Comment 142•7 years ago
|
||
Comment 143•7 years ago
|
||
Comment 144•7 years ago
|
||
Comment 145•7 years ago
|
||
Comment 146•7 years ago
|
||
Comment 147•7 years ago
|
||
Comment 148•7 years ago
|
||
Comment 149•7 years ago
|
||
Comment 150•7 years ago
|
||
Updated•7 years ago
|
Comment 151•7 years ago
|
||
Updated•7 years ago
|
Comment 152•7 years ago
|
||
Comment 153•7 years ago
|
||
Comment 154•7 years ago
|
||
Comment 155•7 years ago
|
||
Comment 156•7 years ago
|
||
Comment 157•7 years ago
|
||
Comment 158•7 years ago
|
||
Comment 159•7 years ago
|
||
Updated•7 years ago
|
Comment 160•7 years ago
|
||
Comment 161•7 years ago
|
||
Comment 162•7 years ago
|
||
Comment 163•7 years ago
|
||
Comment 164•7 years ago
|
||
Comment 165•7 years ago
|
||
Comment 166•7 years ago
|
||
Comment 167•7 years ago
|
||
Updated•6 years ago
|
Comment 169•6 years ago
|
||
Comment 170•6 years ago
|
||
Comment 171•6 years ago
|
||
Comment 172•6 years ago
|
||
Comment 173•6 years ago
|
||
Comment 174•6 years ago
|
||
Comment 175•6 years ago
|
||
Comment 176•6 years ago
|
||
Comment 177•6 years ago
|
||
Comment 178•6 years ago
|
||
Comment 179•6 years ago
|
||
Comment 180•6 years ago
|
||
Comment 181•6 years ago
|
||
Comment 182•6 years ago
|
||
Comment 183•6 years ago
|
||
Comment 184•6 years ago
|
||
Comment 185•6 years ago
|
||
Comment 186•6 years ago
|
||
Comment 187•6 years ago
|
||
Comment 188•6 years ago
|
||
Comment 189•6 years ago
|
||
Comment 190•6 years ago
|
||
Comment 191•6 years ago
|
||
Comment 192•6 years ago
|
||
Comment 193•6 years ago
|
||
Comment 195•6 years ago
|
||
Comment 196•6 years ago
|
||
So, is this bug going to be taken to completion? Despite what BenB said, I'd like to see the patches rebased and bundled. I believe we can handle more than one line per patch. So for example, why don't you bundle anything that is clean-up into on patch to start off with? Like 1-5 could be one patch.
Comment 197•6 years ago
|
||
Hi,
We've updated our patch series. It was very straightforward to work with the new priority system, and having all methods run in parallel is such a time saver (especially when running over slow Tor circuits). Great job! \o/
Most of our patches are identical (modulo trivial things like bracket positions) to our previous patch submission. The ones that were rewritten from scratch are:
- 0004-Also-fetch-ISP-configuration-using-SSL.patch
- 0005-Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch
For the other still ~identical patches our Comment 169 [0] is still unanswered by you.
Below I list for each new patch which of our responses to your comments that you should look at:
- 0003-Prefer-fetched-configurations-using-SSL-over-plainte.patch → Commend 152 (so, in our Comment 169, look for our response to your Comment 152)
- 0006-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch → Comment 162
- 0007-Add-SOCKS-proxy-support-for-account-guessing.patch → Comment 153
- 0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch → Comment 159:
The remaining four patches (0001, 0002, 0008, 0009) are, as far as I can tell, already reviewed and ACKed by you.
Cheers!
[0] https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c169
Comment 198•6 years ago
|
||
I'll upload the new version of the patches in ~1h.
Comment 199•6 years ago
|
||
Comment 200•6 years ago
|
||
Comment 201•6 years ago
|
||
Comment 202•6 years ago
|
||
Comment 203•6 years ago
|
||
Comment 204•6 years ago
|
||
Comment 205•6 years ago
|
||
Comment 206•6 years ago
|
||
Comment 207•6 years ago
|
||
Comment 208•6 years ago
|
||
Comment 209•6 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #196)
So, is this bug going to be taken to completion? Despite what BenB said, I'd like to see the patches rebased and bundled. I believe we can handle more than one line per patch. So for example, why don't you bundle anything that is clean-up into on patch to start off with? Like 1-5 could be one patch.
Hi Jorg,
I have seen your comment only today. So there is no bundling in what we propose today to lead this issue to completion. I think in general that patches are easier to apply if they're atomic - especially if many developers operate on the same code base and code changes happen often.
Comment 210•6 years ago
|
||
hi u: there has been significant bitrot of these patches. I understand you have the 10 tiny different commits, but this is an unusual process for us, so please just fold it into one patch that we can test out, review and get landed.
Updated•6 years ago
|
Comment 211•6 years ago
|
||
Comment 212•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #210)
hi u: there has been significant bitrot of these patches. I understand you have the 10 tiny different commits, but this is an unusual process for us, so please just fold it into one patch that we can test out, review and get landed.
That would be great! Here it is: https://bugzilla.mozilla.org/attachment.cgi?id=9056143
Updated•6 years ago
|
Updated•6 years ago
|
Comment 213•6 years ago
|
||
Comment 214•6 years ago
|
||
Comment 215•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 216•6 years ago
|
||
Comment 217•6 years ago
|
||
Comment 218•6 years ago
|
||
Comment 219•6 years ago
|
||
Comment 220•6 years ago
|
||
Comment 221•6 years ago
|
||
Updated•6 years ago
|
Comment 222•6 years ago
|
||
@Jörg:
Patches with r+: You can land those.
@u:
Patches with r+: They can land as-is.
Patches with r- feedback+: The patch is mostly good, I accept the idea, but there are some minor code changes needed before they can land.
Patches with r- feedback-: Please drop those. They are a bad idea from Thunderbird perspective. You could keep them as local patches to TorBirdy.
Patches with r- and no feedback: I'm on the fence for these whether they are a good idea or should be dropped. And they need code changes, too.
Comment 223•6 years ago
|
||
OK, I'll land the four r+ patches. In the future, please provide patches with a proper HG header. Also, these patches need manual massaging to apply, diff --git a/comm/mail/components/accountcreation/content/emailWizard.js ...
is not correct, the patch/diff needs to be taken from the C-C repository, so there is no comm/
. So next time, please supply then in an acceptable form.
Comment 224•6 years ago
|
||
And of course splitting it all into small patches is a pain since, for example, part 8 doesn't apply if we don't use some of the earlier parts. So whilst in theory "pick and choose" is a nice idea, in practise it doesn't work, or someone else needs to do the work :-(
Comment 225•6 years ago
|
||
Comment 226•6 years ago
|
||
@Jörg: Thanks for landing!
part 8 doesn't apply if we don't use some of the earlier parts. So whilst in theory "pick and choose" is a nice idea, in practise it doesn't work, or someone else needs to do the work :-(
Yes. If that happens, feel free to throw the patch back to u (patch author) to update it.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 227•6 years ago
|
||
@u: Could you please update patches:
- 0004 Also-fetch-ISP-configuration-using-SSL (which fixes the main focus of this bug)
- 0007 Add-SOCKS-proxy-support-for-account-guessing
?
Comment 228•5 years ago
|
||
@BenB : thank you for reviewing and merging some of these patches. We will work on them again soon, I will keep you posted once this is clear. (Do 4 weeks matter after the 5th anniversary of this bug? ;))
Comment 229•5 years ago
|
||
Yes, since TB 68 goes to beta in about 10 days. Whatever lands before will be part of TB 68 beta and then TB 68 ESR. Whatever misses the date will wait for a year before it faces the users in TB 76 ESR.
Comment 230•5 years ago
|
||
Thanks, good to know!
Comment 231•5 years ago
|
||
@u: If you can only revise the patches 0004 and 0007, we can get it in, and I think we can call this bug done. They address the original purpose of this bug. And the changes I requested are reasonably small. I'd be happy to bring this to conclusion.
Comment 232•5 years ago
|
||
@BenB: yes, we are trying to that in time for TB 68. I cannot yet promise we'll succeed.
Comment 233•5 years ago
|
||
Hi!
One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))
tl;dr: The two patches you requested have been updated to do exactly
what you asked for:
- 0001-Also-fetch-ISP-configuration-using-SSL.patch (was 0004)
- 0002-Add-SOCKS-proxy-support-for-account-guessing.patch (was 0007)
And we'd also urge you to consider
0003-Add-pref-for-guessing-only-SSL-configs.patch (see below for
context).
Comment on attachment 9047429 [details] [diff] [review]
0007-Add-SOCKS-proxy-support-for-account-guessing.patchJust a small code structure comment: Could you please implement a
async function getProxy()
or
function getProxy(resultCallback)
which returns the proxyInfo or null?
Ah, yes that makes a lot of sense! We went with the latter although
it's called doProxy() ("get" sounds like it should return the
proxyInfo, not pass it via the callback). If you have a better name
perhaps you can just change it yourself to avoid another patch
roundtrip (which we probably won't have time with before the freeze).
Comment on attachment 9047426 [details] [diff] [review]
0004-Also-fetch-ISP-configuration-using-SSL.patch[...]
+// When false an active attacker can block non-SSL fetches and then
+// MitM the HTTP fetch, granting the attacker full control over the
+// client configuration.I understand your idea, but everybody is missing 2 important points:
We understand that when designing this protocol your top priority was
to maximize the chance of ending up with a working config. This is not
all users' priority. For some subsets of users (e.g. Tails' and
TorBirdy's) security is more important than config success. In fact,
successfully enabling an insecure config is probably the worst failure
mode imaginable for them. Please make room for this other perspective
when reading my comments below.
- Using https only will violate the established protocol
Yes, the protocol is insecure by design, and this option breaks it, in
order to make it safe and thus usable for users that require
security.
and will not work in many cases.
Yes, we pay for the security by making insecure configs
unavailable. For users that require security, an insecure protocol
is unusable, so for them it works for no cases. Such users have no
better option than accepting a secure version of the protocol that
works for fewer cases -- those lost cases (i.e. the insecure ones) are
actually exactly the ones they absolutely want to avoid, so from their
perspective this is not a loss but a win.
I know it's insecure, but if you insist on https, you will simply
not find a config, plain and simple.
That is the exact, desired behavior for users that require security.
- Even with this set to true, you are still suspect to a MITM, due
to the guess config.
We mostly agree. You now seem to reject
0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, which
would fix this for those that enable the pref it added. You were
previously in favor of merging this patch (see comment 159) although
we agree to some extent that the changes to readFromXML.js are mostly
covered by the scary warning. So we have lifted only the parts about
forcing SSL when guessing into a third patch adding the guess.sslOnly
pref which will be go great together with fetchFromISP.sslOnly, as per
the updated comments in mailnews.js.
So, please also consider
0003-Add-pref-for-guessing-only-SSL-configs.patch.
Above we said "mostly agree" because we think that HTTP ISP fetch is a
lot worse. In that case the attacker can present a config using SSL
(i.e. there won't be a scary warning) but pointing to domains under the attacker's
control → game over for the user. In the guessing case the attacker
cannot present a configuration using SSL (unless the attacker can
break SSL for that domain somehow) so they are limited to plaintext
configs which results in the scary warning → the user should at least
be more suspicious.
Or it can be put like this: when guessing it is the user's client that
generates configs and tests them, so the attacker can only filter a lot
among these configs; with HTTP fetch, the attacker can create
arbitrary configs, which is fundamentally worse.
And you cannot disable the guess config,
because only 40-50% of the configs are in the DB and via ISP and the
rest needs to be guessed. Guessing often leads to success, so we
really need that. Internal email servers often have no SSL.
This is not true: for users that require security disabling insecure
features is a must. For them those 50-60% configs that can only be
obtained insecurely were never options to begin with, so your argument
doesn't apply to them.
However, with the third patch's guess.sslOnly pref set, this point is
completely moot since guessing SSL only makes it a safe method for
those users again. Yay!
Comment 234•5 years ago
|
||
Comment 235•5 years ago
|
||
Comment 236•5 years ago
|
||
This is a new patch.
Comment 237•5 years ago
|
||
Comment 238•5 years ago
|
||
Comment 239•5 years ago
|
||
Comment 240•5 years ago
|
||
(In reply to u from comment #233)
One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))
Hmm, the patches that got landed all had "anonym <anonym@riseup.net>" in the hg header. How are we supposed to know that the assignee of the bug, someone called "u" who also attached the patches, is in fact not the patch author? I've never seen a case like this before.
Updated•5 years ago
|
Comment 241•5 years ago
|
||
Hold on, aren't we going to address comment #237?
Comment 242•5 years ago
|
||
Comment 243•5 years ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #237)
Comment on attachment 9063503 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing.patchReview of attachment 9063503 [details] [diff] [review]:
[...]
Minor: As mentioned in the review 1 year ago, you're resolving the proxy for
every round. That's dozens of times at the same time. Rather, the proxy
should be resolved once, the nsIProxyInfo stored, and then re-used.
Fixed by fixup-0002-Add-SOCKS-proxy-support-for-account-guessing.patch. I have just done rudimentary testing, but it was a mechanical code transformation so I guess I'm confident. :)
@@ +1087,5 @@
- var proxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"]
.getService(Ci.nsIProtocolProxyService);
- // Use some arbitrary scheme just because it is required...
- var uri = Services.io.newURI("http://" + hostname, null, null);
- // ... we'll ignore it any way. We prefer SOCKS since that's the
NIT: If you ignore it anyway, then you might as well use "imap://" to be at
least closer to reality.
"imap://" triggers an internal error, that is why I picked "http://".
Comment 244•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #240)
(In reply to u from comment #233)
One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))Hmm, the patches that got landed all had "anonym <anonym@riseup.net>" in the hg header. How are we supposed to know that the assignee of the bug, someone called "u" who also attached the patches, is in fact not the patch author? I've never seen a case like this before.
We are git-centric and assumed you would use something like git am
which would import the meta-data from the patch. Sorry for the inconvenience!
Comment 245•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 246•5 years ago
|
||
Comment 247•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/36511bbb7713
Also fetch ISP configuration using SSL. r=BenB
https://hg.mozilla.org/comm-central/rev/8d4257424669
Add pref for guessing only SSL configs. r=BenB
Comment 248•5 years ago
|
||
Comment 249•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #246)
Comment on attachment 9063603 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing-fixup.patchThis patch doesn't apply at all
Crap, I was unclear: fixup-0002 was supposed to be squashed into 0002 (that's why I didn't obsolete 0002). The 0002 I just attached is squashed already. Sorry for the inconvenience!
plus the indentation is wrong in
doProxy()
.
I've removed one indentation level (i.e. two spaces) which I believe was the problem.
001 and 003 have been a pain as well: Patch needed to me made
to apply manually by correcting the incorrect paths. And frankly, whether
you're GIT-centric or not doesn't matter, here we work with Mercurial and
patches must apply and have a decent HG header :-(
Sorry about this!
I assume it's about "comm/" missing in the beginning of the paths? I don't even get that when using Mercurial to generate the patch so I had to manually edit the patch. I hope all is ok now, but I'm available on short notice to fix any remaining issues.
Comment 250•5 years ago
|
||
You mean "merged" or "folded" when you say "squashed"? I tried applying the 002-fixup in top of the 002-original, but that didn't work either. Besides, the 002-original WAS made obsolete in comment #235.
No, the "comm/" MUST NOT be there since this is a patch for the comm-central repository which happens to reside in the "comm/" subdirectory of some other repository. I had to remove "comm/" from the patches. If you look at 001 and 003 you will see that "comm/" was inconsistently applied.
Never mind, I hope this will be the last manual comment in this bug.
Comment 251•5 years ago
|
||
I had to remove "comm/" from the patches.
patch -p 2
or hg import -p 2
Comment 252•5 years ago
|
||
diff --git a/mail/... b/comm/mail/...
Ah, I see what you mean.
Comment 253•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #250)
... I tried applying the 002-fixup in top of the 002-original, but that didn't work either.
That must have been my confusion, it worked now. The result is the final version attached here. Also, please always provide 8 lines of context in your patches using:
[diff]
git = 1
showfunc = 1
unified = 8
in your mercurial.ini.
Comment 254•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f40eb5ec3bfa
Add SOCKS proxy support for account guessing. r=BenB DONTBUILD
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 255•5 years ago
|
||
Thanks to u and anonym@riseup for these good patches! And thanks for your patience and hanging in there over all this time.
Also thanks to Jörg for landing them.
Comment 256•5 years ago
|
||
Landed with the following linting errors fixed.
1085:5 error Extra space after key 'onProxyAvailable'. key-spacing (eslint)
1085:5 error Expected method shorthand. object-shorthand (eslint)
1091:28 error Missing semicolon. semi (eslint)
1092:6 error Missing trailing comma. comma-dangle (eslint)
1097:13 error newURI's last parameters are optional. mozilla/no-useless-parameters (eslint)
Author and reviewer MUST always run linting before submitting or approving patches.
Just keep in mind that none of these patches would have been accepted for mozilla-central. Anything that doesn't cleanly apply or causes linting errors is rejected immediately. You're very lucky to have the (grumpy) comm-central sheriffing service which also serves as a clean-up maid for everyone :-(
Comment 257•5 years ago
|
||
Just keep in mind that none of these patches would have been accepted for mozilla-central. Anything that doesn't cleanly apply or causes linting errors is rejected immediately. You're very lucky to have the (grumpy) comm-central sheriffing service which also serves as a clean-up maid for everyone :-(
@jorgk: thank you very much! We very much appreciate your efforts :)
Description
•