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

JAVASERVERFACES-3531 didn't correct all uses of getExternalContext().isSecure() #4078

Closed
ren-zhijun-oracle opened this issue Oct 6, 2015 · 15 comments
Milestone

Comments

@ren-zhijun-oracle
Copy link
Contributor

It seems that the fix applied in #3535 didn't correct all uses of getExternalContext().isSecure(). In particular, it's still called from elsewhere in ELFlash#setCookie [1].

Please fix the bug, because a Red Hat customer needs this issue to be resolved.

[1] https://github.com/jboss/mojarra/blob/8111d0950fa9a1bb2e48934bae76a466f474b552/jsf-ri/src/main/java/com/sun/faces/context/flash/ELFlash.java#L1002

Affected Versions

[2.1.29-06]

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
Reported by ivassile

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
Issue-Links:
is cloned by
JAVASERVERFACES-4100
JAVASERVERFACES-4101
JAVASERVERFACES-4102

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Any thoughts on this one?

Thanks!

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Note that we have a customer currently running into this issue with Mojarra 2.1.28 on EAP 6.4.x. I've sent a patch to [email protected] that fixes the remaining usage of getExternalContext().isSecure() in ELFlash#setCookie as follows:

diff --git a/jsf-ri/src/main/java/com/sun/faces/context/flash/ELFlash.java b/jsf-ri/src/main/java/com/sun/faces/context/flash/ELFlash.java
index b3494d9..60da575 100644
--- a/jsf-ri/src/main/java/com/sun/faces/context/flash/ELFlash.java
+++ b/jsf-ri/src/main/java/com/sun/faces/context/flash/ELFlash.java
@@ -999,7 +999,23 @@ public class ELFlash extends Flash {
             if (null != (val = toSet.getMaxAge())) {
 properties.put("maxAge", val);
             }
-            if (context.getExternalContext().isSecure()) {
+            // Bug 18611757 / JAVASERVERFACES-4074: only use extContext.isSecure() if we +            // absolutely must.  For example, if we are in a portlet environment. +            boolean isSecure = false;
+            Object request = extContext.getRequest();
+            if (request instanceof ServletRequest) {
+isSecure = ((ServletRequest)request).isSecure();
+            } else {
+try {
+    isSecure = extContext.isSecure();
+} catch (UnsupportedOperationException uoe) {
+    if (LOGGER.isLoggable(Level.SEVERE)) {
+        LOGGER.log(Level.SEVERE, "ExternalContext {0} does not implement isSecure().  Please implement this per the JSF 2.1 specification.",
+new Object [] { extContext });
+    }
+}
+            }
+            if (isSecure) {
 properties.put("secure", Boolean.TRUE);
             } else if (null != (val = toSet.getSecure())) {
 properties.put("secure", val);

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Please let us know if this fix looks good. (A Red Hat customer is running into this issue with Mojarra 2.1.28 on EAP 6.4.x.)

Thanks!

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Here's a PR that's been created for this issue:

https://github.com/javaserverfaces/mojarra/pull/2

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
ren.zhijun.oracle said:
I will fix it in this week, please stay tuned

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Thanks very much! Please point me to the branch where this fix was applied. I checked the following branch but I don't seem to see the fix there:

https://github.com/javaserverfaces/mojarra/tree/2.1.29

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
ren.zhijun.oracle said:
Hi, thanks for reminder, just push the commit to repository.

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
ren.zhijun.oracle said:
and will port to 2.2/2.2.8/2.3 later.

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
Strange, I'm still not seeing it in the 2.1.29 branch:

https://github.com/javaserverfaces/mojarra/tree/2.1.29

Am I looking in the right spot?

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
fjuma said:
I am now able to see the fix in the 2.1.29 branch. Thanks!

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
Marked as fixed on Wednesday, May 18th 2016, 1:58:08 am

@ren-zhijun-oracle
Copy link
Contributor Author

@javaserverfaces Commented
This issue was imported from java.net JIRA JAVASERVERFACES-4074

@ren-zhijun-oracle
Copy link
Contributor Author

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

1 participant