GH-9711: Avoid double URL encoding in SmbShare

artembilan · artembilan · commit 5e3d8d890a54 · 2024-12-17T13:06:46.000-05:00
Fixes: #9711 Issue link: #9711 The `SmbConfig.getUrl(_includePassword)` uses `URI.toASCIIString()` which has all the parts of the URI encoded. Then this string is used by the `SmbShare` for its super constructor, which, in turn, calls `new URL()` leading, essentially, to a second encoding round. * Add `SmbConfig.rawUrl()` methods to return an `smb://` url as a plain string. * Use this new API in the `SmbShare` for a delegation constructor * Modify some tests to reflect and cover new behavior **Auto-cherry-pick to `6.3.x`**
diff --git a/spring-integration-smb/src/main/java/org/springframework/integration/smb/session/SmbConfig.java b/spring-integration-smb/src/main/java/org/springframework/integration/smb/session/SmbConfig.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2022 the original author or authors.
+ * Copyright 2012-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,7 +25,7 @@
 import org.springframework.util.StringUtils;
 
 /**
- * Data holder class for a SMB share configuration.
+ * Data holder class for an SMB share configuration.
  *<p>
  * SmbFile URLs syntax:
  *      smb://[[[domain;]username[:password]@]server[:port]/[[share/[dir/]file]]][?[param=value[param2=value2[...]]]
@@ -197,22 +197,50 @@ public final String getUrl() {
 	}
 
 	public final String getUrl(boolean _includePassword) {
-		String domainUserPass = getDomainUserPass(_includePassword);
+		return createUri(_includePassword).toASCIIString();
+	}
 
-		String path = StringUtils.cleanPath(this.shareAndDir);
+	/**
+	 * Return the url string for the share connection without encoding.
+	 * Used in the {@link SmbShare} constructor delegation.
+	 * @return the url string for the share connection without encoding.
+	 * @since 6.3.8
+	 */
+	public final String rawUrl() {
+		return rawUrl(true);
+	}
 
-		if (!path.startsWith("/")) {
-			path = "/" + path;
-		}
+	/**
+	 * Return the url string for the share connection without encoding.
+	 * Used in the {@link SmbShare} constructor delegation.
+	 * @param _includePassword whether password has to be masked in credentials of URL.
+	 * @return the url string for the share connection without encoding.
+	 * @since 6.3.8
+	 */
+	public final String rawUrl(boolean _includePassword) {
+		String domainUserPass = getDomainUserPass(_includePassword);
+		String path = cleanPath();
+		return "smb://%s@%s%s".formatted(domainUserPass, getHostPort(), path);
+	}
 
+	private URI createUri(boolean _includePassword) {
+		String domainUserPass = getDomainUserPass(_includePassword);
+		String path = cleanPath();
 		try {
-			return new URI("smb", domainUserPass, this.host, this.port, path, null, null)
-					.toASCIIString();
+			return new URI("smb", domainUserPass, this.host, this.port, path, null, null);
 		}
 		catch (URISyntaxException e) {
 			throw new IllegalArgumentException(e);
 		}
+	}
 
+	private String cleanPath() {
+		String path = StringUtils.cleanPath(this.shareAndDir);
+
+		if (!path.startsWith("/")) {
+			path = "/" + path;
+		}
+		return path;
 	}
 
 	@Override
