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

FISH-8545 SameSite Cookie #7199

Merged
merged 3 commits into from
Feb 21, 2025
Merged

Conversation

Viii3
Copy link
Member

@Viii3 Viii3 commented Feb 19, 2025

Description

  • Resolves FISH-8545
    • Makes Payara parse the payara-web.xml file correctly.
    • Adds SameSite to various cookie configurations.

Important Info

Blockers

Testing

New tests

Testing Performed

Tested against the reproducer.

Testing Environment

Maven version: 3.9.6
Java version: 11.0.23, vendor: Eclipse Adoptium
Default locale: en_GB, platform encoding: Cp1252
OS name: "windows 11", version: "10.0", arch: "amd64", family: "windows"

Documentation

Notes for Reviewers

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
@@ -636,7 +636,7 @@ protected void read(InputStream input) throws XMLStreamException {
versionIdentifier = parser.getElementText();
} else if (RuntimeTagNames.PAYARA_WHITELIST_PACKAGE.equals(name)) {
application.addWhitelistPackage(parser.getElementText());
} else if ("cookie-properties".equals(name)) {
} else if ("session-config".equals(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a constant value instead to compare literal value directly?

@@ -2072,6 +2072,8 @@ private void configureCookieProperties(CookieProperties bean) {
cookieConfig.setSecure(value);
} else if("cookieHttpOnly".equalsIgnoreCase(name)) {
cookieConfig.setHttpOnly(Boolean.valueOf(value));
} else if("cookieSameSite".equalsIgnoreCase(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably same here move the literal value to a constant and then compare

@@ -282,6 +293,10 @@ public String toString() {
sb.append(", domain=");
sb.append(_domain);
}
if (sameSite != null) {
sb.append(", sameSite=");
Copy link
Contributor

Choose a reason for hiding this comment

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

here also split more the append calls and use sameSite literal value on a constant

@breakponchito
Copy link
Contributor

@Viii3 I tested the cookie is there but when selecting any of the values from the console on the SameSite value it is always set strict either if you select none or lax from the admin console

image

@Viii3
Copy link
Member Author

Viii3 commented Feb 20, 2025

The cookie value in question is set on the application level, the admin console sets the value globally instead. The documentation does not specify, but I would presume the application settings are intended to override the global settings.

@breakponchito breakponchito self-requested a review February 20, 2025 17:00
@breakponchito
Copy link
Contributor

little comments but in general this is working as expected

@Viii3 Viii3 merged commit 0c05682 into payara:main Feb 21, 2025
1 check passed
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Feb 21, 2025
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Mar 3, 2025
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants