addCookie in HttpServletResponse not working correctly about aws-serverless-java-container

[nisbshah]

I believe the key thing to note in the above CustomCookie is why is it adding Expires= !? Max-Age=-1 means cookie will expire when browsing session ends. Where as for some reason, the above mentioned cookie gets the expiry time as the current time! And that is the reason it expires the cookie as soon as it sets it.

[sapessi]

Verified this is a bug. Addressing it now in the servlet-improvements branch

[sapessi]

Haven't heard back on this. Unit tests seem to be working fine so resolving.

[nisbshah]

Hi @sapessi

Sorry, could not test this earlier.
Things have got worse after I pulled 0.6 version. Now it's NOT setting up the cookie at all with following code.
((HttpServletResponse) response).addCookie(ck);

[sapessi]

Hey @nisbshah, Let me re-open this. The code for the addCookie method is pretty straightforward. Are you checking in the response from API Gateway or on the return value from your Lambda function?

Any chance you have some sample code I can try and use to replicate?

I've just added a unit test to the Spark module and I can get the cookie. Method configuration:

get("/cookie", (req, res) -> {
    Cookie testCookie = new Cookie(COOKIE_NAME, COOKIE_VALUE);
    testCookie.setDomain(COOKIE_DOMAIN);
    testCookie.setPath(COOKIE_PATH);
    res.raw().addCookie(testCookie);
    return BODY_TEXT_RESPONSE;
});

The test checks this:

AwsProxyRequest req = new AwsProxyRequestBuilder().method("GET").path("/cookie").build();
AwsProxyResponse response = handler.proxy(req, new MockLambdaContext());

assertEquals(200, response.getStatusCode());
assertTrue(response.getHeaders().containsKey(HttpHeaders.SET_COOKIE));
assertTrue(response.getHeaders().get(HttpHeaders.SET_COOKIE).contains(COOKIE_NAME + "=" + COOKIE_VALUE));
assertTrue(response.getHeaders().get(HttpHeaders.SET_COOKIE).contains(COOKIE_DOMAIN));
assertTrue(response.getHeaders().get(HttpHeaders.SET_COOKIE).contains(COOKIE_PATH));

[nisbshah]

I am checking the response from Api gateway (Lamda backend). Also, Just to point out, I am using aws-Serverless-java-container-Spring.

following is my code to set cookie

public static void setCookie(ServletRequest request, ServletResponse response, String name, String value,
      boolean set, boolean global, boolean bSecureCookie, Integer maxAge, boolean httpOnly) {
    Cookie ck = new Cookie(name, value);

    HttpServletRequest httpRequest = (HttpServletRequest) request;
    setRootDomain(httpRequest);

    if (httpOnly) {
      ck.setHttpOnly(true);
    }

    if (set) {
      LOGGER.info("Setting cookie {} for the domain {} ", name, rootDomain);
      if (maxAge != null) {
        ck.setMaxAge(maxAge.intValue());
      } else {
        ck.setMaxAge(-1);
      }
    } else {
      LOGGER.info("Deleting cookie {} for the domain {} ", name, rootDomain);
      ck.setMaxAge(0);
    }
    ck.setPath("/");

    // for local and fngn envs., we should not set cookie as a secure cookie
    if (bSecureCookie) {
      ck.setSecure(true);
    }

    if (Utils.goodString(rootDomain) && global) {
      LOGGER.info("rootDomain : {}", rootDomain);
      ck.setDomain(rootDomain);
    }

    ((HttpServletResponse) response).addCookie(ck);
  }

Let me know if you have more questions.

[sapessi]

Thanks @nisbshah - let me test with Spring. Is this a static method you call from a method annotated with @RequestMapping?

[nisbshah]

yes!

[sapessi]

Hey @nisbshah, I'm struggling to replicate. This is my test: I have added your static method (verbatim) and added a new mapped method that looks like this:

@RequestMapping(path = "/cookie", method=RequestMethod.GET)
public SingleValueModel setCookie(ServletRequest request, ServletResponse response) {
    setCookie(request, response, COOKIE_NAME, COOKIE_VALUE, true, false, true, null, false);
    return new SingleValueModel();
}

