Set-cookie headers with no "path" attribute incorrectly processed by CookieManager

[asfimport]

William Herndon (Bug 38256):
JMeter's HTTP CookieManager (as of 2.1.20060112) processes set-cookie headers by
initially setting the new cookie's path to the same path as the requested URL
(modified slightly to trim the terminal page name). When a set-cookie header
comes in with no path attribute, the URL's path is left in the cookie and that
path is used as the lookup key. This is different than contemporary browsers
that use the root path (i.e., "/") as the path for set-cookie headers that lack
a path attribute.

The current behavior can cause many requests to set cookies with incorrect path
causing downstream failures for HTTP requests that need cookies that would
ordinarily be passed becauese thay have a root path.

The problem seems to be lines 327 through 336 of:

org.apache.jmeter.protocol.http.control.CookieManager

...method setCookieFromHeader()

Version: Nightly
Severity: major
OS: All

[asfimport]

William Herndon (migrated from Bugzilla):
I should note that I don't think that the URL setting that is being replaced is
necessary since browsers don't behave as if they ever use the submission URL.
However, the the replaced code could be useful if moved to the section where
the set-cookie header WITH a path attribute is detected.

Created attachment 38256-CookieManager.patch: Patch for CookieManager#setCookieFromHeader

38256-CookieManager.patch

326,336c326,329
< 		String path = url.getPath();
< 		if (path.length() == 0) {
< 			path = "/"; // $NON-NLS-1$ default if no path specified
< 		} else {
< 			int lastSlash = path.lastIndexOf("/");// $NON-NLS-1$
< 			if (lastSlash > 0) {// Must be after initial character
<                 // Upto, but not including, trailing slash for Set-Cookie:
<                 // (Set-Cookie2: would need the trailing slash as well
< 				path=path.substring(0,lastSlash);
< 			}
< 		}
---
> 		
> 		// BUG 38256 - Simply sets the root path to cover cases where there is no
> 		//             path attribute in the set-cookie header
> 		String path = "/";

[asfimport]

William Herndon (migrated from Bugzilla):
I should note that I don't think that the URL setting that is being replaced is
necessary since browsers don't behave as if they ever use the submission URL.
However, the the replaced code could be useful if moved to the section where
the set-cookie header WITH a path attribute is detected.

Created attachment 38256-CookieManager.patch: Patch for CookieManager#setCookieFromHeader

38256-CookieManager.patch

326,336c326,329
< 		String path = url.getPath();
< 		if (path.length() == 0) {
< 			path = "/"; // $NON-NLS-1$ default if no path specified
< 		} else {
< 			int lastSlash = path.lastIndexOf("/");// $NON-NLS-1$
< 			if (lastSlash > 0) {// Must be after initial character
<                 // Upto, but not including, trailing slash for Set-Cookie:
<                 // (Set-Cookie2: would need the trailing slash as well
< 				path=path.substring(0,lastSlash);
< 			}
< 		}
---
> 		
> 		// BUG 38256 - Simply sets the root path to cover cases where there is no
> 		//             path attribute in the set-cookie header
> 		String path = "/";

[asfimport]

William Herndon (migrated from Bugzilla):
The first two of these test cases should fail on 2.1.20060112 and succeed after
application of patch 17406. The other two are included for completeness.

Created attachment 38256-TestCookieManager.patch: 4 JUnit test cases to cover the set-cookie behavior

38256-TestCookieManager.patch

152a153,192
>         
> 		/** Tests missing cookie path for a trivial URL fetch from the domain 
> 		 *  Note that this fails prior to a fix for BUG 38256
> 		 */
> 		public void testMissingPath0() throws Exception {
> 			URL url = new URL("http://d.e.f/goo.html");
> 			man.addCookieFromHeader("test=moo", url);
> 			String s = man.getCookieHeaderForURL(new URL("http://d.e.f/"));
> 			assertNotNull(s);
> 			assertEquals("test=moo", s);
> 		}
> 		
> 		/** Tests missing cookie path for a non-trivial URL fetch from the 
> 		 *  domain.  Note that this fails prior to a fix for BUG 38256
> 		 */
> 		public void testMissingPath1() throws Exception {
> 			URL url = new URL("http://d.e.f/moo.html");
> 			man.addCookieFromHeader("test=moo", url);
> 			String s = man.getCookieHeaderForURL(new URL("http://d.e.f/goo.html"));
> 			assertNotNull(s);
> 			assertEquals("test=moo", s);
> 		}
> 		
> 		/** Tests explicit root path with a trivial URL fetch from the domain */
> 		public void testRootPath0() throws Exception {
> 			URL url = new URL("http://d.e.f/goo.html");
> 			man.addCookieFromHeader("test=moo;path=/", url);
> 			String s = man.getCookieHeaderForURL(new URL("http://d.e.f/"));
> 			assertNotNull(s);
> 			assertEquals("test=moo", s);
> 		}
> 		
> 		/** Tests explicit root path with a non-trivial URL fetch from the domain */
> 		public void testRootPath1() throws Exception {
> 			URL url = new URL("http://d.e.f/moo.html");
> 			man.addCookieFromHeader("test=moo;path=/", url);
> 			String s = man.getCookieHeaderForURL(new URL("http://d.e.f/goo.html"));
> 			assertNotNull(s);
> 			assertEquals("test=moo", s);
> 		}

[asfimport]

William Herndon (migrated from Bugzilla):
Sorry my comment for attachment 38406 should have said the replaced code is not
necessary but could be useful elsewhere.

[asfimport]

Sebb (migrated from Bugzilla):
Thanks, applied to 2.1 branch.

By the way, the patches were quite difficult to use.
They were not in unified diff format, so could not be used by Eclipse.