HTTP: cookie file stores expiration in seconds

A cookie file stores the expiration in seconds since the Linux Epoch,
not in milliseconds. Correct reading and writing cookie files; with
a backwards-compatibility hack to read files that contain a millisecond
timestamp.

Add a test, and fix tests not to rely on the actual current time so
that they will also run successfully after 2030-01-01 noon.

Bug: 571574
Change-Id: If3ba68391e574520701cdee119544eedc42a1ff2
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/internal/transport/http/cookies-simple1.txt

@@ -1,2 +1,2 @@
-some-domain1	TRUE	/some/path1	FALSE	1893499200000	key1	valueFromSimple1
-some-domain1	TRUE	/some/path1	FALSE	1893499200000	key2	valueFromSimple1
+some-domain1	TRUE	/some/path1	FALSE	1893499200	key1	valueFromSimple1
+some-domain1	TRUE	/some/path1	FALSE	1893499200	key2	valueFromSimple1

org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/internal/transport/http/cookies-simple2.txt

@@ -1,2 +1,2 @@
-some-domain1	TRUE	/some/path1	FALSE	1893499200000	key1	valueFromSimple2
-some-domain1	TRUE	/some/path1	FALSE	1893499200000	key3	valueFromSimple2
+some-domain1	TRUE	/some/path1	FALSE	1893499200	key1	valueFromSimple2
+some-domain1	TRUE	/some/path1	FALSE	1893499200	key3	valueFromSimple2

org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/internal/transport/http/cookies-with-empty-and-comment-lines.txt

@@ -3,6 +3,6 @@
 some-domain1	TRUE	/some/path1	FALSE	0	key1	value1
 
 # expires date is 01/01/2030 @ 12:00am (UTC)
-#HttpOnly_.some-domain2	TRUE	/some/path2	TRUE	1893499200000	key2	value2
+#HttpOnly_.some-domain2	TRUE	/some/path2	TRUE	1893499200	key2	value2
 
-some-domain3	TRUE	/some/path3	FALSE	1893499200000	key3	value3
+some-domain3	TRUE	/some/path3	FALSE	1893499200	key3	value3

org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/internal/transport/http/cookies-with-milliseconds.txt

@@ -0,0 +1,2 @@
+some-domain1	TRUE	/some/path1	FALSE	1893499200000	key1	valueFromSimple1
+some-domain1	TRUE	/some/path1	FALSE	1893499200	key2	valueFromSimple1

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/http/NetscapeCookieFileTest.java

@@ -22,13 +22,13 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.time.Duration;
 import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import java.util.Arrays;
-import java.util.Date;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.regex.Pattern;
 
 import org.eclipse.jgit.internal.storage.file.LockFile;
 import org.eclipse.jgit.util.http.HttpCookiesMatcher;
