bug#6957: url-cookie-expired-p

bug#6957: url-cookie-expired-p

[shawn boles]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

Hello,

Please forgive me if this list is not the correct recipient for this
message. If there is a better destination, I will be happy to send it
on.

I am working on a web services client that is mostly a wrapper around
URL Package. I am using GNU Emacs v.23.2. My client implementation
requires keeping tight rein on a session ID cookie. My sessions are
terminating abnormally. I believe I have tracked this issue to a bug
in url-cookie-expired-p.

If the cookie's expiration date is the same as today's date,
url-cookie-expired-p normalizes the times to seconds and compares the
difference. I believe the problem is in the normalization algorithm.
In pseudo-code, the current implementation does:

(+ (* 360 seconds) (* 60 minutes) (* 1 hours))

I believe this should be:

(+ (* 1 seconds) (* 60 minutes) (* 360 hours)).

As is, the result of the comparison is almost always dependent on the
number of seconds in the time strings. It is interesting how
frequently this mistaken comparison is correct.

I have attached a patch.

Thank you,

-shawn

[-- Attachment #2: url-cookie.el.patch --]
[-- Type: application/octet-stream, Size: 1032 bytes --]

--- /cygdrive/c/src/Emacs/emacs-23.2/emacs-23.2/lisp/url/url-cookie.el	2010-08-31 10:53:08.860091900 -0700
+++ url-cookie.el	2010-08-31 12:33:41.970367100 -0700
@@ -215,12 +215,12 @@
      (t					; Expires sometime today, check times
       (let* ((cur-time (timezone-parse-time (aref cur-date 3)))
 	     (exp-time (timezone-parse-time (aref exp-date 3)))
-	     (cur-norm (+ (* 360 (string-to-number (aref cur-time 2)))
+	     (cur-norm (+ (* 360 (string-to-number (aref cur-time 0)))
 			  (*  60 (string-to-number (aref cur-time 1)))
-			  (*   1 (string-to-number (aref cur-time 0)))))
-	     (exp-norm (+ (* 360 (string-to-number (aref exp-time 2)))
+			  (*   1 (string-to-number (aref cur-time 2)))))
+	     (exp-norm (+ (* 360 (string-to-number (aref exp-time 0)))
 			  (*  60 (string-to-number (aref exp-time 1)))
-			  (*   1 (string-to-number (aref exp-time 0))))))
+			  (*   1 (string-to-number (aref exp-time 2))))))
 	(> (- cur-norm exp-norm) 1))))))
 
 (defun url-cookie-retrieve (host &optional localpart secure)

bug#6957: url-cookie-expired-p Redux

[shawn boles]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

So this is buggier than I thought.

First, the hour normalization is still incorrect: there are 3600
seconds in an hour, not 360.

Second, the cookie expiration time is GMT, per the cookie spec. As is,
url-cookie-expired-p compares current time (no GMT adjustment) with a
GMT expiration time.

Please see the attached patch.

I fixed the hour normalization.

I added a function called url-cookie-gmt-time-string that returns the
current time adjusted to GMT. Please check this function. While I
believe my implementation is correct, there may be a better way to
adjust a time to GMT in Emacs.

I updated url-cookie-expire-p so that it compares local time as GMT
with the expiration time, also as GMT.

Thanks!
shawn

[-- Attachment #2: url-cookie.el.patch --]
[-- Type: application/octet-stream, Size: 1795 bytes --]

--- /cygdrive/c/src/Emacs/emacs-23.2/emacs-23.2/lisp/url/url-cookie.el	2010-08-31 10:53:08.860091900 -0700
+++ url-cookie.el	2010-08-31 15:38:55.062492700 -0700
@@ -193,6 +193,12 @@
        (t
 	(setq url-cookie-storage (list (list domain tmp))))))))
 
