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.

Sign up for GitHub

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

Jump to bottom

SUP-14730: Use HttpOnly cookie flag #1598

Open
wants to merge 4 commits into
base: hotfix-1.10.x
Choose a base branch
from
Open

SUP-14730: Use HttpOnly cookie flag #1598

wants to merge 4 commits into from

Conversation

plyhun
Copy link
Contributor

@plyhun plyhun commented Mar 13, 2024 •
edited
Loading

Abstract

Potential XSS attacks prevention.

Checklist

General

  • Added abstract that describes the change
  • Added changelog entry to /CHANGELOG.adoc
  • Ensured that the change is covered by tests
  • Ensured that the change is documented in the docs

On API Changes

  • Checked if the changes are breaking or not
  • Added GraphQL API if applicable
  • Added Elasticsearch mapping if applicable

@plyhun plyhun requested review from npomaroli and yrucrem March 13, 2024 14:29
plyhun
plyhun commented Mar 13, 2024
View reviewed changes
common/src/main/java/com/gentics/mesh/auth/handler/MeshJWTAuthHandler.java
.setMaxAge(meshOptions.getAuthenticationOptions().getTokenExpirationTime()).setPath("/"));
context.response().removeCookie(SharedKeys.TOKEN_COOKIE_KEY);
context.response().addCookie(Cookie.cookie(SharedKeys.TOKEN_COOKIE_KEY, jwtToken)
.setHttpOnly(true)
Copy link
Contributor Author

@plyhun plyhun Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yrucrem
I'm not really sure if it can/should be parameterized. AFAIK our Java REST client ignores this setting. Did not try the PHP/JS clients though (Mesh UI expectedly works). Need an advice here, if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also not sure if it can be unit-tested from within Mesh.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to make this configurable, always enabling httpOnly should not be a problem (portal-java does this as well). As for tests: If there are already integration tests which check if the authentication cookie is set, the flag could be checked there. Otherwise I think manual checks suffice since this is not a feature on our part but Vert.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npomaroli npomaroli Awaiting requested review from npomaroli

@yrucrem yrucrem Awaiting requested review from yrucrem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plyhun @yrucrem