Repository: pgjdbc/pgjdbc
Pull request #1201 (2018-05-23 23:14:11)

Encode url parameters
BaseDataSource did not properly encode url parameters, meaning that users could
not log in if their password contained illegal characters. The error can be
reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal
characters for query parameters).

DriverTest then failed because it did not encode the parameters either.

codecov-io: # [Codecov](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1201?src=pr&el=h1) Report
> Merging [#1201](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1201?src=pr&el=desc) into [master](https://codecov.io/gh/pgjdbc/pgjdbc/commit/9b6506dfa1076ad27a16de8fc3e85bc23f1a5b97?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `85%`.



```diff
@@             Coverage Diff              @@
##             master    #1201      +/-   ##
============================================
+ Coverage     69.21%   69.22%   +0.01%     
- Complexity     3803     3805       +2     
============================================
  Files           171      171              
  Lines         15766    15768       +2     
  Branches       2578     2578              
============================================
+ Hits          10912    10916       +4     
  Misses         3616     3616              
+ Partials       1238     1236       -2
```

davecramer: @bazzargh so it fails on Java 6 due to the fact that it java 6 does not have constructor AssertionError(java.lang.String,java.io.UnsupportedEncodingException)

Also I don't actually see a test for the user password mentioned above ?
bazzargh: I didn't add the test because the existing tests don't have any tests like that at all - they rely on the passwords set externally for the test and postgres users. However, I tried to add one and discovered this whole thing relies on a second oddity. The password only appears in the URL when the datasource is part of a JNDI test. BaseDataSource.getReference creates reference properties for username/password as well as other properties; when unpacking them again in setFromReference, it sets the 'special' properties correctly but then adds _all_ reference properties back into the ds's properties object, _including those that had already been handled specially_.

I think I should patch both issues, the encoding issue is still a problem if any other properties include special characters, like APPLICATION_NAME. But the 'special' set of properties shouldn't leak back into the standard properties too, so I'll fix that.
davecramer: @bazzargh 
Yes the data source code handling of properties is in need of some attention.
Is it even possible to use these special characters in APPLICATION_NAME ?
bazzargh: @davecramer yes, you can set that with special characters - and also the database name, which can be anything you like. I've fixed up a couple of tests to exercise this
vlsi: I think proper encoding/decoding makes sense.

I think we must test that otherwise we might end up in case the code fails to work for both encoded and plain passwords.

I think it might be just enough if we use special symbols for Travis PostgreSQL password. @bazzargh , what do you think?
bazzargh: @vlsi I think it would be better to set the application name to include special characters - particularly something like a % with non-hexadecimal after it, as that trips up URLDecoder if it's not been encoded.
The reason for using application name is that after these patches the password is no longer included in the urls generated by the datasource but other properties like application name are. But, as a double check - the tests need two passwords, one for the 'postgres' user and one for 'test', it wouldn't hurt to make one of those use special characters and leave one plain.
It may be unnecessary though, since the tests do actually cover this now. The 'bad' url generated by the datasource was eventually parsed by Driver.parseURL, where it was split at the wrong places and so the wrong password got sent to the backend. I added an explicit test that what BaseDataSource produces can be consumed by Driver in PGPropertyTest.java.

davecramer: @bazzargh +1 on APPLICATION_NAME
I still want to see special chars in the password, just in case pg doesn't allow them
bazzargh: @davecramer the reason this came up is because our DBA's create accounts with strong passwords, and we discovered that while we could use the password from the command line client, our java apps couldn't connect. The workaround was to generate a new password, but I dug in to why it had happened. During testing I was using ';/?:@&=+$,' as my password, as described above, so I'm anecdataly confirming this works in pg but not jdbc, you may want to set your tests up that way as another datapoint.

I only tested with url-special characters; I've no idea if there's any further restrictions on the password character set, eg do emoji work? But that's beyond the scope of this bug.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7b5e8de8..8c34e5e9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Wrong results when single statement is used with different bind types[PR 1137](https://github.com/pgjdbc/pgjdbc/pull/1137)
 - Support generated keys for WITH queries that miss RETURNING [PR 1138](https://github.com/pgjdbc/pgjdbc/pull/1138)
 - Support generated keys when INSERT/UPDATE/DELETE keyword is followed by a comment [PR 1138](https://github.com/pgjdbc/pgjdbc/pull/1138)
+- Properly encode special symbols in passwords in BaseDataSource [PR 1201](https://github.com/pgjdbc/pgjdbc/pull/1201)
 
 ## [42.2.1] (2018-01-25)
 ### Known issues
diff --git a/pgjdbc/src/main/java/org/postgresql/Driver.java b/pgjdbc/src/main/java/org/postgresql/Driver.java
index 637af62c..0d222a09 100644
--- a/pgjdbc/src/main/java/org/postgresql/Driver.java
+++ b/pgjdbc/src/main/java/org/postgresql/Driver.java
@@ -6,7 +6,6 @@
 package org.postgresql;
 
 import org.postgresql.jdbc.PgConnection;
-
 import org.postgresql.util.DriverInfo;
 import org.postgresql.util.ExpressionProperties;
 import org.postgresql.util.GT;
@@ -14,12 +13,12 @@ import org.postgresql.util.HostSpec;
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;
 import org.postgresql.util.SharedTimer;
+import org.postgresql.util.URLCoder;
 import org.postgresql.util.WriterHandler;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
-import java.net.URLDecoder;
 import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
@@ -64,6 +63,8 @@ public class Driver implements java.sql.Driver {
   private static final Logger PARENT_LOGGER = Logger.getLogger("org.postgresql");
   private static final Logger LOGGER = Logger.getLogger("org.postgresql.Driver");
   private static SharedTimer sharedTimer = new SharedTimer();
+  private static final String DEFAULT_PORT =
+      /*$"\""+mvn.project.property.template.default.pg.port+"\";"$*//*-*/"5431";
 
   static {
     try {
@@ -561,7 +562,7 @@ public class Driver implements java.sql.Driver {
       if (slash == -1) {
         return null;
       }
-      urlProps.setProperty("PGDBNAME", URLDecoder.decode(l_urlServer.substring(slash + 1)));
+      urlProps.setProperty("PGDBNAME", URLCoder.decode(l_urlServer.substring(slash + 1)));
 
       String[] addresses = l_urlServer.substring(0, slash).split(",");
       StringBuilder hosts = new StringBuilder();
@@ -580,7 +581,7 @@ public class Driver implements java.sql.Driver {
           ports.append(portStr);
           hosts.append(address.subSequence(0, portIdx));
         } else {
-          ports.append("/*$mvn.project.property.template.default.pg.port$*/");
+          ports.append(DEFAULT_PORT);
           hosts.append(address);
         }
         ports.append(',');
@@ -596,13 +597,13 @@ public class Driver implements java.sql.Driver {
        then set it to default
       */
       if (defaults == null || !defaults.containsKey("PGPORT")) {
-        urlProps.setProperty("PGPORT", "/*$mvn.project.property.template.default.pg.port$*/");
+        urlProps.setProperty("PGPORT", DEFAULT_PORT);
       }
       if (defaults == null || !defaults.containsKey("PGHOST")) {
         urlProps.setProperty("PGHOST", "localhost");
       }
       if (defaults == null || !defaults.containsKey("PGDBNAME")) {
-        urlProps.setProperty("PGDBNAME", URLDecoder.decode(l_urlServer));
+        urlProps.setProperty("PGDBNAME", URLCoder.decode(l_urlServer));
       }
     }
 
@@ -616,7 +617,7 @@ public class Driver implements java.sql.Driver {
       if (l_pos == -1) {
         urlProps.setProperty(token, "");
       } else {
-        urlProps.setProperty(token.substring(0, l_pos), URLDecoder.decode(token.substring(l_pos + 1)));
+        urlProps.setProperty(token.substring(0, l_pos), URLCoder.decode(token.substring(l_pos + 1)));
       }
     }
 
diff --git a/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java b/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
index 1ea38388..50dfaf69 100644
--- a/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
+++ b/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
@@ -3,10 +3,6 @@
  * See the LICENSE file in the project root for more information.
  */
 
-/**
- * Bulk data copy for PostgreSQL
- */
-
 package org.postgresql.copy;
 
 import org.postgresql.core.BaseConnection;
diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
index e15e57e3..e438f171 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@@ -1731,14 +1731,17 @@ public class QueryExecutorImpl extends QueryExecutorBase {
         : "Queries that might contain ; must be executed with QueryExecutor.QUERY_EXECUTE_AS_SIMPLE mode. "
         + "Given query is " + query.getNativeSql();
 
-    // As per "46.2. Message Flow" documentation (quote from 9.1):
-    // If successfully created, a named portal object lasts till the end of the current transaction, unless explicitly destroyed
-    //
-    // That is named portals do not require to use named statements.
+    // Per https://www.postgresql.org/docs/current/static/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY
+    // A Bind message can use the unnamed prepared statement to create a named portal.
+    // If the Bind is successful, an Execute message can reference that named portal until either
+    //      the end of the current transaction
+    //   or the named portal is explicitly destroyed
 
     boolean noResults = (flags & QueryExecutor.QUERY_NO_RESULTS) != 0;
     boolean noMeta = (flags & QueryExecutor.QUERY_NO_METADATA) != 0;
     boolean describeOnly = (flags & QueryExecutor.QUERY_DESCRIBE_ONLY) != 0;
+    // extended queries always use a portal
+    // the usePortal flag controls whether or not we use a *named* portal
     boolean usePortal = (flags & QueryExecutor.QUERY_FORWARD_CURSOR) != 0 && !noResults && !noMeta
         && fetchSize > 0 && !describeOnly;
     boolean oneShot = (flags & QueryExecutor.QUERY_ONESHOT) != 0;
diff --git a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
index 0d5261a3..0d6aff84 100644
--- a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
+++ b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
@@ -12,6 +12,7 @@ import org.postgresql.util.ExpressionProperties;
 import org.postgresql.util.GT;
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;
+import org.postgresql.util.URLCoder;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -25,7 +26,6 @@ import java.sql.SQLException;
 import java.util.Properties;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-
 import javax.naming.NamingException;
 import javax.naming.RefAddr;
 import javax.naming.Reference;
@@ -1069,17 +1069,17 @@ public abstract class BaseDataSource implements CommonDataSource, Referenceable
     if (portNumber != 0) {
       url.append(":").append(portNumber);
     }
-    url.append("/").append(databaseName);
+    url.append("/").append(URLCoder.encode(databaseName));
 
     StringBuilder query = new StringBuilder(100);
-    for (PGProperty property : PGProperty.values()) {
+    for (PGProperty property: PGProperty.values()) {
       if (property.isPresent(properties)) {
         if (query.length() != 0) {
           query.append("&");
         }
         query.append(property.getName());
         query.append("=");
-        query.append(property.get(properties));
+        query.append(URLCoder.encode(property.get(properties)));
       }
     }
 
@@ -1216,11 +1216,9 @@ public abstract class BaseDataSource implements CommonDataSource, Referenceable
       portNumber = Integer.parseInt(port);
     }
     serverName = getReferenceProperty(ref, "serverName");
-    user = getReferenceProperty(ref, "user");
-    password = getReferenceProperty(ref, "password");
 
     for (PGProperty property : PGProperty.values()) {
-      property.set(properties, getReferenceProperty(ref, property.getName()));
+      setProperty(property, getReferenceProperty(ref, property.getName()));
     }
   }
 
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
index 62261672..e8c0a45d 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
@@ -3027,7 +3027,7 @@ public class PgResultSet implements ResultSet, org.postgresql.PGRefCursorResultS
    * @param bytes The bytes of the numeric field.
    * @param oid The oid of the field.
    * @param minVal the minimum value allowed.
-   * @param minVal the maximum value allowed.
+   * @param maxVal the maximum value allowed.
    * @param targetType The target type. Used for error reporting.
    * @return The value as long.
    * @throws PSQLException If the field type is not supported numeric type or if the value is out of
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java b/pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java
index 20492383..1d75784d 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java
@@ -820,6 +820,8 @@ public class TimestampUtils {
   /**
    * Formats {@link LocalDateTime} to be sent to the backend, thus it adds time zone.
    * Do not use this method in {@link java.sql.ResultSet#getString(int)}
+   * @param localDateTime The local date to format as a String
+   * @return The formatted local date
    */
   public synchronized String toString(LocalDateTime localDateTime) {
     if (localDateTime.isAfter(MAX_LOCAL_DATETIME)) {
diff --git a/pgjdbc/src/main/java/org/postgresql/replication/PGReplicationConnection.java b/pgjdbc/src/main/java/org/postgresql/replication/PGReplicationConnection.java
index ab659880..9e1a04ea 100644
--- a/pgjdbc/src/main/java/org/postgresql/replication/PGReplicationConnection.java
+++ b/pgjdbc/src/main/java/org/postgresql/replication/PGReplicationConnection.java
@@ -39,6 +39,7 @@ public interface PGReplicationConnection {
 
   /**
    * @param slotName not null replication slot name exists in database that should be drop
+   * @throws SQLException if the replication slot cannot be dropped.
    */
   void dropReplicationSlot(String slotName) throws SQLException;
 }
diff --git a/pgjdbc/src/main/java/org/postgresql/replication/fluent/ChainedCommonStreamBuilder.java b/pgjdbc/src/main/java/org/postgresql/replication/fluent/ChainedCommonStreamBuilder.java
index 0d6d8b75..bc03468e 100644
--- a/pgjdbc/src/main/java/org/postgresql/replication/fluent/ChainedCommonStreamBuilder.java
+++ b/pgjdbc/src/main/java/org/postgresql/replication/fluent/ChainedCommonStreamBuilder.java
@@ -20,6 +20,7 @@ public interface ChainedCommonStreamBuilder<T extends ChainedCommonStreamBuilder
    * rows which could cause a recovery conflict even when the standby is disconnected.
    *
    * @param slotName not null replication slot already exists on server.
+   * @return this instance as a fluent interface
    */
   T withSlotName(String slotName);
 
diff --git a/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPI.java b/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPI.java
index e83f8c93..3e4b0def 100644
--- a/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPI.java
+++ b/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPI.java
@@ -36,7 +36,8 @@ interface NTDSAPI extends StdCallLibrary {
    *        terminator (out)
    * @param spn SPN buffer (in/out)
    * @return Error code ERROR_SUCCESS, ERROR_BUFFER_OVERFLOW or ERROR_INVALID_PARAMETER
-   * @see "http://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx"
+   * @see <a href="https://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx">
+   *     https://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx</a>
    */
   int DsMakeSpnW(WString serviceClass, /* in */
       WString serviceName, /* in */
diff --git a/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPIWrapper.java b/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPIWrapper.java
index cd8acd4a..46cc8db7 100644
--- a/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPIWrapper.java
+++ b/pgjdbc/src/main/java/org/postgresql/sspi/NTDSAPIWrapper.java
@@ -24,8 +24,8 @@ public class NTDSAPIWrapper {
    * @param referrer See MSDN
    * @return SPN generated
    * @throws LastErrorException If buffer too small or parameter incorrect
-   * @see <a href="http://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx">http://msdn.
-   *      microsoft.com/en-us/library/ms676007</a>
+   * @see <a href="https://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx">
+   *     https://msdn.microsoft.com/en-us/library/ms676007(v=vs.85).aspx</a>
    */
   public String DsMakeSpn(String serviceClass, String serviceName, String instanceName,
       short instancePort, String referrer) throws LastErrorException {
diff --git a/pgjdbc/src/main/java/org/postgresql/util/Base64.java b/pgjdbc/src/main/java/org/postgresql/util/Base64.java
index 35fb7f5d..0caf0161 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/Base64.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/Base64.java
@@ -126,7 +126,7 @@ public class Base64 {
       (byte) '6', (byte) '7', (byte) '8', (byte) '9', (byte) '+', (byte) '/'
   };
 
-  /** Determine which ALPHABET to use. */
+  /* Determine which ALPHABET to use. */
   static {
     byte[] __bytes;
     try {
diff --git a/pgjdbc/src/main/java/org/postgresql/util/MD5Digest.java b/pgjdbc/src/main/java/org/postgresql/util/MD5Digest.java
index fec1fa73..2677db7b 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/MD5Digest.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/MD5Digest.java
@@ -5,15 +5,14 @@
 
 package org.postgresql.util;
 
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
 /**
  * MD5-based utility function to obfuscate passwords before network transmission.
  *
  * @author Jeremy Wohl
  */
-
-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
-
 public class MD5Digest {
   private MD5Digest() {
   }
diff --git a/pgjdbc/src/main/java/org/postgresql/util/URLCoder.java b/pgjdbc/src/main/java/org/postgresql/util/URLCoder.java
new file mode 100644
index 00000000..accbc233
--- /dev/null
+++ b/pgjdbc/src/main/java/org/postgresql/util/URLCoder.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.util;
+
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+
+/**
+ * This class helps with URL encoding and decoding. UTF-8 encoding is used by default to make
+ * encoding consistent across the driver, and encoding might be changed via {@code
+ * postgresql.url.encoding} property
+ *
+ * Note: this should not be used outside of PostgreSQL source, this is not a public API of the
+ * driver.
+ */
+public final class URLCoder {
+  private static final String ENCODING_FOR_URL =
+      System.getProperty("postgresql.url.encoding", "UTF-8");
+
+  /**
+   * Decodes {@code x-www-form-urlencoded} string into Java string.
+   *
+   * @param encoded encoded value
+   * @return decoded value
+   * @see URLDecoder#decode(String, String)
+   */
+  public static String decode(String encoded) {
+    try {
+      return URLDecoder.decode(encoded, ENCODING_FOR_URL);
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException(
+          "Unable to decode URL entry via " + ENCODING_FOR_URL + ". This should not happen", e);
+    }
+  }
+
+  /**
+   * Encodes Java string into {@code x-www-form-urlencoded} format
+   *
+   * @param plain input value
+   * @return encoded value
+   * @see URLEncoder#encode(String, String)
+   */
+  public static String encode(String plain) {
+    try {
+      return URLEncoder.encode(plain, "UTF-8");
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException(
+          "Unable to encode URL entry via " + ENCODING_FOR_URL + ". This should not happen", e);
+    }
+  }
+}
diff --git a/pgjdbc/src/main/java/org/postgresql/util/WriterHandler.java b/pgjdbc/src/main/java/org/postgresql/util/WriterHandler.java
index 822f9536..a86770fa 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/WriterHandler.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/WriterHandler.java
@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
  * distributed with this work for additional information
@@ -17,7 +17,6 @@
  * under the License.
  */
 
-
 package org.postgresql.util;
 
 import java.io.Writer;
diff --git a/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java b/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java
index fdf123e2..06f59313 100644
--- a/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java
+++ b/pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java
@@ -152,9 +152,6 @@ public class PGXAConnection extends PGPooledConnection implements XAConnection,
     }
   }
 
-
-  /**** XAResource interface ****/
-
   /**
    * Preconditions: 1. flags must be one of TMNOFLAGS, TMRESUME or TMJOIN 2. xid != null 3.
    * connection must not be associated with a transaction 4. the TM hasn't seen the xid before


Test patch:
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java
index 317f4374..502cced1 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java
@@ -16,6 +16,7 @@ import org.postgresql.Driver;
 import org.postgresql.PGProperty;
 import org.postgresql.test.TestUtil;
 import org.postgresql.util.NullOutputStream;
+import org.postgresql.util.URLCoder;
 import org.postgresql.util.WriterHandler;
 
 import org.junit.Test;
@@ -106,7 +107,9 @@ public class DriverTest {
 
     // Test with the username in the url
     con = DriverManager.getConnection(
-        TestUtil.getURL() + "&user=" + TestUtil.getUser() + "&password=" + TestUtil.getPassword());
+        TestUtil.getURL()
+            + "&user=" + URLCoder.encode(TestUtil.getUser())
+            + "&password=" + URLCoder.encode(TestUtil.getPassword()));
     assertNotNull(con);
     con.close();
 
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java
index 7d20eea4..7c5b5299 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java
@@ -16,6 +16,7 @@ import org.postgresql.PGProperty;
 import org.postgresql.ds.PGSimpleDataSource;
 import org.postgresql.ds.common.BaseDataSource;
 import org.postgresql.test.TestUtil;
+import org.postgresql.util.URLCoder;
 
 import org.junit.After;
 import org.junit.Before;
@@ -24,7 +25,6 @@ import org.junit.Test;
 import java.beans.BeanInfo;
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
-import java.net.URLEncoder;
 import java.sql.DriverPropertyInfo;
 import java.util.ArrayList;
 import java.util.Map;
@@ -205,10 +205,10 @@ public class PGPropertyTest {
     String userName = "&u%ser";
     String password = "p%a&s^s#w!o@r*";
     String url = "jdbc:postgresql://"
-            + "localhost" + ":" + 5432 + "/"
-            + URLEncoder.encode(databaseName)
-            + "?user=" + URLEncoder.encode(userName)
-            + "&password=" + URLEncoder.encode(password);
+        + "localhost" + ":" + 5432 + "/"
+        + URLCoder.encode(databaseName)
+        + "?user=" + URLCoder.encode(userName)
+        + "&password=" + URLCoder.encode(password);
     Properties parsed = Driver.parseURL(url, new Properties());
     assertEquals("database", databaseName, PGProperty.PG_DBNAME.get(parsed));
     assertEquals("user", userName, PGProperty.USER.get(parsed));
@@ -257,4 +257,25 @@ public class PGPropertyTest {
       }
     }
   }
+
+  @Test
+  public void testEncodedUrlValuesFromDataSource() {
+    String databaseName = "d&a%ta+base";
+    String userName = "&u%ser";
+    String password = "p%a&s^s#w!o@r*";
+    String applicationName = "Laurel&Hardy=Best?Yes";
+    PGSimpleDataSource dataSource = new PGSimpleDataSource();
+
+    dataSource.setDatabaseName(databaseName);
+    dataSource.setUser(userName);
+    dataSource.setPassword(password);
+    dataSource.setApplicationName(applicationName);
+
+    Properties parsed = Driver.parseURL(dataSource.getURL(), new Properties());
+    assertEquals("database", databaseName, PGProperty.PG_DBNAME.get(parsed));
+    // datasources do not pass username and password as URL parameters
+    assertFalse("user", PGProperty.USER.isPresent(parsed));
+    assertFalse("password", PGProperty.PASSWORD.isPresent(parsed));
+    assertEquals("APPLICATION_NAME", applicationName, PGProperty.APPLICATION_NAME.get(parsed));
+  }
 }
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java
index 36e970b0..f17be0ff 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java
@@ -5,6 +5,7 @@
 
 package org.postgresql.test.jdbc2.optional;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNotSame;
@@ -197,6 +198,7 @@ public abstract class BaseDataSourceTest {
   public void testJndi() {
     initializeDataSource();
     BaseDataSource oldbds = bds;
+    String oldurl = bds.getURL();
     InitialContext ic = getInitialContext();
     try {
       ic.rebind(DATA_SOURCE_JNDI, bds);
@@ -207,9 +209,12 @@ public abstract class BaseDataSourceTest {
       fail(e.getMessage());
     }
     oldbds = bds;
+    String url = bds.getURL();
     testUseConnection();
     assertSame("Test should not have changed DataSource (" + bds + " != " + oldbds + ")!",
         oldbds , bds);
+    assertEquals("Test should not have changed DataSource URL",
+        oldurl, url);
   }
 
   /**