+(defun url-cookie-gmt-time-string ()
+	"Returns the current time, adjusted to GMT, as a time string"
+	(let* ((offset (- 0 (car (current-time-zone))))
+				(gmt (+ offset (float-time))))
+		(seconds-to-time gmt)))
+
 (defun url-cookie-expired-p (cookie)
   (let* (
 	 (exp (url-cookie-expires cookie))
@@ -212,15 +218,16 @@
      ((not exp)	nil)			; No expiry == expires at browser quit
      ((< diff-in-days 0) nil)		; Expires sometime after today
      ((> diff-in-days 0) t)		; Expired before today
-     (t					; Expires sometime today, check times
-      (let* ((cur-time (timezone-parse-time (aref cur-date 3)))
+     (t					; Expires sometime today, check times (GMT)
+      (let* ((gmt-time (format-time-string "%T" (url-cookie-gmt-time-string)))
+						 (cur-time (timezone-parse-time gmt-time))
 	     (exp-time (timezone-parse-time (aref exp-date 3)))
-	     (cur-norm (+ (* 360 (string-to-number (aref cur-time 2)))
+	     (cur-norm (+ (* 3600 (string-to-number (aref cur-time 0)))
 			  (*  60 (string-to-number (aref cur-time 1)))
-			  (*   1 (string-to-number (aref cur-time 0)))))
-	     (exp-norm (+ (* 360 (string-to-number (aref exp-time 2)))
+			  (*   1 (string-to-number (aref cur-time 2)))))
+	     (exp-norm (+ (* 3600 (string-to-number (aref exp-time 0)))
 			  (*  60 (string-to-number (aref exp-time 1)))
-			  (*   1 (string-to-number (aref exp-time 0))))))
+			  (*   1 (string-to-number (aref exp-time 2))))))
 	(> (- cur-norm exp-norm) 1))))))
 
 (defun url-cookie-retrieve (host &optional localpart secure)

bug#6957: url-cookie-expired-p Redux

[Andreas Schwab]

shawn boles <shawn.boles@gmail.com> writes:

> I added a function called url-cookie-gmt-time-string that returns the
> current time adjusted to GMT. Please check this function. While I
> believe my implementation is correct, there may be a better way to
> adjust a time to GMT in Emacs.

(float-time) already gives the seconds since epoch.  No need to convert
to string and back, or doing any time zone adjustments.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#6957: url-cookie-expired-p Redux

[shawn boles]

On Tue, Aug 31, 2010 at 4:39 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> (float-time) already gives the seconds since epoch.  No need to convert
> to string and back, or doing any time zone adjustments.

Hi Andreas,

Thank you for your reply.

I understand that (float-time) gives the seconds since Epoch. Maybe I
am missing something here...

This does not change the issue that (url-cookie-expired-p) is taking
the time string from (current-time-string), adjusted to the user's
timezone and comparing it against a cookie expiration time string,
adjusted to GMT. (url-cookie-expired-p) is not taking into account
that the times are (most likely) in different timezones.

Once I had settled the time normalization issue, I noticed that my
hour long session cookies were still not expiring. After I added
debugging I discovered that (url-cookie-expired-p) was comparing the
current time: "16:30:00" (PST) against "23:30:00" (GMT). My solution
(arguably not the best!) is to get the current time in seconds since
the Epoch (float-time), adjust it to a time that is GMT with the value
of (car (current-timezone)) and then make the comparison. This may not
be the best solution, but! (url-cookie-expired-p) is now comparing
like times.

Please let me know if I am way off base here.

It occurs to me that the date comparison is also wrong, for the same
reason: It takes a date string from (current-time-string), adjusted to
the user's timezone and compares it against a cookie date string in
GMT. I would like to fix this as well.

Thank you,
shawn

bug#6957: url-cookie-expired-p Redux (at last)

[shawn boles]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Tue, Aug 31, 2010 at 4:39 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> (float-time) already gives the seconds since epoch.  No need to convert
> to string and back, or doing any time zone adjustments.

I spent yesterday laid up with a cold. Between naps I found myself
coming back to (url-cookie-expired-p) and Andreas' response to my GMT
conversion. I kept thinking that there must be kernel of wisdom for
this grasshopper to find.

I would like to suggest the following replacement for
(url-cookie-expired-p); please see attached patch.

(defun url-cookie-expired-p (cookie)
	(let* ((exp (url-cookie-expires cookie))
				 (exp-time (and exp (float-time (date-to-time exp)))))
		(if (not exp) nil
			(> (float-time) exp-time))))

If the cookie has an expiration date, (float-time (date-to-time exp))
takes care of converting this to a float time, adjusted to the
client's time zone. Then all we need to do is compare this exp-time
against (float-time).

As an added bonus, we can now remove the url-cookie dependency on timezone.

shawn

[-- Attachment #2: url-cookie.el.patch --]
[-- Type: application/octet-stream, Size: 2240 bytes --]

--- /cygdrive/c/src/Emacs/emacs-23.2/emacs-23.2/lisp/url/url-cookie.el	2010-08-31 10:53:08.860091900 -0700
+++ url-cookie.el	2010-09-02 10:06:09.694111100 -0700
@@ -24,7 +24,6 @@
 
 ;;; Code:
 
-(require 'timezone)
 (require 'url-util)
 (require 'url-parse)
 (eval-when-compile (require 'cl))
@@ -194,34 +193,12 @@
 	(setq url-cookie-storage (list (list domain tmp))))))))
 
 (defun url-cookie-expired-p (cookie)
-  (let* (
-	 (exp (url-cookie-expires cookie))
-	 (cur-date (and exp (timezone-parse-date (current-time-string))))
-	 (exp-date (and exp (timezone-parse-date exp)))
-	 (cur-greg (and cur-date (timezone-absolute-from-gregorian
-				  (string-to-number (aref cur-date 1))
-				  (string-to-number (aref cur-date 2))
-				  (string-to-number (aref cur-date 0)))))
-	 (exp-greg (and exp (timezone-absolute-from-gregorian
-			     (string-to-number (aref exp-date 1))
-			     (string-to-number (aref exp-date 2))
-			     (string-to-number (aref exp-date 0)))))
-	 (diff-in-days (and exp (- cur-greg exp-greg)))
-	 )
-    (cond
-     ((not exp)	nil)			; No expiry == expires at browser quit
-     ((< diff-in-days 0) nil)		; Expires sometime after today
-     ((> diff-in-days 0) t)		; Expired before today
-     (t					; Expires sometime today, check times
-      (let* ((cur-time (timezone-parse-time (aref cur-date 3)))
-	     (exp-time (timezone-parse-time (aref exp-date 3)))
-	     (cur-norm (+ (* 360 (string-to-number (aref cur-time 2)))
-			  (*  60 (string-to-number (aref cur-time 1)))
-			  (*   1 (string-to-number (aref cur-time 0)))))
-	     (exp-norm (+ (* 360 (string-to-number (aref exp-time 2)))
-			  (*  60 (string-to-number (aref exp-time 1)))
-			  (*   1 (string-to-number (aref exp-time 0))))))
-	(> (- cur-norm exp-norm) 1))))))
+	"Returns true if COOKIE is expired.
+If COOKIE has an expiration date it is converted to seconds, adjusted to the client timezone and then compared against (float-time)." 
+	(let* ((exp (url-cookie-expires cookie))
+				 (exp-time (and exp (float-time (date-to-time exp)))))
+		(if (not exp) nil	(> (float-time) exp-time)))
+	)
 
 (defun url-cookie-retrieve (host &optional localpart secure)
   "Retrieve all the netscape-style cookies for a specified HOST and LOCALPART."

bug#6957: url-cookie-expired-p Redux (at last)

[Glenn Morris]

Version: 23.3

Thank you for the patch; applied.

BTW, something like

(if (not foo) nil bar)

can be written as (and foo bar).