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

csrf: cookie settings + builder + `createResponseCookie` usage #2228

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
5 participants
@ahjohannessen
Contributor

ahjohannessen commented Oct 31, 2018

  • fix checkCSRFToken to use createResponseCookie.
  • make it possible to configure various relevant settings
    for csrf cookie.
  • add builder for CSRF
  • add ability to read token from form
@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Oct 31, 2018

@jmcardon Fix for the bug you mentioned in #2213

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 31, 2018

I don't know CSRF very well, but I stumbled across this discussion. Does that discussion ring true to people who know what they're taking about?

@jmcardon

This comment has been minimized.

Member

jmcardon commented Oct 31, 2018

@rossabaker that's correct. I was actually wrong about this. I traditionally only had it set up to send the csrf token in certain locations, but this is correct.

I think better yet would be making this field optional. The csrf token being accessible from js is sort of a necessity anyway @ some point.

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 1, 2018

@jmcardon @rossabaker Updated the PR by adding CSRFCookieSettings. WDYT?

@jarreds

This comment has been minimized.

jarreds commented Nov 1, 2018

I like this setup. Was looking to add SameSite=strict to the cookie and this would allow that.

@rossabaker

I like the approach. See comment below.

@@ -244,6 +247,24 @@ final class CSRF[F[_], G[_]] private[middleware] (
object CSRF {
final case class CSRFCookieSettings(

This comment has been minimized.

@rossabaker

rossabaker Nov 1, 2018

Member

Are we fairly certain that nothing will be added to this in the future? I'm trying to move away from case classes for config for binary compatibility reasons. My altnernative has been the server and client builders, which are painful to write, but can support additions in the future without breaking code.

This comment has been minimized.

@jarreds

jarreds Nov 1, 2018

+1 on builders. Started on a (much less thorough) builder-like addition here #2235 until i discovered this PR.

This comment has been minimized.

@ahjohannessen

ahjohannessen Nov 1, 2018

Contributor

I don’t think much more is needed wrt cookie settings. Are you thinking about a builder for the middleware itself @rossabaker ?

This comment has been minimized.

@rossabaker

rossabaker Nov 2, 2018

Member

I was thinking a builder for the config, but I suppose it could be a builder for the middleware. We merged configuration and builder in the server and client builders. I haven't fully thought through the ergonomics of it for middleware. 🤔

  • CSRF(http): defaults
  • CSRF.withCookieName("x-csrf-token").withSecure(true)(http)

So CSRF is a class that is Http => Http. Object (or value) CSRF is CSRF with default values. And you can configure things until applying the http.

This pattern could apply to a few middlewares, and people are antsy for 0.20. Maybe we roll with it as is, and introduce this refactoring before 1.0.

@ahjohannessen ahjohannessen force-pushed the ahjohannessen:wip-fix-csrf-cookie branch from 152aac5 to 93e35d7 Nov 2, 2018

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 2, 2018

According to this site, apparently:

Same-Site cookies completely and effectively neutralise CSRF attacks.

@rossabaker wrt nothing being added in the future, these two are missing:

expires: Option[HttpDate] = None,
maxAge: Option[Long] = None,

I do not see how those are relevant.

@ahjohannessen ahjohannessen changed the title from middleware: fix `checkCSRFToken` wrt. `httpOnly` to csrf: cookie settings & `createResponseCookie` usage Nov 2, 2018

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 2, 2018

@rossabaker Would it be useful to add this as a middleware:

/** [[Middleware]] for lifting application/x-www-form-urlencoded values
  * into the request header values. This middleware is particular useful
  * for scenarios where you use some middleware, e.g. `CSRF`, and need
  * to check for a header value that is only available as a form value.
  * */
object UrlFormToHeader {

  type FieldName  = String
  type HeaderName = String

  /*
   * Form fields listed in `fieldNames` are attempted to be lifted into headers.
   * It does not try to replace headers that already exist with same name.
   * */
  def apply[F[_]: Sync, G[_]: Sync](nt: G ~> F)(
    fieldNames: NonEmptyList[String],
    fieldToHeader: FieldName  HeaderName = identity,
    checkRequest: Request[F]  Boolean = !_.method.isSafe,
    strictDecode: Boolean = false
  ): Http[F, G]  Http[F, G] =
    http 
      Kleisli { req 
        def fieldsToHeaders(form: UrlForm): F[Response[G]] = {

          val relevantFormValues = form.values.filterKeys(fieldNames.contains_).toList
          val lifted = relevantFormValues.flatMap {
            case (k, vs)  vs.headOption.map(v  Header(fieldToHeader(k), v))
          }

          val originalHeaders = req.headers.toList
          val filteredHeaders = lifted.filterNot(l  originalHeaders.exists(_.name === l.name))
          val newRequest      = req.putHeaders(filteredHeaders: _*)

          http(newRequest)
        }

        req.headers.get(headers.`Content-Type`) match {
          case Some(headers.`Content-Type`(MediaType.application.`x-www-form-urlencoded`, _)) if checkRequest(req) 
            for {
              decoded  nt(UrlForm.entityDecoder[G].decode(req, strictDecode).value)
              resp     decoded.fold(mf  nt(mf.toHttpResponse[G](req.httpVersion)), fieldsToHeaders)
            } yield resp

          case _  http(req)
        }
    }
}

This means that one could to this:

def csrf(routes: HttpRoutes[F]) = {
 val ufthM = UrlFormToHeader(NonEmptyList.one("X-Csrf-Token"))
 val csrfM = ???
 utfhM(csrfM.validate()(routes))
} 

val wrapped = csrf(routes1 <+> routes2)
@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 2, 2018

wrt nothing being added in the future, these two are missing... I do not see how those are relevant.

I think that's true. I'd like to futureproof it eventually, but I'm comfortable doing that for 1.0.

Would UrlFormToHeader be useful to add this as a middleware:

The idea of "a header value that is only available as a form value" is strange to me. I've seen more people conflate query strings and url forms than headers and url forms. But it seems you've got a real-world use case, and I don't object to adding it if it simplifies things.

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 2, 2018

To be honest @rossabaker I only have a specific need wrt. CSRF. It can be solved similar to this:

  def filter2(predicate: Request[G]  Boolean = _.method.isSafe,
              r: Request[G],
              http: Http[F, G],
              f: G ~> F): F[Response[G]] =
    if (predicate(r)) {
      validate(r, http)
    } else {
      checkCSRFDefaultV2(r, http, f)
    }

  def validate2(predicate: Request[G]  Boolean = _.method.isSafe,
                f: G ~> F): Middleware[F, Request[G], Response[G], Request[G], Response[G]] = { http 
    Kleisli { r: Request[G] 
      filter2(predicate, r, http, f)
    }
  }

  def checkCSRFDefaultV2(r: Request[G], http: Http[F, G], fk: G ~> F): F[Response[G]] = {
    for {
      ht  F.pure(r.headers.get(headerName).map(_.value))
      ft  if (ht.isDefined) F.pure(ht) else {
            fk(r.as[UrlForm].map(_.values.get("X-TSec-Csrf").flatMap(_.headOption)))
           }
      r  ft match {
           case Some(t)  checkCSRFToken(r, http, t)
           case None     F.pure(onFailure)
         }
    } yield r
  }

in user code.

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 2, 2018

@rossabaker Perhaps adding something like

def checkAlsoForm(fieldName: String, nt: G ~> F): CSRF[F, G]

Would be a good compromise and helpful for people using twirl?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 2, 2018

Yeah, that could be good.

@ahjohannessen ahjohannessen force-pushed the ahjohannessen:wip-fix-csrf-cookie branch from 93e35d7 to 0f4b15c Nov 6, 2018

@ahjohannessen ahjohannessen changed the title from csrf: cookie settings & `createResponseCookie` usage to csrf: cookie settings + builder + `createResponseCookie` usage Nov 6, 2018

@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 6, 2018

@rossabaker I took at stab at using a builder approach, WDYT?

@ahjohannessen ahjohannessen force-pushed the ahjohannessen:wip-fix-csrf-cookie branch 7 times, most recently from b1c69d5 to 500fbc0 Nov 6, 2018

csrf: cookie settings & `createResponseCookie` usage
 - fix `checkCSRFToken` to use `createResponseCookie`.
 - make it possible to configure various relevant settings
   for csrf cookie.
 - add builder for `CSRF`.
 - add ability to read token from form in `CSRF`.

@ahjohannessen ahjohannessen force-pushed the ahjohannessen:wip-fix-csrf-cookie branch from 500fbc0 to 5c9c2ba Nov 9, 2018

}
for {
fst <- F.pure(csrf.getHeaderToken(r))

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 9, 2018

Member

Why is the pure necessary here, seems this should just be on the next line

This comment has been minimized.

@ahjohannessen

ahjohannessen Nov 9, 2018

Contributor

Not sure why I did that, perhaps because a fold resulted in v => F.pure(Some(v)) - If you have a more elegant approach please share:) on the phone now

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 10, 2018

Member

You're right, all of them I know of would involve at least one thunk. Here's hoping that someone else has a clean idea.

This comment has been minimized.

@rossabaker

rossabaker Nov 13, 2018

Member

A pure generator that begins a for-comprehension can move outside:

val fst = F.pure(csrf.getHeaderToken(r))
for {
  snd <- if(fst.isDefined) F.pure(fst) else getFormToken

I don't think this is a dealbreaker to getting this merged and fixed.

}
for {
fst <- F.pure(csrf.getHeaderToken(r))

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 10, 2018

Member

You're right, all of them I know of would involve at least one thunk. Here's hoping that someone else has a clean idea.

}
for {
fst <- F.pure(csrf.getHeaderToken(r))

This comment has been minimized.

@rossabaker

rossabaker Nov 13, 2018

Member

A pure generator that begins a for-comprehension can move outside:

val fst = F.pure(csrf.getHeaderToken(r))
for {
  snd <- if(fst.isDefined) F.pure(fst) else getFormToken

I don't think this is a dealbreaker to getting this merged and fixed.

@rossabaker rossabaker merged commit 2456776 into http4s:master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ahjohannessen

This comment has been minimized.

Contributor

ahjohannessen commented Nov 13, 2018

👍

@ahjohannessen ahjohannessen deleted the ahjohannessen:wip-fix-csrf-cookie branch Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment