8000 [Fix][4.1.x] Add HttpOnly support CookieRetrievingCookieGenerator by cslzchen · Pull Request #1769 · apereo/cas · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Fix][4.1.x] Add HttpOnly support CookieRetrievingCookieGenerator #1769

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
Merged 8000
merged 3 commits into from
May 17, 2016
Merged

[Fix][4.1.x] Add HttpOnly support CookieRetrievingCookieGenerator #1769

merged 3 commits into from
May 17, 2016

Conversation

cslzchen
Copy link
Contributor
@cslzchen cslzchen commented May 13, 2016 •
edited
Loading

Version

4.1.x

Issue

Current CookieRetrievingCookieGenerator does not set HttpOnly flag when RememberMe is true.

Fix

Move the HttpOnly container check from the constructor to addCookie() where setting HttpOnly flag is added.

if (isCookieHttpOnly()) {
    final Method setHttpOnlyMethod = ReflectionUtils.findMethod(Cookie.class, "setHttpOnly", boolean.class);
    if(setHttpOnlyMethod != null) {
        cookie.setHttpOnly(true);
    } else {
        logger.debug("Cookie cannot be marked as HttpOnly; container is not using servlet 3.0.");
    }
}

Limitations & Side effects

Need to explicitly set cookieHttpOnly=true in warnCookieGenerator.xml for CASPRIVACY.

Issues & Other PR

No active related issue or open PR found.

References

@mmoayyed
Copy link
Member

This should already be handled via:
https://github.com/apereo/cas/blob/4.1.x/cas-server-webapp-support/src/main/java/org/jasig/cas/web/support/CookieRetrievingCookieGenerator.java#L66

@mmoayyed
Copy link
Member

Also, you can't quite freely set that flag because you need to ensure the container supports that method based on the servlet spec. 4.1 does not have that requirement so there are additional checks in place to ensure CAS wont crash.

@cslzchen
Copy link
Contributor Author
cslzchen commented May 13, 2016 •
edited
Loading

@mmoayyed thanks for the quick comments.

addCookie() directly creates a new cookie and adds it to the response. The constructor does not take any effects for this cookie. Your mentioning the container check is very helpful and probably we can add it to the addCookie()?

In addCookie(), if RememberMe not set, it calls super.addCookie() which does set HttpOnly (if CookieGenerator has it). However, if RememberMe set, it never set HttpOnly no matter what. I guess this part of the code attempts to do what super.addCookie() does. At the time it was written, there is no setHttpOnly() in the CookieGenerator.

@mmoayyed
Copy link
Member

Ah I see what you mean. So let me ask:

When remember-me is set, and this call is executed:
final Cookie cookie = createCookie(theCookieValue);

Does createCookie not take into account changes that were set by the constructor? If not, then yes we could add the same check into addCookie and possibly then remove the check from the ctor.

@cslzchen
Copy link
Contributor Author

No, it does not. Here is the reference.

In my temporary fix for our own server, we override the createCookie() instead of addCookie(), which is easier. But for a long term fix for CAS, I suggest that we put it in the addCookie(). Need more investigation on whether to remove the check from constructor.

@mmoayyed
Copy link
Member

Cool. Thanks for the update. Investigate away :)

@cslzchen
Copy link
Contributor Author

Here is what I learned after further investigation. Hope it is helpful :).

CookieGenerater itself is just a helper class (not the cookie class itself). Most fields in this class are just flags. We need to create a real cookie and explicitly set those flags to the cookie based on these fields. For example:
super.setCookieHttpOnly(true); only sets the flag,
cookie.setHttpOnly(true); does the job.

The container check need to be removed from the constructor and be placed at wherever we set HttpOnly on the real cookie. super.setCookieHttpOnly(true); is also removed since this should come from the config xml (or params in constructor).

if (isCookieHttpOnly()) {
    final Method setHttpOnlyMethod = ReflectionUtils.findMethod(Cookie.class, "setHttpOnly", boolean.class);
    if(setHttpOnlyMethod != null) {
        cookie.setHttpOnly(true);
    } else {
        logger.debug("Cookie cannot be marked as HttpOnly; container is not using servlet 3.0.");
    }
}

@mmoayyed
Copy link
Member

Looks good to me. Could you update the pull with the proposed changed? That should provide a clearer view on how things are organized.
8000

@cslzchen
Copy link
Contributor Author

Sure.

@cslzchen
Copy link
Contributor Author

Ready for review.

@mmoayyed mmoayyed added this to the 4.1.8 milestone May 14, 2016
@mmoayyed
Copy link
Member

Great. Looks fine.

Would 8000 you be willing to submit the same changeset over to 4.2.x and master? Or would you rather just do one? Or would you rather we port things forward manually by cherry picking?

@cslzchen
Copy link
Contributor Author

Sure, I can do both. I will let you know if I run into any issues.

This was referenced May 14, 2016
Merged
Merged
@cslzchen
Copy link
Contributor Author
cslzchen commented May 16, 2016 •
edited
Loading

@[Updated] From TGC to CASPRIVACY:
CASPRIVACY does not have HttpOnly issue by default since remember me is not enabled. Before the fix, HttpOnly is always true (super.setCookieHttpOnly(true);), but it will depend on the configuration when the fix is in place (default is false). Need to explicitly set cookieHttpOnly to true in warnCookieGenerator.xml

@cslzchen cslzchen mentioned this pull request May 16, 2016
Merged
@mmoayyed
Copy link
Member

Thanks!

@mmoayyed mmoayyed merged commit 97f30ad into apereo:4.1.x May 17, 2016
frett pushed a commit to frett/cas that referenced this pull request Sep 19, 2016
@cslzchen @SavvasMisaghMoayyed
[Fix][4.1.x] Add HttpOnly support CookieRetrievingCookieGenerator (ap…
0e89987 
…ereo#1769)

* Add HttpOnly to class CookieRetrievingCookieGenerator.

* Add container check for HttpOnly in addCookie() and improve the logic.

* Explicitly set cookieHttpOnly to true for CASPRIVACY
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Authentication Remember-Me
Projects
None yet
Milestone
4.1.8
Development

Successfully merging this pull request may close these issues.
2 participants
@cslzchen @mmoayyed
0