Handle bad signature on flash cookie like a missing signature

[cies]

Why do we need two? Currently the ForbiddenException is thrown when the flash signature is incorrect. This is clearly not a 5xx error, but ForbiddenException is handled like a 5xx error (errors in the logs and a 500 response).

I think this is a Forbidden or a BadRequest.

Also I do not understand why we'd need both a ForbiddenException and a Forbidden exception. If there is a good reason this could be put in a code comment for future reference.

[asolntsev]

@cies I agree, we should remove ForbiddenException, and use Forbidden instead.

[cies]

Say no more @asolntsev , I'll produce a PR for this. :)

[cies]

While implementing this I stumbled on this:

  @Nonnull
  public Scope.Flash restore(@Nonnull Http.Request request) {
      Http.Cookie cookie = request.cookies.get(COOKIE_PREFIX + "_FLASH");
      if (cookie == null) {
        return new Scope.Flash();
      }
      int splitterPosition = cookie.value.indexOf('-');
      if (splitterPosition == -1) {
          logger.warn("Cookie without signature: {}", cookie.value);
        return new Scope.Flash();
      }

      String signature = cookie.value.substring(0, splitterPosition);
      String realValue = cookie.value.substring(splitterPosition + 1);
      if (!signer.isValid(signature, realValue)) {
          throw new Forbidden(String.format("Invalid flash signature: %s", cookie.value));
      }
      return new Scope.Flash(encoder.decode(realValue));
  }

Basically if you remove the signature: it passes with a warning in the logs, but you get an empty flash object back. While if you have the signature wrong you get a now 500 error (and according to the to-be-produced PR a Forbidden error). This is unjust to the point that I think it's better to let both log a message as warning AND let both return an empty flash object, fair and square.

We get about 10 Invalid flash signature logs messages per day (from our prod logs) and about one mvc.FlashStore Cookie without signature per day.

I have made a PR that handles bad signatures, as it handles missing signatures.

[asolntsev]

@cies I would say that it's better to throw an exception in both cases. It's safer.

I think we left just a log only because of backward compatibility.

1. Before some moment, Play didn't sign flash cookie
2. At some moment, we added signing the flash cookie, but
   2.1. We were afraid that there might be tons of users who already have unsigned flash cookie in their browsers.

So it was just a transition period.
Nowadays all the users already have a signed flash cookie, and we can strictly check the cookie signature.

[cies]

Ok, will change that. Did not know the history.

[cies]

Fixed it. Both cases.