Skip to content
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

Don't include sensitive user data in `toString` output #2409

Open
pfcoperez opened this issue Jan 30, 2019 · 3 comments · May be fixed by #2560

Comments

@pfcoperez
Copy link

commented Jan 30, 2019

There are certain environments where it is easy to either print, write or log HttpRequest or HttpResponse instances (for example when side-effecting in the passed function when using directives such as mapRequest or mapRequestContext).

Both of these classes override toString as:

override def toString = s"""HttpRequest(${_1},${_2},${_3},${_4},${_5})"""

and

override def toString = s"""HttpResponse(${_1},${_2},${_3},${_4})"""

respectively. This means that the contents of the headers, for example, could inadvertently leak into log files. For some industrial environments this poses legal risks.

This issue aims to suggest adding a template method to HttpMessage to allow customizing how toString behaves in each case. Surely, defaulting to the above listed implementations.

For example:

/**
 * Common base class of HttpRequest and HttpResponse.
 */
sealed trait HttpMessage extends jm.HttpMessage {
  type Self <: HttpMessage { type Self = HttpMessage.this.Self }
  def self: Self

  def isRequest: Boolean
  def isResponse: Boolean

  def headers: immutable.Seq[HttpHeader]
  def entity: ResponseEntity
  def protocol: HttpProtocol
  def stringRepresentation: Self  String // <---------------------  NEW

  override def toString: String = stringRepresentation(self) // <---------------------  NEW

  /** Returns a copy of this message where `f` is used to generate its string representation. **/
  def withStringRepresentation(f: Self  String): Self  // <---------------------  NEW

...

And

/**
 * The immutable model HTTP request model.
 */
final class HttpRequest(
  val method:   HttpMethod,
  val uri:      Uri,
  val headers:  immutable.Seq[HttpHeader],
  val entity:   RequestEntity,
  val protocol: HttpProtocol,
  val stringRepresentation: HttpRequest  String = { request  // <---------------------  NEW
    import request._
    s"""HttpRequest(${_1},${_2},${_3},${_4},${_5})"""
  }
) extends jm.HttpRequest with HttpMessage {

  HttpRequest.verifyUri(uri)
  require(entity.isKnownEmpty || method.isEntityAccepted, s"Requests with method '${method.value}' must have an empty entity")
  require(
    protocol != HttpProtocols.`HTTP/1.0` || !entity.isChunked,
    "HTTP/1.0 requests must not have a chunked entity")

  /** Returns a copy of this message where `f` is used to generate its string representation. **/
  override def withStringRepresentation(f: HttpRequest  String): HttpRequest = 
    copy(stringRepresentation = f) // <---------------------  NEW

// This class `toString` override disappears // <---------------------  NEW

I also considered the possibility of using

@InternalApi
private[http] trait Renderer[-T] {
  def render[R <: Rendering](r: R, value: T): r.type
}

Instead of a function from the object to string but refrained as it seems that's not intended to be part of the DSL. Besides, I find the function more comfortable for the exposed purpose.

I don't mind opening a PR including these changes as I have it compiling in my local clone.

@raboof

This comment has been minimized.

Copy link
Member

commented May 28, 2019

As discussed in the PR: I'm not sure the extra complexity to make the toString configurable is worth the advantage. An alternative approach might be to trim down the default toString implementation, and recommend users to do their own formatting if they want to see request details. /cc @jrudolph @jypma @jlprat WDYT?

@jlprat

This comment has been minimized.

Copy link
Member

commented May 28, 2019

My vote would be to trim toString and then document how you could write your own formatting with some working examples.

@pfcoperez

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Thank you @raboof @jlprat for your comments, suggestions and considering this issue. I've just opened a PR with the trimming by default approach: #2560

It also includes the documentation changes to add documentation about the usage of type classes to change this default behaviour.

@jrudolph jrudolph changed the title `HttpMessage#withStringRepresentation` Don't include sensitive user data in `toString` output Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.