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

server.servlet.session.cookie.same-site isn't applied to Spring Session's SESSION cookie #28772

Closed
wilkinsona opened this issue Nov 22, 2021 · 6 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@wilkinsona
Copy link
Member

wilkinsona commented Nov 22, 2021

As reported by @OrangeDog on Gitter, there's an unfortunate mismatch between Servlet's default cookie name (JSESSIONID) and Spring Session's default cookie name (SESSION). This mismatch means that the server.servlet.session.cookie.same-site property has no effect when using Spring Session. I think that setting server.servlet.session.cookie.name=SESSION will gets things working. We should confirm that this is the case and also see if there's something that we can do so that this works out of the box.

@wilkinsona wilkinsona added type: bug A general bug for: team-meeting An issue we'd like to discuss as a team to make progress labels Nov 22, 2021
@wilkinsona wilkinsona added this to the 2.6.x milestone Nov 22, 2021
@OrangeDog
Copy link
Contributor

Setting server.servlet.session.cookie.name=SESSION does not appear to have any effect.

@wilkinsona wilkinsona changed the title Spring Session's default cookie name is SESSION but SameSite support uses JSESSIONID by default server.servlet.session.cookie.same-site isn't applied to Spring Session's SESSION cookie Nov 22, 2021
@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 22, 2021
@wilkinsona
Copy link
Member Author

Thanks for trying that out, @OrangeDog. It doesn't work as I misdiagnosed the cause here. It's actually simpler than I thought and we just need to map the property onto Spring Session's DefaultCookieSerializer. You can do that yourself as a workaround in the meantime by defining the following bean in your application:

@Bean
DefaultCookieSerializer cookieSerializer(ServerProperties serverProperties,
        ObjectProvider<DefaultCookieSerializerCustomizer> cookieSerializerCustomizers) {
    Cookie cookie = serverProperties.getServlet().getSession().getCookie();
    DefaultCookieSerializer cookieSerializer = new DefaultCookieSerializer();
    PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull();
    map.from(cookie::getName).to(cookieSerializer::setCookieName);
    map.from(cookie::getDomain).to(cookieSerializer::setDomainName);
    map.from(cookie::getPath).to(cookieSerializer::setCookiePath);
    map.from(cookie::getHttpOnly).to(cookieSerializer::setUseHttpOnlyCookie);
    map.from(cookie::getSecure).to(cookieSerializer::setUseSecureCookie);
    map.from(cookie::getMaxAge).asInt(Duration::getSeconds).to(cookieSerializer::setCookieMaxAge);
    map.from(cookie::getSameSite).as(SameSite::attributeValue).to(cookieSerializer::setSameSite);
    cookieSerializerCustomizers.orderedStream().forEach((customizer) -> customizer.customize(cookieSerializer));
    return cookieSerializer;
}

@OrangeDog
Copy link
Contributor

That’s what I already do, but with @ConfigurationProperties.

@vpavic
Copy link
Contributor

vpavic commented Nov 22, 2021

@wilkinsona I just saw this issue and wanted to leave the same comment as you did in the meantime. Anyway, if no one is working on this, I prepared the branch with changes to address this so I can proceed to submit the PR fairly soon. But this could also be a nice issue for first-timers so I'll let you decide.

vpavic added a commit to vpavic/spring-boot that referenced this issue Nov 23, 2021
…lizer

This commit adds the mapping of `server.servlet.session.cookie.same-site` configuration property to `DefaultCookieSerializer` bean configured in the Spring Session auto-configuration.

See spring-projectsgh-28772
@vpavic
Copy link
Contributor

vpavic commented Nov 23, 2021

As there was no feedback on the previous comment, I've opened #28784 to address this.

@philwebb
Copy link
Member

Closing in favor of PR #28784. Thanks @vpavic!

@philwebb philwebb added status: superseded An issue that has been superseded by another and removed type: bug A general bug labels Nov 29, 2021
@philwebb philwebb removed this from the 2.6.x milestone Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

4 participants