Skip to content

Commit

Permalink
finagle-base-http: add SameSite attribute to Cookie
Browse files Browse the repository at this point in the history
Summary: Problem

Finagle's Cookies do not support the SameSite attribute.

Solution

Allow the attribute to be added via the Cookie constructor.

Differential Revision: https://phabricator.twitter.biz/D157942
  • Loading branch information
Stefan Lance authored and jenkins committed Apr 14, 2018
1 parent 3160a81 commit 4b0a58b
Show file tree
Hide file tree
Showing 14 changed files with 618 additions and 36 deletions.
9 changes: 9 additions & 0 deletions CHANGES
Expand Up @@ -10,6 +10,15 @@ Unreleased
New Features
~~~~~~~~~~~~

* finagle-base-http: Added ability to add SameSite attribute to Cookies to
comply with https://tools.ietf.org/html/draft-west-first-party-cookies-07.
The attribute may be set in the constructor via the `c.t.f.http.Cookie`
`sameSite` param or via the `c.t.f.http.Cookie.sameSite` method.

- Pass `SameSite.Lax` to the `Cookie` to add the "Lax" attribute.
- Pass `SameSite.Strict` to the `Cookie` to add the "Strict" attribute.
``PHAB_ID=D157942``

* finagle-mysql: Added APIs to `Row` which simplify the common access pattern.
For example, `Row.stringOrNull(columnName: String): String` and
`Row.getString(columnName: String): Option[String]`.
Expand Down
11 changes: 11 additions & 0 deletions doc/src/sphinx/Metrics.rst
Expand Up @@ -299,6 +299,17 @@ using `.withHttpStats` on `Http.Client` and `Http.Server`.
**stream/failures**
A counter of the number of times any failure has been observed in the middle of a stream.

**http/cookie/samesite_failures**
A counter of the number of failed attempts to decode the SameSite Cookie attribute.

**http/cookie/flagless_samesites**
A counter of the number of times the SameSite attribute was set in a Response despite the
SameSiteCodec being disabled.

**http/cookie/dropped_samesites**
A counter of the number of times the SameSite attribute was present in a Response Cookie
but dropped after encoding.

HTTP2
-----
These stats pertain to HTTP2 only.
Expand Down
@@ -1,6 +1,7 @@
package com.twitter.finagle.http

import com.twitter.conversions.time._
import com.twitter.finagle.http.cookie.SameSite
import com.twitter.util.Duration
import java.util.{BitSet => JBitSet}
import org.jboss.netty.handler.codec.http.{Cookie => NettyCookie}
Expand Down Expand Up @@ -79,7 +80,8 @@ final class Cookie private (
val path: String,
private[this] val _maxAge: Option[Duration],
val secure: Boolean,
val httpOnly: Boolean
val httpOnly: Boolean,
val sameSite: SameSite
) { self =>

/**
Expand All @@ -92,15 +94,17 @@ final class Cookie private (
path: Option[String] = None,
maxAge: Option[Duration] = Some(Cookie.DefaultMaxAge),
secure: Boolean = false,
httpOnly: Boolean = false
httpOnly: Boolean = false,
sameSite: SameSite = SameSite.Unset
) = this(
name = Cookie.validateName(name),
value = value,
domain = Cookie.validateField(domain.orNull),
path = Cookie.validateField(path.orNull),
_maxAge = maxAge,
secure = secure,
httpOnly = httpOnly
httpOnly = httpOnly,
sameSite = sameSite
)

def this(
Expand All @@ -113,7 +117,27 @@ final class Cookie private (
None,
Some(Cookie.DefaultMaxAge),
false,
false
false,
SameSite.Unset
)

def this(
name: String,
value: String,
domain: Option[String],
path: Option[String],
maxAge: Option[Duration],
secure: Boolean,
httpOnly: Boolean
) = this(
name,
value,
domain,
path,
maxAge,
secure,
httpOnly,
SameSite.Unset
)

@deprecated("Use Bijections.from to create a Cookie from a Netty Cookie")
Expand All @@ -125,7 +149,8 @@ final class Cookie private (
underlying.getPath,
Option(underlying.getMaxAge.seconds),
underlying.isSecure,
underlying.isHttpOnly
underlying.isHttpOnly,
SameSite.Unset /* Netty cookies do not support the SameSite attribute */
)
}