This is the unit test I ran against it and it succeeds:

@Test
    public void cookie_injectInResponse_expectCustomSetCookie() {
        AwsProxyRequest request = new AwsProxyRequestBuilder("/context/cookie", "GET")
                                          .stage(STAGE)
                                          .header(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE)
                                          .build();

        AwsProxyResponse output = handler.proxy(request, lambdaContext);

        assertEquals(200, output.getStatusCode());
        assertTrue(output.getHeaders().containsKey(HttpHeaders.SET_COOKIE));
        assertTrue(output.getHeaders().get(HttpHeaders.SET_COOKIE).contains(ContextResource.COOKIE_NAME + "=" + ContextResource.COOKIE_VALUE));
        assertTrue(output.getHeaders().get(HttpHeaders.SET_COOKIE).contains(ContextResource.COOKIE_DOMAIN));
}

I get the expected cookie in the headers map:
CustomCookie=CookieValue; Path=/; Secure; Domain=mydomain.com; Max-Age=-1; Expires=Mon, 14 Aug 2017 20:43:05 GMT

Where do you get the ServletResponse parameter you pass to the method from?
Are you trying to set multiple cookies?

[sapessi]

Thanks @nisbshah, got you now. So the Set-Cookie header is returned, but the value is wrong. The reason I was setting both is because I expected Max-Age to take precedence as described here:

Max-Age: Number of seconds until the cookie expires. One or more digits 1 through 9. Older browsers (ie6, ie7, and ie8) do not support max-age. For other browsers, if both (Expires and Max-Age) are set, Max-Age will have precedence.

I can change the logic to skip the Expires field if Max-Age is set to -1, would this work?

[nisbshah]

I would recommend leaving Expires field altogether. Anyways, I believe it's deprecated in the latest versions.
Also, if no value is provided to the max-age it defaults to -1
private int maxAge = -1;
Above LOC is from Cookie class in Javax.servlet

[sapessi]

Thanks @nisbshah, I want to make sure that older browsers are supported too. If the Max-Age is -1, I skip the expire header altogether. If Max-Age is > 0, I calculate the expire header to match the max age. Pushing a commit of these changes now to the servlet-improvements brach, let me know if you get a chance to test this.

[sapessi]

I've made an additional fix to this: Max-Age should not be returned at all if the value is negative. I've tested this running inside Lambda from a browser and the cookie is set correctly. Change is in servlet-improvements branch and I will merge it today.

[nisbshah]

I got the latest code and tested.. its working as expected! Thank you :)
Sorry for the delayed reply.

[nisbshah]

@sapessi I have found one more issue with addCookie() in the AwsHttpServletResponse class..
While trying to set multiple cookies, addCookie() just keeps on appending all cookies to the same "Set-Cookie" header. Due to which, Browser only sets the 1st cookie in the list and ignores all others. While setting multiple cookies, each cookie should be set by a separate response header.
For Eg:
Set-Cookie:session=tst1basf021jb23zck3wo; Domain=feitest.com; Path=/; Secure; HttpOnly
Set-Cookie:ticket=1318245we514383407312883722; Domain=feitest.com; Path=/; Secure; HttpOnly
Set-Cookie:authType=legacy; Domain=feitest.com; Path=/
here is the specification: https://tools.ietf.org/html/rfc6265#section-4.1.2

Also, with Api gateway Lambda setting multiple headers with the same name will NOT work. https://forums.aws.amazon.com/thread.jspa?threadID=205782
there are a couple of hacks mentioned in the about link but they are not scalable.

Let me know your recommendation and hope you can push for the fix being an insider in AWS. ;)

[sapessi]

Hey @nisbshah unfortunately this is behavior I had to hard-code in the library. API Gateway expects headers to be a plain map (only one Set-Cookie key in the response). Even if we were to expose the cookies as a multi-valued map in the library, API Gateway would only pick the first one.

[nisbshah]

Thank you for your response.. Will wait for the AWS to fix the root issue.