diff --git a/spring-integration-smb/src/main/java/org/springframework/integration/smb/session/SmbShare.java b/spring-integration-smb/src/main/java/org/springframework/integration/smb/session/SmbShare.java
@@ -63,7 +63,7 @@ public class SmbShare extends SmbFile {
 	 * @throws IOException if an invalid SMB URL was constructed by jCIFS
 	 */
 	public SmbShare(SmbConfig _smbConfig) throws IOException {
-		super(StringUtils.cleanPath(_smbConfig.validate().getUrl()),
+		super(StringUtils.cleanPath(_smbConfig.validate().rawUrl()),
 				SingletonContext.getInstance().withCredentials(
 						new NtlmPasswordAuthenticator(
 								_smbConfig.getDomain(), _smbConfig.getUsername(), _smbConfig.getPassword())));
@@ -76,7 +76,7 @@ public SmbShare(SmbConfig _smbConfig) throws IOException {
 	 * @throws IOException if an invalid SMB URL was constructed by jCIFS
 	 */
 	public SmbShare(SmbConfig _smbConfig, CIFSContext _context) throws IOException {
-		super(StringUtils.cleanPath(_smbConfig.validate().getUrl()), _context);
+		super(StringUtils.cleanPath(_smbConfig.validate().rawUrl()), _context);
 	}
 
 	/**
@@ -88,7 +88,7 @@ public SmbShare(SmbConfig _smbConfig, CIFSContext _context) throws IOException {
 	 * @throws IOException if an invalid property was set or an invalid SMB URL was constructed by jCIFS
 	 */
 	public SmbShare(SmbConfig _smbConfig, Properties _props) throws IOException {
-		super(StringUtils.cleanPath(_smbConfig.validate().getUrl()),
+		super(StringUtils.cleanPath(_smbConfig.validate().rawUrl()),
 				new BaseContext(
 						new PropertyConfiguration(_props)).withCredentials(
 						new NtlmPasswordAuthenticator(
diff --git a/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbMessageHistoryTests-context.xml b/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbMessageHistoryTests-context.xml
@@ -1,30 +1,31 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <beans xmlns="http://www.springframework.org/schema/beans"
-	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-	xmlns:int="http://www.springframework.org/schema/integration"
-	xmlns:int-smb="http://www.springframework.org/schema/integration/smb"
-	xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
+	   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	   xmlns:int="http://www.springframework.org/schema/integration"
+	   xmlns:int-smb="http://www.springframework.org/schema/integration/smb"
+	   xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
 		http://www.springframework.org/schema/integration  https://www.springframework.org/schema/integration/spring-integration.xsd
 		http://www.springframework.org/schema/integration/smb https://www.springframework.org/schema/integration/smb/spring-integration-smb.xsd">
 
 	<int:message-history/>
 
 	<bean id="smbSessionFactory" class="org.springframework.integration.smb.session.SmbSessionFactory">
-		<property name="host"        value="localhost"/>
-		<property name="port"        value="0"/>
-		<property name="domain"      value=""/>
-		<property name="username"    value="sambagu@est"/>
-		<property name="password"    value="sambag%uest"/>
-		<property name="shareAndDir" value="smb-share/"/>
+		<property name="host" value="localhost"/>
+		<property name="port" value="0"/>
+		<property name="domain" value=""/>
+		<property name="username" value="sambagu@est"/>
+		<property name="password" value="sambag%uest"/>
+		<property name="shareAndDir" value="smb share/"/>
 	</bean>
 
 	<int-smb:inbound-channel-adapter id="smbInboundChannelAdapter"
-		session-factory="smbSessionFactory"
-		channel="smbInboundChannel"
-		auto-create-local-directory="true"
-		local-directory="file:test-temp/local-5"
-		remote-directory="test-temp/remote-9"
-		delete-remote-files="false">
+									 session-factory="smbSessionFactory"
+									 channel="smbInboundChannel"
+									 auto-create-local-directory="true"
+									 local-directory="file:test-temp/local-5"
+									 remote-directory="test-temp/remote-9"
+									 auto-startup="false"
+									 delete-remote-files="false">
 		<int:poller fixed-rate="1000"/>
 	</int-smb:inbound-channel-adapter>
 
diff --git a/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbMessageHistoryTests.java b/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbMessageHistoryTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2023 the original author or authors.
+ * Copyright 2012-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -47,8 +47,10 @@ public void testMessageHistory() throws URISyntaxException {
 
 			String url = smbSessionFactory.getUrl();
 			URI uri = new URI(url);
-			assertThat("sambagu%40est:sambag%25uest").isEqualTo(uri.getRawUserInfo());
-			assertThat("sambagu@est:sambag%uest").isEqualTo(uri.getUserInfo());
+			assertThat(uri.getRawUserInfo()).isEqualTo("sambagu%40est:sambag%25uest");
+			assertThat(uri.getUserInfo()).isEqualTo("sambagu@est:sambag%uest");
+			assertThat(uri.getPath()).isEqualTo("/smb share/");
+			assertThat(uri.getRawPath()).isEqualTo("/smb%20share/");
 		}
 	}
 
diff --git a/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbParserInboundTests-context.xml b/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbParserInboundTests-context.xml
@@ -1,48 +1,49 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <beans xmlns="http://www.springframework.org/schema/beans"
-	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-	xmlns:int="http://www.springframework.org/schema/integration"
-	xmlns:int-smb="http://www.springframework.org/schema/integration/smb"
-	xmlns:context="http://www.springframework.org/schema/context"
-	xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
+	   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	   xmlns:int="http://www.springframework.org/schema/integration"
+	   xmlns:int-smb="http://www.springframework.org/schema/integration/smb"
+	   xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
 		http://www.springframework.org/schema/integration  https://www.springframework.org/schema/integration/spring-integration.xsd
 		http://www.springframework.org/schema/integration/smb https://www.springframework.org/schema/integration/smb/spring-integration-smb.xsd">
 
 	<bean id="smbSessionFactory" class="org.springframework.integration.smb.session.SmbSessionFactory">
-		<property name="host"        value="localhost"/>
-		<property name="domain"      value=""/>
-		<property name="username"    value="sambaguest"/>
-		<property name="password"    value="sambaguest"/>
+		<property name="host" value="localhost"/>
+		<property name="domain" value=""/>
+		<property name="username" value="sambaguest"/>
+		<property name="password" value="sambaguest"/>
 		<property name="shareAndDir" value="smb-share/"/>
 	</bean>
 
 	<int-smb:inbound-channel-adapter id="adapterSmb"
-		session-factory="smbSessionFactory"
-		channel="smbIn"
-		filename-pattern="foo"
-		local-directory="test-temp/local-10"
-		remote-directory="test-temp/remote-10"
-		auto-create-local-directory="true"
-		delete-remote-files="false">
-		<int:poller ref="smbPoller" />
+									 session-factory="smbSessionFactory"
+									 channel="smbIn"
+									 filename-pattern="foo"
+									 local-directory="test-temp/local-10"
+									 remote-directory="test-temp/remote-10"
+									 auto-create-local-directory="true"
+									 auto-startup="false"
+									 delete-remote-files="false">
+		<int:poller ref="smbPoller"/>
 	</int-smb:inbound-channel-adapter>
 
 	<int-smb:inbound-channel-adapter id="adapterSmb2"
-		session-factory="smbSessionFactory"
-		channel="smbIn"
-		filter="filter"
-		local-directory="test-temp"
-		remote-directory="test-temp/remote-11"
-		auto-create-local-directory="true"
-		delete-remote-files="false">
-		<int:poller ref="smbPoller" />
+									 session-factory="smbSessionFactory"
+									 channel="smbIn"
+									 filter="filter"
+									 local-directory="test-temp"
+									 remote-directory="test-temp/remote-11"
+									 auto-create-local-directory="true"
+									 auto-startup="false"
+									 delete-remote-files="false">
+		<int:poller ref="smbPoller"/>
 	</int-smb:inbound-channel-adapter>
 
 	<bean id="filter" class="org.mockito.Mockito" factory-method="mock">
-		<constructor-arg value="org.springframework.integration.file.filters.FileListFilter"  type="java.lang.Class"/>
+		<constructor-arg value="org.springframework.integration.file.filters.FileListFilter" type="java.lang.Class"/>
 	</bean>
 
-	<int:poller fixed-rate="3000" id="smbPoller" />
+	<int:poller fixed-rate="3000" id="smbPoller"/>
 
 	<int:channel id="smbIn">
 		<int:queue/>
diff --git a/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbParserInboundTests.java b/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbParserInboundTests.java
@@ -63,9 +63,4 @@ public void cleanUp() {
 		delete("test-temp/local-10", "test-temp/local-6");
 	}
 
-	public static void main(String[] _args) throws Exception {
-		new SmbParserInboundTests().cleanUp();
-		runTests(SmbParserInboundTests.class, "testLocalFilesAutoCreationTrue", "testLocalFilesAutoCreationFalse");
-	}
-
 }
diff --git a/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbTestSupport.java b/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbTestSupport.java
@@ -68,7 +68,7 @@ public class SmbTestSupport extends RemoteFileTestSupport {
 
 	public static final String HOST = "127.0.0.1";
 
-	public static final String SHARE_AND_DIR = "smb-share";
+	public static final String SHARE_AND_DIR = "smb share";
 
 	public static final String USERNAME = "sambaguest";
 
@@ -110,7 +110,7 @@ public static void connectToSMBServer() throws IOException {
 	}
 
 	public static String smbServerUrl() {
-		return smbSessionFactory.getUrl().replaceFirst('/' + SHARE_AND_DIR + '/', "");
+		return smbSessionFactory.rawUrl().replaceFirst('/' + SHARE_AND_DIR + '/', "");
 	}
 
 	public static SessionFactory<SmbFile> sessionFactory() {

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright 2012-2023 the original author or authors.`
	`2`	`+ * Copyright 2012-2024 the original author or authors.`
`3`	`3`	`*`
`4`	`4`	`* Licensed under the Apache License, Version 2.0 (the "License");`
`5`	`5`	`* you may not use this file except in compliance with the License.`
`@@ -47,8 +47,10 @@ public void testMessageHistory() throws URISyntaxException {`
`47`	`47`
`48`	`48`	`String url = smbSessionFactory.getUrl();`
`49`	`49`	`URI uri = new URI(url);`
`50`		`- assertThat("sambagu%40est:sambag%25uest").isEqualTo(uri.getRawUserInfo());`
`51`		`- assertThat("sambagu@est:sambag%uest").isEqualTo(uri.getUserInfo());`
	`50`	`+ assertThat(uri.getRawUserInfo()).isEqualTo("sambagu%40est:sambag%25uest");`
	`51`	`+ assertThat(uri.getUserInfo()).isEqualTo("sambagu@est:sambag%uest");`
	`52`	`+ assertThat(uri.getPath()).isEqualTo("/smb share/");`
	`53`	`+ assertThat(uri.getRawPath()).isEqualTo("/smb%20share/");`
`52`	`54`	`}`
`53`	`55`	`}`
`54`	`56`
Original file line number	Diff line number	Diff line change
`@@ -63,9 +63,4 @@ public void cleanUp() {`
`63`	`63`	`delete("test-temp/local-10", "test-temp/local-6");`
`64`	`64`	`}`
`65`	`65`
`66`		`- public static void main(String[] _args) throws Exception {`
`67`		`- new SmbParserInboundTests().cleanUp();`
`68`		`- runTests(SmbParserInboundTests.class, "testLocalFilesAutoCreationTrue", "testLocalFilesAutoCreationFalse");`
`69`		`- }`
`70`		`-`
`71`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ public class SmbTestSupport extends RemoteFileTestSupport {`
`68`	`68`
`69`	`69`	`public static final String HOST = "127.0.0.1";`
`70`	`70`
`71`		`- public static final String SHARE_AND_DIR = "smb-share";`
	`71`	`+ public static final String SHARE_AND_DIR = "smb share";`
`72`	`72`
`73`	`73`	`public static final String USERNAME = "sambaguest";`
`74`	`74`
`@@ -110,7 +110,7 @@ public static void connectToSMBServer() throws IOException {`
`110`	`110`	`}`
`111`	`111`
`112`	`112`	`public static String smbServerUrl() {`
`113`		`- return smbSessionFactory.getUrl().replaceFirst('/' + SHARE_AND_DIR + '/', "");`
	`113`	`+ return smbSessionFactory.rawUrl().replaceFirst('/' + SHARE_AND_DIR + '/', "");`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`public static SessionFactory<SmbFile> sessionFactory() {`