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

ExternalContext.addResponseCookie does not support different properties #5165

Closed
NicolaIsotta opened this issue Nov 9, 2022 · 5 comments
Closed

Comments

@NicolaIsotta
Copy link
Contributor

NicolaIsotta commented Nov 9, 2022

Describe the bug
I've added SameSite to my app cookies using Tomcat Cookie Processor. In some cases (eg. PrimeFaces filedownload) the page breaks beacuse of an IllegalArgumentException thrown by mojarra ExternalContextImpl at this line:

PREDEFINED_COOKIE_PROPERTIES p = PREDEFINED_COOKIE_PROPERTIES.valueOf(key);

To Reproduce

Steps to reproduce the behavior:
1. Add <CookieProcessor sameSiteCookies="strict" /> to the context.xml of your app
2. Deploy to Tomcat 10
3. Navigate to a page which has p:fileDownload
4. Try to download the file

Expected behavior
No exception.

Screenshots
cookie error

Additional context
Tomcat 10.0.20
mojarra 4.0.0
primefaces 12.0.0

@NicolaIsotta NicolaIsotta changed the title ExternalContext.addResponseCookie does not support different values ExternalContext.addResponseCookie does not support different properties Nov 9, 2022
@melloware
Copy link
Contributor

melloware commented Nov 9, 2022

@NicolaIsotta here is what PrimeFaces code does... Not sure if this helps at all.

        PrimeRequestContext requestContext = PrimeRequestContext.getCurrentInstance(context);
        PrimeApplicationContext applicationContext = requestContext.getApplicationContext();

        if (requestContext.isSecure() && applicationContext.getConfig().isCookiesSecure()) {
            properties.put("secure", true);

            if (applicationContext.getEnvironment().isAtLeastJsf40()) {
                properties.put("SameSite", applicationContext.getConfig().getCookiesSameSite());
            }
        }

        context.getExternalContext().addResponseCookie(name, value, properties);

@NicolaIsotta
Copy link
Contributor Author

Yeah, I just discovered that Tomcat Cookie Processor is not involved at all. In fact disabling it does not fix the issue.

@BalusC seems like PR #4991 does not work: the default case of the switch cannot be reached since an IllegalArgumentException is thrown by PREDEFINED_COOKIE_PROPERTIES.valueOf

@NicolaIsotta
Copy link
Contributor Author

Aside from that, there's another problem.
I see from the readme and from the specification document that Faces 4 targets Servlet 5:

mojarra/README.md

Lines 17 to 27 in e651989

# Mojarra 4.0 WIP
Eclipse's implementation of the upcoming Jakarta Faces 4.0 specification
For Mojarra / JSF 2.3 please have a look at https://github.com/eclipse-ee4j/mojarra/blob/2.3/README.md.
For Mojarra / JSF 3.0 please have a look at https://github.com/eclipse-ee4j/mojarra/blob/3.0/README.md.
## Minimum Requirements
- Java 11
- Jakarta Servlet 5.0

But mojarra 4 targets servlet 6 api in the pom:

mojarra/impl/pom.xml

Lines 51 to 56 in e651989

<dependency>
<groupId>jakarta.servlet</groupId>
<artifactId>jakarta.servlet-api</artifactId>
<version>6.0.0</version>
<scope>provided</scope>
</dependency>

So what's the correct version?
Because Servlet 5 does not have Cookie.setAttribute so even if this is fixed it would fail because it cannot find the method in Tomcat 10.0.x or other Servlet 5 containers.

@BalusC
Copy link
Contributor

BalusC commented Nov 9, 2022

Excellent find. This badly slipped through because there's no TCK test for that.

@BalusC
Copy link
Contributor

BalusC commented Nov 12, 2022

Reminder/note to self: TCK couldn't be created because currently used GF 7.0.0-M4 hasn't it implemented yet and because newer milestones are not in Maven Central due to dependency issues. We need to wait until GF 7.0.0 is released.

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

No branches or pull requests

3 participants