Expand Down Expand Up @@ -153,7 +178,8 @@ final class Cookie private (
path: Option[String] = Option(self.path),
maxAge: Option[Duration] = self._maxAge,
secure: Boolean = self.secure,
httpOnly: Boolean = self.httpOnly
httpOnly: Boolean = self.httpOnly,
sameSite: SameSite = self.sameSite
): Cookie =
new Cookie(
name,
Expand All @@ -162,7 +188,8 @@ final class Cookie private (
path,
maxAge,
secure,
httpOnly
httpOnly,
sameSite
)

/**
Expand Down Expand Up @@ -201,6 +228,12 @@ final class Cookie private (
def secure(secure: Boolean): Cookie =
copy(secure = secure)

/**
* Create a new [[Cookie]] with the same set fields, and sameSite `sameSite`
*/
def sameSite(sameSite: SameSite): Cookie =
copy(sameSite = sameSite)

/**
* Returns true if `obj` equals `this`. Two cookies are considered equal if their names,
* paths, and domains are the same (ignoring case). This mimics the `equals` method on Netty's
Expand Down
@@ -1,8 +1,11 @@
package com.twitter.finagle.http

import com.twitter.finagle.http.cookie.SameSite
import com.twitter.finagle.http.cookie.exp.supportSameSiteCodec
import com.twitter.finagle.http.netty3.Netty3CookieCodec
import com.twitter.finagle.http.netty4.Netty4CookieCodec
import com.twitter.finagle.server.ServerInfo
import com.twitter.finagle.stats.LoadedStatsReceiver
import org.jboss.netty.handler.codec.http.HttpHeaders
import scala.collection.mutable

Expand All @@ -14,6 +17,13 @@ private[finagle] object CookieMap {
private val cookieCodec =
if (UseNetty4CookieCodec(ServerInfo().id.hashCode())) Netty4CookieCodec
else Netty3CookieCodec

private[finagle] val includeSameSite: Boolean = supportSameSiteCodec()

private[finagle] val flaglessSameSitesCounter =
LoadedStatsReceiver.scope("http").scope("cookie").counter("flagless_samesites")
private[finagle] val silentlyDroppedSameSitesCounter =
LoadedStatsReceiver.scope("http").scope("cookie").counter("dropped_samesites")
}

/**
Expand Down Expand Up @@ -71,8 +81,14 @@ class CookieMap private[twitter](message: Message, cookieCodec: CookieCodec)
message.headerMap.set(cookieHeaderName, cookieCodec.encodeClient(values))
} else {
foreach {
case (_, cookie) =>
message.headerMap.add(cookieHeaderName, cookieCodec.encodeServer(cookie))
case (_, cookie) => {
message.headerMap.add(cookieHeaderName,
cookieCodec.encodeServer(cookie))
if (message.headerMap.toString.contains("SameSite")
&& cookie.sameSite != SameSite.Unset) {
CookieMap.silentlyDroppedSameSitesCounter.incr()
}
}
}
}
}
Expand Down Expand Up @@ -182,6 +198,13 @@ class CookieMap private[twitter](message: Message, cookieCodec: CookieCodec)
cookieHeader <- message.headerMap.getAll(cookieHeaderName)
cookie <- decodeCookies(cookieHeader)
} {
// Checks whether the SameSite attribute is set in the response but the
// codec is disabled. This is undesirable behavior so we wish to report it.
if (!CookieMap.includeSameSite
&& message.isResponse
&& cookieHeader.contains("SameSite")) {
CookieMap.flaglessSameSitesCounter.incr()
}
add(cookie)
}
}
@@ -0,0 +1,29 @@
package com.twitter.finagle.http.cookie

/**
* SameSite cookie attribute. The server may set this in the Set-Cookie to
* ensure that the cookie is not sent with cross-site requests.
*
* As of April 2018, support for this attribute is not implemented by
* all browsers. See [0] for more details.
*
* [0] https://tools.ietf.org/html/draft-west-first-party-cookies-07
*/
sealed trait SameSite

object SameSite {

/**
* See [1] for the distinction between the Lax and Strict types.
* [1] https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1.1
*/
case object Lax extends SameSite

case object Strict extends SameSite

/**
* Represents the attribute not being set on the Cookie.
*/
case object Unset extends SameSite

}
@@ -0,0 +1,103 @@
package com.twitter.finagle.http.cookie

import com.twitter.finagle.http.Cookie
import com.twitter.finagle.stats.LoadedStatsReceiver
import com.twitter.logging.Logger
import io.netty.handler.codec.http.CookieDecoder
import java.lang.reflect.Method
import java.util.{ArrayList => AList, List => JList}

/**
* Encodes and decodes the SameSite Set-Cookie attribute to Strings and to
* Cookies, respectively.
*/
private[http] object SameSiteCodec {

private val log: Logger = Logger()
private val cookieDecoderClass = classOf[CookieDecoder]
private val initFailureCounter =
LoadedStatsReceiver.scope("http").scope("cookie").counter("samesite_failures")

private val _extractKeyValuePairs: Option[Method] = try {
val method = cookieDecoderClass.getDeclaredMethod(
"extractKeyValuePairs",
classOf[String],
classOf[JList[String]],
classOf[JList[String]]
)
method.setAccessible(true)
Some(method)
} catch {
case t: Throwable =>
log.error(t, "Failed to initialize `_extractKeyValuePairs`.")
None
}

/**
* Provides access to `CookieDecoder.extractKeyValuePairs` within Netty. It is
* a private static method which we change the visibility on in order to reuse
* for parsing the cookie header again. Cookie header parsing is error prone and
* not code we wish to rewrite.
*/
private def extractKeyValuePairs(
header: String,
names: JList[String],
values: JList[String]
): Unit = {
_extractKeyValuePairs match {
case Some(method) => method.invoke(null /* static method */, header, names, values)
case None => ()
}
}

/**
* Adds the `SameSite` cookie attribute to a header value which has already been encoded
* from an existing Finagle `Cookie`.
*/
def encodeSameSite(cookie: Cookie, encoded: String): String = cookie.sameSite match {
case SameSite.Lax => encoded + "; SameSite=Lax"
case SameSite.Strict => encoded + "; SameSite=Strict"
case _ => encoded
}

/**
* Decodes the `SameSite` cookie attribute and adds it to an existing Finagle
* `Cookie` value.
*/
def decodeSameSite(header: String, cookie: Cookie): Cookie = {
val names: AList[String] = new AList[String]()
val values: AList[String] = new AList[String]()

try {
extractKeyValuePairs(header, names, values)
} catch {
case t: Throwable =>
log.warning(t, "Failed to extract attributes from header.")
initFailureCounter.incr()
return cookie
}

var index: Int = 0
val len: Int = names.size()
var pos: Int = -1

while (index < len && pos == -1) {
if (names.get(index).equalsIgnoreCase("samesite")) {
pos = index
}
index += 1
}

if (pos <= -1) {
cookie
} else {
val sameSite =
if (values.get(pos).equalsIgnoreCase("lax")) SameSite.Lax
else if (values.get(pos).equalsIgnoreCase("strict")) SameSite.Strict
else SameSite.Unset

cookie.sameSite(sameSite)
}
}

}
@@ -0,0 +1,11 @@
package com.twitter.finagle.http.cookie.exp

import com.twitter.app.GlobalFlag

/**
* Enables / disables SameSite support in the CookieCodec.
*/
object supportSameSiteCodec extends GlobalFlag[Boolean](
false, // disabled by default
"Allow the SameSite attribute to be added to the Set-Cookie header on Responses"
)
@@ -1,6 +1,7 @@
package com.twitter.finagle.http.netty3

import com.twitter.finagle.http._
import com.twitter.finagle.http.cookie.SameSiteCodec
import com.twitter.finagle.http.netty3.Bijections.{cookieFromNettyInjection, cookieToNettyInjection}
import org.jboss.netty.handler.codec.http.{
CookieDecoder => NettyCookieDecoder,
Expand Down Expand Up @@ -29,17 +30,22 @@ private[http] object Netty3CookieCodec extends CookieCodec {
def encodeServer(cookie: Cookie): String = {
val encoder = new NettyCookieEncoder(true /* encode server-style cookies */)
encoder.addCookie(Bijections.from[Cookie, Netty3Cookie](cookie))
encoder.encode()
val encoded = encoder.encode()
if (CookieMap.includeSameSite) SameSiteCodec.encodeSameSite(cookie, encoded)
else encoded
}

def decodeClient(header: String): Option[Iterable[Cookie]] =
def decodeClient(header: String): Option[Iterable[Cookie]] = {
try {
Some(decoder.decode(header).asScala.map { cookie: Netty3Cookie =>
Bijections.from[Netty3Cookie, Cookie](cookie)
val decoded = Bijections.from[Netty3Cookie, Cookie](cookie)
if (CookieMap.includeSameSite) SameSiteCodec.decodeSameSite(header, decoded)
else decoded
})
} catch {
case e: IllegalArgumentException => None
}
}

def decodeServer(header: String): Option[Iterable[Cookie]] =
try {
Expand Down

0 comments on commit 4b0a58b

Please sign in to comment.