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

add setting ignore-illegal-header-for #687 #1942

Merged
merged 18 commits into from
May 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Don't monitor changes to internal API
ProblemFilters.exclude[Problem]("akka.http.impl.*")

# #1942 New ignoreIllegalHeaderFor setting
ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.settings.ParserSettings.getIgnoreIllegalHeaderFor")
ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.scaladsl.settings.ParserSettings.ignoreIllegalHeaderFor")
6 changes: 6 additions & 0 deletions akka-http-core/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,12 @@ akka.http {
# `error-logging-verbosity`.
illegal-header-warnings = on

# Sets the list of headers for which illegal values will *not* cause warning logs to be emitted;
#
# Adding a header name to this setting list disables the logging of warning messages in case an incoming message
# contains an HTTP header which cannot be parsed into its high-level model class due to incompatible syntax.
ignore-illegal-header-for = []

# Parse headers into typed model classes in the Akka Http core layer.
#
# If set to `off`, only essential headers will be parsed into their model classes. All other ones will be provided
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ private[http] object HttpHeaderParser {
def headerValueCacheLimit(headerName: String): Int
def customMediaTypes: MediaTypes.FindCustom
def illegalHeaderWarnings: Boolean
def ignoreIllegalHeaderFor: Set[String]
def illegalResponseHeaderValueProcessingMode: IllegalResponseHeaderValueProcessingMode
def errorLoggingVerbosity: ErrorLoggingVerbosity
def modeledHeaderParsing: Boolean
Expand Down Expand Up @@ -461,7 +462,7 @@ private[http] object HttpHeaderParser {

def defaultIllegalHeaderHandler(settings: HttpHeaderParser.Settings, log: LoggingAdapter): ErrorInfo ⇒ Unit =
if (settings.illegalHeaderWarnings)
info ⇒ logParsingError(info withSummaryPrepended "Illegal header", log, settings.errorLoggingVerbosity)
info ⇒ logParsingError(info withSummaryPrepended "Illegal header", log, settings.errorLoggingVerbosity, settings.ignoreIllegalHeaderFor)
else
(_: ErrorInfo) ⇒ _ // Does exactly what the label says - nothing

Expand Down Expand Up @@ -529,7 +530,7 @@ private[http] object HttpHeaderParser {
val header = parser(trimmedHeaderValue) match {
case HeaderParser.Success(h) ⇒ h
case HeaderParser.Failure(error) ⇒
onIllegalHeader(error.withSummaryPrepended(s"Illegal '$headerName' header"))
onIllegalHeader(error.withSummaryPrepended(s"Illegal '$headerName' header").withErrorHeaderName(headerName))
RawHeader(headerName, trimmedHeaderValue)
case HeaderParser.RuleNotFound ⇒
throw new IllegalStateException(s"Unexpected RuleNotFound exception for modeled header [$headerName]")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,16 @@ package object parsing {
}

private[http] def logParsingError(info: ErrorInfo, log: LoggingAdapter,
setting: ParserSettings.ErrorLoggingVerbosity): Unit =
setting match {
case ParserSettings.ErrorLoggingVerbosity.Off ⇒ // nothing to do
case ParserSettings.ErrorLoggingVerbosity.Simple ⇒ log.warning(info.summary)
case ParserSettings.ErrorLoggingVerbosity.Full ⇒ log.warning(info.formatPretty)
settings: ParserSettings.ErrorLoggingVerbosity,
ignoreHeaderNames: Set[String] = Set.empty): Unit =
settings match {
case ParserSettings.ErrorLoggingVerbosity.Off ⇒ // nothing to do
case ParserSettings.ErrorLoggingVerbosity.Simple ⇒
if (!ignoreHeaderNames.contains(info.errorHeaderName))
log.warning(info.summary)
case ParserSettings.ErrorLoggingVerbosity.Full ⇒
if (!ignoreHeaderNames.contains(info.errorHeaderName))
log.warning(info.formatPretty)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ private[akka] final case class ParserSettingsImpl(
uriParsingMode: Uri.ParsingMode,
cookieParsingMode: CookieParsingMode,
illegalHeaderWarnings: Boolean,
ignoreIllegalHeaderFor: Set[String],
errorLoggingVerbosity: ErrorLoggingVerbosity,
illegalResponseHeaderValueProcessingMode: IllegalResponseHeaderValueProcessingMode,
headerValueCacheLimits: Map[String, Int],
Expand Down Expand Up @@ -82,6 +83,7 @@ object ParserSettingsImpl extends SettingsCompanion[ParserSettingsImpl]("akka.ht
Uri.ParsingMode(c getString "uri-parsing-mode"),
CookieParsingMode(c getString "cookie-parsing-mode"),
c getBoolean "illegal-header-warnings",
(c getStringList "ignore-illegal-header-for").asScala.map(_.toLowerCase).toSet,
ErrorLoggingVerbosity(c getString "error-logging-verbosity"),
IllegalResponseHeaderValueProcessingMode(c getString "illegal-response-header-value-processing-mode"),
cacheConfig.entrySet.asScala.map(kvp ⇒ kvp.getKey → cacheConfig.getInt(kvp.getKey))(collection.breakOut),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ abstract class ParserSettings private[akka] () extends BodyPartParser.Settings {
def getUriParsingMode: Uri.ParsingMode
def getCookieParsingMode: ParserSettings.CookieParsingMode
def getIllegalHeaderWarnings: Boolean
def getIgnoreIllegalHeaderFor: Set[String]
def getErrorLoggingVerbosity: ParserSettings.ErrorLoggingVerbosity
def getIllegalResponseHeaderValueProcessingMode: ParserSettings.IllegalResponseHeaderValueProcessingMode
def getHeaderValueCacheLimits: ju.Map[String, Int]
Expand Down Expand Up @@ -64,6 +65,7 @@ abstract class ParserSettings private[akka] () extends BodyPartParser.Settings {
def withHeaderValueCacheLimits(newValue: ju.Map[String, Int]): ParserSettings = self.copy(headerValueCacheLimits = newValue.asScala.toMap)
def withIncludeTlsSessionInfoHeader(newValue: Boolean): ParserSettings = self.copy(includeTlsSessionInfoHeader = newValue)
def withModeledHeaderParsing(newValue: Boolean): ParserSettings = self.copy(modeledHeaderParsing = newValue)
def withIgnoreIllegalHeaderFor(newValue: List[String]): ParserSettings = self.copy(ignoreIllegalHeaderFor = newValue.map(_.toLowerCase).toSet)

// special ---

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,56 @@
package akka.http.scaladsl.model

import StatusCodes.ClientError
import akka.annotation.InternalApi

/**
* Two-level model of error information.
* The summary should explain what is wrong with the request or response *without* directly
* repeating anything present in the message itself (in order to not open holes for XSS attacks),
* while the detail can contain additional information from any source (even the request itself).
*/
final case class ErrorInfo(summary: String = "", detail: String = "") {
final class ErrorInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

please leave it as a case class, no reason to change IMHO; we should mark it as @InternalApi perhaps even

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.
I put case class back. and add annotation @InternalAPI as described below.

/**
 * INTERNAL API
 * Two-level model of error information.
 * The summary should explain what is wrong with the request or response *without* directly
 * repeating anything present in the message itself (in order to not open holes for XSS attacks),
 * while the detail can contain additional information from any source (even the request itself).
 */
@InternalApi private[akka] final case class ErrorInfo(summary: String = "", detail: String = "", errorHeaderName: String = "") {
  def withSummary(newSummary: String) = copy(summary = newSummary)
  def withSummaryPrepended(prefix: String) = withSummary(if (summary.isEmpty) prefix else prefix + ": " + summary)
  def withErrorHeaderName(headerName: String) = copy(errorHeaderName = headerName.toLowerCase)
  def withFallbackSummary(fallbackSummary: String) = if (summary.isEmpty) withSummary(fallbackSummary) else this
  def formatPretty = if (summary.isEmpty) detail else if (detail.isEmpty) summary else summary + ": " + detail
  def format(withDetail: Boolean): String = if (withDetail) formatPretty else summary
}

/** INTERNAL API */
@InternalApi private[akka] object ErrorInfo {
  /**
   * Allows constructing an `ErrorInfo` from a single string.
   * Used for example when catching exceptions generated by the header value parser, which doesn't provide
   * summary/details information but structures its exception messages accordingly.
   */
  def fromCompoundString(message: String): ErrorInfo = message.split(": ", 2) match {
    case Array(summary, detail)  apply(summary, detail)
    case _                       ErrorInfo("", message)
  }
}

I check binary compatible with mimaReportBinaryIssues sbt command.
I still receive a following error log.

[error]  * method copy(java.lang.String,java.lang.String)akka.http.scaladsl.model.ErrorInfo in class akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.copy")
[error]  * method this(java.lang.String,java.lang.String)Unit in class akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.this")
[error]  * method apply(java.lang.String,java.lang.String)akka.http.scaladsl.model.ErrorInfo in object akka.http.scaladsl.model.ErrorInfo does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.ErrorInfo.apply")

Should I add three method(copy, this, apply) to the mima-filters configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the check fail is expected and yes it should be added to the excludes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

akka-http-core/src/main/mima-filters/10.1.0.backwards.excludes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I understand.
I put case class back. And add some methods to 10.1.0.backwards.excludes.

val summary: String = "",
val detail: String = "",
val errorHeaderName: String = ""
) extends scala.Product with scala.Serializable with scala.Equals with java.io.Serializable {
def withSummary(newSummary: String) = copy(summary = newSummary)
def withSummaryPrepended(prefix: String) = withSummary(if (summary.isEmpty) prefix else prefix + ": " + summary)
def withErrorHeaderName(headerName: String) = new ErrorInfo(summary, detail, headerName.toLowerCase)
def withFallbackSummary(fallbackSummary: String) = if (summary.isEmpty) withSummary(fallbackSummary) else this
def formatPretty = if (summary.isEmpty) detail else if (detail.isEmpty) summary else summary + ": " + detail
def format(withDetail: Boolean): String = if (withDetail) formatPretty else summary

/** INTERNAL API */
@InternalApi private[akka] def copy(summary: String = summary, detail: String = detail): ErrorInfo = {
new ErrorInfo(summary, detail, errorHeaderName)
}

override def canEqual(that: Any): Boolean = that.isInstanceOf[ErrorInfo]

override def equals(that: Any): Boolean = that match {
case that: ErrorInfo ⇒ that.canEqual(this) && that.summary == this.summary && that.detail == this.detail && that.errorHeaderName == this.errorHeaderName
case _ ⇒ false
}

override def productElement(n: Int): Any = n match {
case 0 ⇒ summary
case 1 ⇒ detail
case 2 ⇒ errorHeaderName
}

override def productArity: Int = 3

/** INTERNAL API */
@InternalApi private[akka] def this(summary: String, detail: String) = this(summary, detail, "")
}

object ErrorInfo {
/** INTERNAL API */
@InternalApi private[akka] def apply(summary: String = "", detail: String = ""): ErrorInfo = new ErrorInfo(summary, detail, "")

def unapply(arg: ErrorInfo): Option[(String, String)] = Some((arg.summary, arg.detail))

/**
* Allows constructing an `ErrorInfo` from a single string.
* Used for example when catching exceptions generated by the header value parser, which doesn't provide
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ abstract class ParserSettings private[akka] () extends akka.http.javadsl.setting
def uriParsingMode: Uri.ParsingMode
def cookieParsingMode: ParserSettings.CookieParsingMode
def illegalHeaderWarnings: Boolean
def ignoreIllegalHeaderFor: Set[String]
def errorLoggingVerbosity: ParserSettings.ErrorLoggingVerbosity
def illegalResponseHeaderValueProcessingMode: ParserSettings.IllegalResponseHeaderValueProcessingMode
def headerValueCacheLimits: Map[String, Int]
Expand All @@ -55,6 +56,7 @@ abstract class ParserSettings private[akka] () extends akka.http.javadsl.setting
override def getMaxHeaderValueLength = maxHeaderValueLength
override def getIncludeTlsSessionInfoHeader = includeTlsSessionInfoHeader
override def getIllegalHeaderWarnings = illegalHeaderWarnings
override def getIgnoreIllegalHeaderFor = ignoreIllegalHeaderFor
override def getMaxHeaderNameLength = maxHeaderNameLength
override def getMaxChunkSize = maxChunkSize
override def getMaxResponseReasonLength = maxResponseReasonLength
Expand Down Expand Up @@ -88,6 +90,7 @@ abstract class ParserSettings private[akka] () extends akka.http.javadsl.setting
override def withIllegalHeaderWarnings(newValue: Boolean): ParserSettings = self.copy(illegalHeaderWarnings = newValue)
override def withIncludeTlsSessionInfoHeader(newValue: Boolean): ParserSettings = self.copy(includeTlsSessionInfoHeader = newValue)
override def withModeledHeaderParsing(newValue: Boolean): ParserSettings = self.copy(modeledHeaderParsing = newValue)
override def withIgnoreIllegalHeaderFor(newValue: List[String]): ParserSettings = self.copy(ignoreIllegalHeaderFor = newValue.map(_.toLowerCase).toSet)

// overloads for idiomatic Scala use
def withUriParsingMode(newValue: Uri.ParsingMode): ParserSettings = self.copy(uriParsingMode = newValue)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ import akka.http.scaladsl.model.headers._
import akka.http.impl.model.parser.CharacterClasses
import akka.http.impl.util._
import akka.http.scaladsl.settings.ParserSettings.IllegalResponseHeaderValueProcessingMode
import akka.testkit.TestKit
import akka.testkit.{ EventFilter, TestKit }

abstract class HttpHeaderParserSpec(mode: String, newLine: String) extends WordSpec with Matchers with BeforeAndAfterAll {

val testConf: Config = ConfigFactory.parseString("""
akka.event-handlers = ["akka.testkit.TestEventListener"]
akka.loglevel = ERROR
akka.loglevel = WARNING
akka.http.parsing.max-header-name-length = 60
akka.http.parsing.max-header-value-length = 1000
akka.http.parsing.header-cache.Host = 300""")
val system = ActorSystem(getClass.getSimpleName, testConf)
implicit val system = ActorSystem(getClass.getSimpleName, testConf)

s"The HttpHeaderParser (mode: $mode)" should {
"insert the 1st value" in new TestSetup(testSetupMode = TestSetupMode.Unprimed) {
Expand Down Expand Up @@ -254,6 +254,19 @@ abstract class HttpHeaderParserSpec(mode: String, newLine: String) extends WordS
parseAndCache(s"User-Agent: hmpf${newLine}x")(s"USER-AGENT: hmpf${newLine}x") shouldEqual RawHeader("User-Agent", "hmpf")
parseAndCache(s"X-Forwarded-Host: localhost:8888${newLine}x")(s"X-FORWARDED-Host: localhost:8888${newLine}x") shouldEqual RawHeader("X-Forwarded-Host", "localhost:8888")
}
"disables the logging of warning message when set the whitelist for illegal headers" in new TestSetup(
testSetupMode = TestSetupMode.Default,
parserSettings = createParserSettings(system).withIgnoreIllegalHeaderFor(List("Content-Type"))) {
//Illegal header is `Retry-After`. So logged warning message
EventFilter.warning(occurrences = 1).intercept {
parseLine(s"Retry-After: -10${newLine}x")
}

//Illegal header is `Content-Type` and it is in the whitelist. So not logged warning message
EventFilter.warning(occurrences = 0).intercept {
parseLine(s"Content-Type: abc:123${newLine}x")
}
}
}

override def afterAll() = TestKit.shutdownActorSystem(system)
Expand Down Expand Up @@ -284,13 +297,13 @@ abstract class HttpHeaderParserSpec(mode: String, newLine: String) extends WordS
case TestSetupMode.Default ⇒ HttpHeaderParser(parserSettings, system.log)
}

private def defaultIllegalHeaderHandler = (info: ErrorInfo) ⇒ system.log.warning(info.formatPretty)
private def defaultIllegalHeaderHandler = (info: ErrorInfo) ⇒ system.log.debug(info.formatPretty)

def insert(line: String, value: AnyRef): Unit =
if (parser.isEmpty) HttpHeaderParser.insertRemainingCharsAsNewNodes(parser, ByteString(line), value)
else HttpHeaderParser.insert(parser, ByteString(line), value)

def parseLine(line: String) = parser.parseHeaderLine(ByteString(line))() → { system.log.warning(parser.resultHeader.getClass.getSimpleName); parser.resultHeader }
def parseLine(line: String) = parser.parseHeaderLine(ByteString(line))() → { system.log.debug(parser.resultHeader.getClass.getSimpleName); parser.resultHeader }

def parseAndCache(lineA: String)(lineB: String = lineA): HttpHeader = {
val (ixA, headerA) = parseLine(lineA)
Expand Down