Skip to content

[SPARK-32467][UI]Avoid encoding URL twice on https redirect #29271

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.

Sign up for GitHub

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

Closed
gengliangwang wants to merge 2 commits into from
Closed

[SPARK-32467][UI]Avoid encoding URL twice on https redirect #29271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a4c844
Avoid encoding URL twice
gengliangwang Jul 28, 2020
ef6cb87
add jira ticket
gengliangwang Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions core/src/main/scala/org/apache/spark/ui/JettyUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

package org.apache.spark.ui

import java.net.{URI, URL}
import java.net.{URI, URL, URLDecoder}
import java.util.EnumSet
import javax.servlet.DispatcherType
import javax.servlet.http._
Expand Down Expand Up @@ -377,8 +377,7 @@ private[spark] object JettyUtils extends Logging {
if (baseRequest.isSecure) {
return
}
val httpsURI = createRedirectURI(scheme, baseRequest.getServerName, securePort,
baseRequest.getRequestURI, baseRequest.getQueryString)
val httpsURI = createRedirectURI(scheme, securePort, baseRequest)
response.setContentLength(0)
response.sendRedirect(response.encodeRedirectURL(httpsURI))
baseRequest.setHandled(true)
Expand Down Expand Up @@ -440,16 +439,34 @@ private[spark] object JettyUtils extends Logging {
handler.addFilter(holder, "/*", EnumSet.allOf(classOf[DispatcherType]))
}

private def decodeURL(url: String, encoding: String): String = {
if (url == null) {
null
} else {
URLDecoder.decode(url, encoding)
}
}

// Create a new URI from the arguments, handling IPv6 host encoding and default ports.
private def createRedirectURI(
scheme: String, server: String, port: Int, path: String, query: String) = {
private def createRedirectURI(scheme: String, port: Int, request: Request): String = {
val server = request.getServerName
val redirectServer = if (server.contains(":") && !server.startsWith("[")) {
s"[${server}]"
} else {
server
}
val authority = s"$redirectServer:$port"
new URI(scheme, authority, path, query, null).toString
val queryEncoding = if (request.getQueryEncoding != null) {
Copy link
Member

@zsxwing zsxwing Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try to use the same approach in createProxyURI? Looks like we can try to create a method to share in createRedirectURI and createProxyURI.

    val rewrittenURI = URI.create(uri.toString())
    if (query != null) {
      return new URI(
          rewrittenURI.getScheme(),
          rewrittenURI.getAuthority(),
          rewrittenURI.getPath(),
          query,
          rewrittenURI.getFragment()
        ).normalize()
    }

Copy link
Member Author

@gengliangwang gengliangwang Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it doesn't do the decoding with URI.toString

scala> new URI("https://g.com/ctx%281%29?a%5B0%5D=b").normalize().toString
res28: String = https://g.com/ctx%281%29?a%5B0%5D=b

Copy link
Member Author

@gengliangwang gengliangwang Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can build a URI and call get getPath and getQuery, and then build a new URI string from the decode result. ```
scala> new URI("https://g.com/ctx%281%29?a%5B0%5D=b").getPath
res31: String = /ctx(1)

scala> new URI("https://g.com/ctx%281%29?a%5B0%5D=b").getQuery
res33: String = a[0]=b

But it seems equivalent of the current code.

Copy link
Member

@zsxwing zsxwing Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it doesn't do the decoding with URI.toString

URI.toString doesn't do the decoding. It will encode characters if necessary. For example,

scala> val uri = new java.net.URI(null, null, "foo bar", "foo bar", null).toString
uri: String = foo%20bar?foo%20bar

Anyway, this doesn't matter. I tried to consolidate these two method but it didn't look better.

request.getQueryEncoding
} else {
// By default decoding the URI as "UTF-8" should be enough for SparkUI
"UTF-8"
}
// The request URL can be raw or encoded here. To avoid the request URL being
// encoded twice, let's decode it here.
val requestURI = decodeURL(request.getRequestURI, queryEncoding)
val queryString = decodeURL(request.getQueryString, queryEncoding)
new URI(scheme, authority, requestURI, queryString, null).toString
}

def toVirtualHosts(connectors: String*): Array[String] = connectors.map("@" + _).toArray
Expand Down
21 changes: 21 additions & 0 deletions core/src/test/scala/org/apache/spark/ui/UISuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,27 @@ class UISuite extends SparkFunSuite {
}
}

test("SPARK-32467: Avoid encoding URL twice on https redirect") {
val (conf, securityMgr, sslOptions) = sslEnabledConf()
val serverInfo = JettyUtils.startJettyServer("0.0.0.0", 0, sslOptions, conf)
try {
val serverAddr = s"http://localhost:${serverInfo.boundPort}"

val (_, ctx) = newContext("/ctx1")
serverInfo.addHandler(ctx, securityMgr)

TestUtils.withHttpConnection(new URL(s"$serverAddr/ctx%281%29?a%5B0%5D=b")) { conn =>
assert(conn.getResponseCode() === HttpServletResponse.SC_FOUND)
val location = Option(conn.getHeaderFields().get("Location"))
.map(_.get(0)).orNull
val expectedLocation = s"https://localhost:${serverInfo.securePort.get}/ctx(1)?a[0]=b"
assert(location == expectedLocation)
}
} finally {
stopServer(serverInfo)
}
}

test("http -> https redirect applies to all URIs") {
val (conf, securityMgr, sslOptions) = sslEnabledConf()
val serverInfo = JettyUtils.startJettyServer("0.0.0.0", 0, sslOptions, conf)
Expand Down