@@ -48,10 +48,14 @@ public class NetscapeCookieFileTest {
 	private URL baseUrl;
 
 	/**
-	 * This is the expiration date that is used in the test cookie files
+	 * This is the expiration date that is used in the test cookie files.
 	 */
-	private static long JAN_01_2030_NOON = Instant
-			.parse("2030-01-01T12:00:00.000Z").toEpochMilli();
+	private static final Instant TEST_EXPIRY_DATE = Instant
+			.parse("2030-01-01T12:00:00.000Z");
+
+	/** Earlier than TEST_EXPIRY_DATE. */
+	private static final Instant TEST_DATE = TEST_EXPIRY_DATE.minus(180,
+			ChronoUnit.DAYS);
 
 	@Before
 	public void setUp() throws IOException {
@@ -102,14 +106,13 @@ public class NetscapeCookieFileTest {
 		cookie.setPath("/");
 		cookie.setMaxAge(1000);
 		cookies.add(cookie);
-		Date creationDate = new Date();
 		try (Writer writer = Files.newBufferedWriter(tmpFile,
 				StandardCharsets.US_ASCII)) {
-			NetscapeCookieFile.write(writer, cookies, baseUrl, creationDate);
+			NetscapeCookieFile.write(writer, cookies, baseUrl, TEST_DATE);
 		}
 
 		String expectedExpiration = String
-				.valueOf(creationDate.getTime() + (cookie.getMaxAge() * 1000));
+				.valueOf(TEST_DATE.getEpochSecond() + cookie.getMaxAge());
 
 		assertThat(Files.readAllLines(tmpFile, StandardCharsets.US_ASCII),
 				CoreMatchers
@@ -128,13 +131,12 @@ public class NetscapeCookieFileTest {
 		HttpCookie cookie = new HttpCookie("key2", "value2");
 		cookie.setMaxAge(1000);
 		cookies.add(cookie);
-		Date creationDate = new Date();
 		try (Writer writer = Files.newBufferedWriter(tmpFile,
 				StandardCharsets.US_ASCII)) {
-			NetscapeCookieFile.write(writer, cookies, baseUrl, creationDate);
+			NetscapeCookieFile.write(writer, cookies, baseUrl, TEST_DATE);
 		}
 		String expectedExpiration = String
-				.valueOf(creationDate.getTime() + (cookie.getMaxAge() * 1000));
+				.valueOf(TEST_DATE.getEpochSecond() + cookie.getMaxAge());
 
 		assertThat(Files.readAllLines(tmpFile, StandardCharsets.US_ASCII),
 				CoreMatchers.equalTo(
@@ -160,6 +162,21 @@ public class NetscapeCookieFileTest {
 		}
 	}
 
+	@Test
+	public void testReadCookieFileWithMilliseconds() throws IOException {
+		try (InputStream input = this.getClass()
+				.getResourceAsStream("cookies-with-milliseconds.txt")) {
+			Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
+		}
+		NetscapeCookieFile cookieFile = new NetscapeCookieFile(tmpFile,
+				TEST_DATE);
+		long expectedMaxAge = Duration.between(TEST_DATE, TEST_EXPIRY_DATE)
+				.getSeconds();
+		for (HttpCookie cookie : cookieFile.getCookies(true)) {
+			assertEquals(expectedMaxAge, cookie.getMaxAge());
+		}
+	}
+
 	@Test
 	public void testWriteAfterAnotherJgitProcessModifiedTheFile()
 			throws IOException, InterruptedException {
@@ -167,7 +184,8 @@ public class NetscapeCookieFileTest {
 				.getResourceAsStream("cookies-simple1.txt")) {
 			Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
 		}
-		NetscapeCookieFile cookieFile = new NetscapeCookieFile(tmpFile);
+		NetscapeCookieFile cookieFile = new NetscapeCookieFile(tmpFile,
+				TEST_DATE);
 		cookieFile.getCookies(true);
 		// now modify file externally
 		try (InputStream input = this.getClass()
@@ -177,39 +195,19 @@ public class NetscapeCookieFileTest {
 		// now try to write
 		cookieFile.write(baseUrl);
 
-		// validate that the external changes are there as well
-		// due to rounding errors (conversion from ms to sec to ms)
-		// the expiration date might not be exact
 		List<String> lines = Files.readAllLines(tmpFile,
 				StandardCharsets.US_ASCII);
 
 		assertEquals("Expected 3 lines", 3, lines.size());
-		assertStringMatchesPatternWithInexactNumber(lines.get(0),
-				"some-domain1\tTRUE\t/some/path1\tFALSE\t(\\d*)\tkey1\tvalueFromSimple2",
-				JAN_01_2030_NOON, 1000);
-		assertStringMatchesPatternWithInexactNumber(lines.get(1),
-				"some-domain1\tTRUE\t/some/path1\tFALSE\t(\\d*)\tkey3\tvalueFromSimple2",
-				JAN_01_2030_NOON, 1000);
-		assertStringMatchesPatternWithInexactNumber(lines.get(2),
-				"some-domain1\tTRUE\t/some/path1\tFALSE\t(\\d*)\tkey2\tvalueFromSimple1",
-				JAN_01_2030_NOON, 1000);
-	}
-
-	@SuppressWarnings("boxing")
-	private static final void assertStringMatchesPatternWithInexactNumber(
-			String string, String pattern, long expectedNumericValue,
-			long delta) {
-		java.util.regex.Matcher matcher = Pattern.compile(pattern)
-				.matcher(string);
-		assertTrue("Given string '" + string + "' does not match '" + pattern
-				+ "'", matcher.matches());
-		// extract numeric value
-		Long actualNumericValue = Long.decode(matcher.group(1));
-
-		assertTrue(
-				"Value is supposed to be close to " + expectedNumericValue
-						+ " but is " + actualNumericValue + ".",
-				Math.abs(expectedNumericValue - actualNumericValue) <= delta);
+		assertEquals(
+				"some-domain1\tTRUE\t/some/path1\tFALSE\t1893499200\tkey1\tvalueFromSimple2",
+				lines.get(0));
+		assertEquals(
+				"some-domain1\tTRUE\t/some/path1\tFALSE\t1893499200\tkey3\tvalueFromSimple2",
+				lines.get(1));
+		assertEquals(
+				"some-domain1\tTRUE\t/some/path1\tFALSE\t1893499200\tkey2\tvalueFromSimple1",
+				lines.get(2));
 	}
 
 	@Test
@@ -229,14 +227,13 @@ public class NetscapeCookieFileTest {
 		cookie.setHttpOnly(true);
 		cookies.add(cookie);
 
-		Date creationDate = new Date();
-
 		try (Writer writer = Files.newBufferedWriter(tmpFile,
 				StandardCharsets.US_ASCII)) {
-			NetscapeCookieFile.write(writer, cookies, baseUrl, creationDate);
+			NetscapeCookieFile.write(writer, cookies, baseUrl, TEST_DATE);
 		}
 		Set<HttpCookie> actualCookies = new NetscapeCookieFile(tmpFile,
-				creationDate).getCookies(true);
+				TEST_DATE)
+				.getCookies(true);
 		assertThat(actualCookies, HttpCookiesMatcher.containsInOrder(cookies));
 	}
 
@@ -246,15 +243,12 @@ public class NetscapeCookieFileTest {
 				.getResourceAsStream("cookies-simple1.txt")) {
 			Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
 		}
-		// round up to the next second (to prevent rounding errors)
-		Date creationDate = new Date(
-				(System.currentTimeMillis() / 1000) * 1000);
-		Set<HttpCookie> cookies = new NetscapeCookieFile(tmpFile, creationDate)
+		Set<HttpCookie> cookies = new NetscapeCookieFile(tmpFile, TEST_DATE)
 				.getCookies(true);
 		Path tmpFile2 = folder.newFile().toPath();
 		try (Writer writer = Files.newBufferedWriter(tmpFile2,
 				StandardCharsets.US_ASCII)) {
-			NetscapeCookieFile.write(writer, cookies, baseUrl, creationDate);
+			NetscapeCookieFile.write(writer, cookies, baseUrl, TEST_DATE);
 		}
 		// compare original file with newly written one, they should not differ
 		assertEquals(Files.readAllLines(tmpFile), Files.readAllLines(tmpFile2));
@@ -267,13 +261,13 @@ public class NetscapeCookieFileTest {
 			Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
 		}
 
-		Date creationDate = new Date();
 		Set<HttpCookie> cookies = new LinkedHashSet<>();
 
 		HttpCookie cookie = new HttpCookie("key2", "value2");
 		cookie.setDomain("some-domain2");
 		cookie.setPath("/some/path2");
-		cookie.setMaxAge((JAN_01_2030_NOON - creationDate.getTime()) / 1000);
+		cookie.setMaxAge(
+				Duration.between(TEST_DATE, TEST_EXPIRY_DATE).getSeconds());
 		cookie.setSecure(true);
 		cookie.setHttpOnly(true);
 		cookies.add(cookie);
@@ -281,11 +275,12 @@ public class NetscapeCookieFileTest {
 		cookie = new HttpCookie("key3", "value3");
 		cookie.setDomain("some-domain3");
 		cookie.setPath("/some/path3");
-		cookie.setMaxAge((JAN_01_2030_NOON - creationDate.getTime()) / 1000);
+		cookie.setMaxAge(
+				Duration.between(TEST_DATE, TEST_EXPIRY_DATE).getSeconds());
 		cookies.add(cookie);
 
-		Set<HttpCookie> actualCookies = new NetscapeCookieFile(tmpFile, creationDate)
-				.getCookies(true);
+		Set<HttpCookie> actualCookies = new NetscapeCookieFile(tmpFile,
+				TEST_DATE).getCookies(true);
 		assertThat(actualCookies, HttpCookiesMatcher.containsInOrder(cookies));
 	}
 
@@ -296,7 +291,7 @@ public class NetscapeCookieFileTest {
 			Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
 		}
 
-		new NetscapeCookieFile(tmpFile)
-				.getCookies(true);
+		assertTrue(new NetscapeCookieFile(tmpFile, TEST_DATE).getCookies(true)
+				.isEmpty());
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/http/NetscapeCookieFile.java

@@ -22,11 +22,12 @@ import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.text.MessageFormat;
+import java.time.Instant;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Date;
 import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.annotations.Nullable;
@@ -53,6 +54,7 @@ import org.slf4j.LoggerFactory;
  * In general this class is not thread-safe. So any consumer needs to take care
  * of synchronization!
  *
+ * @see <a href="https://curl.se/docs/http-cookies.html">Cookie file format</a>
  * @see <a href="http://www.cookiecentral.com/faq/#3.5">Netscape Cookie File
  *      Format</a>
  * @see <a href=
@@ -92,7 +94,7 @@ public final class NetscapeCookieFile {
 
 	private byte[] hash;
 
-	final Date creationDate;
+	private final Instant createdAt;
 
 	private Set<HttpCookie> cookies = null;
 
@@ -104,13 +106,13 @@ public final class NetscapeCookieFile {
 	 *            where to find the cookie file
 	 */
 	public NetscapeCookieFile(Path path) {
-		this(path, new Date());
+		this(path, Instant.now());
 	}
 
-	NetscapeCookieFile(Path path, Date creationDate) {
+	NetscapeCookieFile(Path path, Instant createdAt) {
 		this.path = path;
 		this.snapshot = FileSnapshot.DIRTY;
-		this.creationDate = creationDate;
+		this.createdAt = createdAt;
 	}
 
 	/**
@@ -142,7 +144,7 @@ public final class NetscapeCookieFile {
 		if (cookies == null || refresh) {
 			try {
 				byte[] in = getFileContentIfModified();
-				Set<HttpCookie> newCookies = parseCookieFile(in, creationDate);
+				Set<HttpCookie> newCookies = parseCookieFile(in, createdAt);
 				if (cookies != null) {
 					cookies = mergeCookies(newCookies, cookies);
 				} else {
@@ -168,9 +170,9 @@ public final class NetscapeCookieFile {
 	 *
 	 * @param input
 	 *            the file content to parse
-	 * @param creationDate
-	 *            the date for the creation of the cookies (used to calculate
-	 *            the maxAge based on the expiration date given within the file)
+	 * @param createdAt
+	 *            cookie creation time; used to calculate the maxAge based on
+	 *            the expiration date given within the file
 	 * @return the set of parsed cookies from the given file (even expired
 	 *         ones). If there is more than one cookie with the same name in
 	 *         this file the last one overwrites the first one!
@@ -180,7 +182,7 @@ public final class NetscapeCookieFile {
 	 *             if the given file does not have a proper format
 	 */
 	private static Set<HttpCookie> parseCookieFile(@NonNull byte[] input,
-			@NonNull Date creationDate)
+			@NonNull Instant createdAt)
 			throws IOException, IllegalArgumentException {
 
 		String decoded = RawParseUtils.decode(StandardCharsets.US_ASCII, input);
@@ -190,7 +192,7 @@ public final class NetscapeCookieFile {
 				new StringReader(decoded))) {
 			String line;
 			while ((line = reader.readLine()) != null) {
-				HttpCookie cookie = parseLine(line, creationDate);
+				HttpCookie cookie = parseLine(line, createdAt);
 				if (cookie != null) {
 					cookies.add(cookie);
 				}
@@ -200,7 +202,7 @@ public final class NetscapeCookieFile {
 	}
 
 	private static HttpCookie parseLine(@NonNull String line,
-			@NonNull Date creationDate) {
+			@NonNull Instant createdAt) {
 		if (line.isEmpty() || (line.startsWith("#") //$NON-NLS-1$
 				&& !line.startsWith(HTTP_ONLY_PREAMBLE))) {
 			return null;
@@ -236,7 +238,12 @@ public final class NetscapeCookieFile {
 		cookie.setSecure(Boolean.parseBoolean(cookieLineParts[3]));
 
 		long expires = Long.parseLong(cookieLineParts[4]);
-		long maxAge = (expires - creationDate.getTime()) / 1000;
+		// Older versions stored milliseconds. This heuristic to detect that
+		// will cause trouble in the year 33658. :-)
+		if (cookieLineParts[4].length() == 13) {
+			expires = TimeUnit.MILLISECONDS.toSeconds(expires);
+		}
+		long maxAge = expires - createdAt.getEpochSecond();
 		if (maxAge <= 0) {
 			return null; // skip expired cookies
 		}
@@ -245,7 +252,7 @@ public final class NetscapeCookieFile {
 	}
 
 	/**
-	 * Read the underying file and return its content but only in case it has
+	 * Read the underlying file and return its content but only in case it has
 	 * been modified since the last access.
 	 * <p>
 	 * Internally calculates the hash and maintains {@link FileSnapshot}s to
@@ -333,7 +340,7 @@ public final class NetscapeCookieFile {
 						path);
 				// reread new changes if necessary
 				Set<HttpCookie> cookiesFromFile = NetscapeCookieFile
-						.parseCookieFile(cookieFileContent, creationDate);
+						.parseCookieFile(cookieFileContent, createdAt);
 				this.cookies = mergeCookies(cookiesFromFile, cookies);
 			}
 		} catch (FileNotFoundException e) {
@@ -343,7 +350,7 @@ public final class NetscapeCookieFile {
 		ByteArrayOutputStream output = new ByteArrayOutputStream();
 		try (Writer writer = new OutputStreamWriter(output,
 				StandardCharsets.US_ASCII)) {
-			write(writer, cookies, url, creationDate);
+			write(writer, cookies, url, createdAt);
 		}
 		LockFile lockFile = new LockFile(path.toFile());
 		for (int retryCount = 0; retryCount < LOCK_ACQUIRE_MAX_RETRY_COUNT; retryCount++) {
@@ -377,24 +384,23 @@ public final class NetscapeCookieFile {
 	 * @param url
 	 *            the url for which to write the cookie (to derive the default
 	 *            values for certain cookie attributes)
-	 * @param creationDate
-	 *            the date when the cookie has been created. Important for
-	 *            calculation the cookie expiration time (calculated from
-	 *            cookie's maxAge and this creation time)
+	 * @param createdAt
+	 *            cookie creation time; used to calculate a cookie's expiration
+	 *            time
 	 * @throws IOException
 	 *             if an I/O error occurs
 	 */
 	static void write(@NonNull Writer writer,
 			@NonNull Collection<HttpCookie> cookies, @NonNull URL url,
-			@NonNull Date creationDate) throws IOException {
+			@NonNull Instant createdAt) throws IOException {
 		for (HttpCookie cookie : cookies) {
-			writeCookie(writer, cookie, url, creationDate);
+			writeCookie(writer, cookie, url, createdAt);
 		}
 	}
 
 	private static void writeCookie(@NonNull Writer writer,
 			@NonNull HttpCookie cookie, @NonNull URL url,
-			@NonNull Date creationDate) throws IOException {
+			@NonNull Instant createdAt) throws IOException {
 		if (cookie.getMaxAge() <= 0) {
 			return; // skip expired cookies
 		}
@@ -422,7 +428,7 @@ public final class NetscapeCookieFile {
 		final String expirationDate;
 		// whenCreated field is not accessible in HttpCookie
 		expirationDate = String
-				.valueOf(creationDate.getTime() + (cookie.getMaxAge() * 1000));
+				.valueOf(createdAt.getEpochSecond() + cookie.getMaxAge());
 		writer.write(expirationDate);
 		writer.write(COLUMN_SEPARATOR);
 		writer.write(cookie.getName());