-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Avoid NPEs in JavaNetAuthenticator #9100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Avoid NPEs in JavaNetAuthenticator #9100
Conversation
54448bb to
415ab5c
Compare
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/authenticator/JavaNetAuthenticator.kt
Show resolved
Hide resolved
| else -> { | ||
| val socketAddress = address() as InetSocketAddress | ||
| if (socketAddress.isUnresolved) { | ||
| dns.lookup(socketAddress.hostString).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this resolving the DNS address? I don’t think we guarantee that we’ll use this same resolved address later when we connect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, I see from your stack trace. Hmmmmm.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s just change this function to return InetAddress? ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that’s what this should do. Authenticator.requestPasswordAuthentication is even documented as such.
addr – The InetAddress of the site requesting authorization, or null if not known.
| else -> { | ||
| val socketAddress = address() as InetSocketAddress | ||
| if (socketAddress.isUnresolved) { | ||
| dns.lookup(socketAddress.hostString).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s just change this function to return InetAddress? ?
| else -> { | ||
| val socketAddress = address() as InetSocketAddress | ||
| if (socketAddress.isUnresolved) { | ||
| dns.lookup(socketAddress.hostString).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that’s what this should do. Authenticator.requestPasswordAuthentication is even documented as such.
addr – The InetAddress of the site requesting authorization, or null if not known.
Configuring
JavaNetAuthenticatoras theproxyAuthenticatorwhen using the default ProxySelector causes NPEs to be thrown because the socket addresses returned are unresolved:The implementation could also NPE on a 407 from an non-proxy host, so I've handled this too.