This comment has been minimized.
This comment has been minimized.
@imbeyondboredom you are totally right, thanks for the advice. Now should be fixed. |
This comment has been minimized.
This comment has been minimized.
When retrieving cookies via getValidCookies(URI uri) , its not detecting the sub-domain perfectly . Problem Case - uri.getPath() returing - api-testing/session.php so getValidCookies(URI uri) not returning cookie for localhost/api-testing/session.php forked here - https://gist.github.com/tibro4u/7ae579c301f4a4e8262a |
This comment has been minimized.
This comment has been minimized.
fieldHttpOnly should not be redeclared in initFieldHttpOnly method, or there will be NPE. |
This comment has been minimized.
This comment has been minimized.
@morganwu under what circumstances initFieldHttpOnly throws a NullPointerException? |
This comment has been minimized.
This comment has been minimized.
@franmontiel He's saying instead of having otherwise the instance variable fieldHttpOnly is never set and will always be null. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I've published a new revision to fix the bug pointed out by @tibro4u and others that I found, all of them related to domain matching and path matching. Now the code should adhere to the rfc6265 specification. |
This comment has been minimized.
This comment has been minimized.
Hi, @franmontiel |
This comment has been minimized.
This comment has been minimized.
Hi @chujj, It seems that you are right, the startsWith() check shouldn't exists but there is no need to change it for endsWith() because it is already checked in the next line. I'm going to publish a new revision just removing the startsWith() call. Thanks! |
This comment has been minimized.
This comment has been minimized.
Great work. There's a small bug in PersistentCookieStore on line 170: it assumes that the requestHost is longer than the cookieHost, which is not necessarily true. This will cause an OutOfBounds exception if you have, for instance, "foo.example.com" in the cookie store and the request host is "example.com". A simple fix might be: |
This comment has been minimized.
This comment has been minimized.
Would you be so kind as to attach a license to this work? Thanks. |
This comment has been minimized.
This comment has been minimized.
@seanpont, fix appplied and license attached. |
This comment has been minimized.
This comment has been minimized.
Great stuff, thanks! |
This comment has been minimized.
This comment has been minimized.
@tassa69 you're correct. Thanks |
This comment has been minimized.
This comment has been minimized.
@franmontiel maybe you should create a repository in order to track all the changes and let us fork it ;) |
This comment has been minimized.
This comment has been minimized.
Excellent work, really helped me, thanks.. |
This comment has been minimized.
This comment has been minimized.
If you run into problems with proguard (you will, reflection doesn't work when minifying/using proguard) when using this gist add this line to your proguard config: |
This comment has been minimized.
This comment has been minimized.
Thanks, it worked great in my case |
This comment has been minimized.
This comment has been minimized.
I'm curious, what is the point of the add/remove cycle here, are you trying to remove a potentially older cookie or something like that? |
This comment has been minimized.
This comment has been minimized.
@shortstuffsushi it is just to assure that the newest cookie replace a possible older one. |
This comment has been minimized.
This comment has been minimized.
Thanks for that @thintsa I was about to lose my mind. |
This comment has been minimized.
This comment has been minimized.
I could easily change this so that it would work for a non-android application right? From what I understand SharedPreferences is like a settings file or something, So instead of that I would just write to a file correct? |
This comment has been minimized.
This comment has been minimized.
@innsR you are right, the code could be changed to use a more standard persistence method. |
This comment has been minimized.
This comment has been minimized.
Hello @franmontiel and thank you so very much for this.
Questions:
Thanks in advance if you can answer these silly questions :) |
This comment has been minimized.
This comment has been minimized.
Hi @theyann, thanks for your code review. Here are some answers:
I originally wrote this in a Java 6 project and that is why I didn't use the diamond operator.
Changed in the last revision. Now an empty check is done.
If a previously stored cookie is received again the old one should be overwritten by the most recent cookie (in order to maintain the most recent expire date). To do that is necessary to remove and then add the new cookie to the set.
Changed in the last revision to directly use a list. |
This comment has been minimized.
This comment has been minimized.
Thanks for sharing, excelent work !! This is part of the stack trace: line 167 is when it calls if (currentCookie.hasExpired()) Tried adding all kinds of stuff to proguard-rules (even including -dontobfuscate) with no luck. Works great with proguard off |
This comment has been minimized.
This comment has been minimized.
OK, please disregard my previous comment. |
This comment has been minimized.
This comment has been minimized.
thank you very much , it help me so much. awesome code. |
This comment has been minimized.
This comment has been minimized.
thank all very much, very useful solution. i use with OkHttp3 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thank you very much it works great. May I suggest that you will upload this to maven and allow us to use gradle. I hate that I have external code in my project as a raw java file. Thanks! |
This comment has been minimized.
This comment has been minimized.
@lgalant Proguard obfuscates your SerializableHttpCookie objects.
@franmontiel If Proguard is not configured correctly your PersistentCookieStore will fail in a silent and confusing manner: |
This comment has been minimized.
This comment has been minimized.
Why the stream object in encode/decode methods does not require close? |
This comment has been minimized.
This comment has been minimized.
Why do you use your own |
This comment has been minimized.
This comment has been minimized.
@hintsa : thanks man saved me from great deal of headache |
This comment has been minimized.
This comment has been minimized.
the solution you have offered does not respect the expiry of cookies, i have created a nice kotlin version of it which does https://gist.github.com/leviyehonatan/0c53e89864a0890c2e524d87c6c70c2a |
This comment has been minimized.
This comment has been minimized.
Should i have a single instance of the ClearableCookieJar? or else the PersistentCookieJar is taking care of it ? Sorry if my question sounds stupid. |
This comment has been minimized.
This comment has been minimized.
@manikantagarikipati it might be possible that your are asking about the PersistentCookieJar library? |
This comment has been minimized.
This comment has been minimized.
I've fixed few issues:
Please feel free to grab the changes from my fork: https://gist.github.com/nuald/ad776c9f7f52d3f6865142bda58c6d3f |
This comment has been minimized.
This comment has been minimized.
If by HttpCookie you mean java.net.HttpCookie then there is a huge mistake in this code. EDIT: I just saw the comment above by nuald and seems someone else has already detected this bug EDIT2: I was looking for a way to implement persistent cookie store in standard java api not android. The bug that I explained refers to that of the standard java api. |
This comment has been minimized.
This comment has been minimized.
@mrmaffen build fail with yours, i change to below: -keepnames class * implements java.io.Serializable -keepclassmembers class * implements java.io.Serializable { |
This comment has been minimized.
This comment has been minimized.
FYI, isHttpOnly() and setHttpOnly() methods were added to 24 API version. |
This comment has been minimized.
This comment has been minimized.
I found strange situation in getValidCookies() method.
I have no Proguard option and I set CookieStore only once.. |
This comment has been minimized.
This comment has been minimized.
@agent10 if I comment SerializableHttpCookie.java npe bug will not appear; |
This comment has been minimized.
This comment has been minimized.
Hi, While uploading a file using okHttp, facing the following issue. Pls help me to sort it out. SerializableHttpCookie: java.lang.NoSuchFieldException: httpOnly System.err: java.lang.NullPointerException |
This comment has been minimized.
This comment has been minimized.
@mrmaffen @torv I also get a build error with these Proguard rules, on the static transient line: It seems like both of you have this line. |
This comment has been minimized.
This comment has been minimized.
Been trying to fix the httpOnly problem on API level 18. Thoughts?
|
This comment has been minimized.
This comment has been minimized.
Hi |
This comment has been minimized.
Just a comment because I stumbled across this code. On line https://gist.github.com/franmontiel/ed12a2295566b7076161#file-persistentcookiestore-java-L133 the code checks if ((storedUri.getPath() == null || storedUri.getPath().equals("/")) && storedUri.getHost().equals(uri.getHost()))
That basically means that if the path is not null and doesn't equal "/" then it will never match. This should be fixed before you use this code.