v23-0006-Review-comments.patch

From 84c6893325491fc860bcb903741e894b60478f9a Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 28 Mar 2024 21:59:02 +0100
Subject: [PATCH v23 6/6] Review comments

Fixes and tidy-ups following a review of v21, a few items
are (listed in no specific order):

* Implement a version check for libcurl in autoconf, the
  equivalent check for Meson is still a TODO.
* Address a few TODOs in the code
* libpq JSON support memory management fixups [ed: these have been moved
  to an earlier commit]
---
 config/programs.m4                           |  21 ++
 configure                                    |  34 +++
 configure.ac                                 |   1 +
 src/backend/libpq/auth-oauth.c               |  28 +-
 src/include/common/oauth-common.h            |   2 +-
 src/interfaces/libpq/Makefile                |   4 +-
 src/interfaces/libpq/fe-auth-oauth-curl.c    | 275 +++++++++++--------
 src/interfaces/libpq/fe-auth-oauth.c         |   9 +-
 src/interfaces/libpq/fe-auth-oauth.h         |   2 +-
 src/test/modules/oauth_validator/validator.c |   2 +-
 src/test/perl/PostgreSQL/Test/OAuthServer.pm |  10 +-
 src/tools/pgindent/typedefs.list             |   1 +
 12 files changed, 256 insertions(+), 133 deletions(-)

diff --git a/config/programs.m4 b/config/programs.m4
index 490ec9fe9f..157da7eec5 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -142,7 +142,28 @@ if test "$pgac_cv_ldap_safe" != yes; then
 *** also uses LDAP will crash on exit.])
 fi])
 
+# PGAC_CHECK_LIBCURL
+# ------------------
+# Check for libcurl 8.4.0 or higher since earlier versions can be compiled
+# with a codepatch containing exit(), and PostgreSQL does not allow any lib
+# linked to libpq which can call exit.
 
+# PGAC_CHECK_LIBCURL
+AC_DEFUN([PGAC_CHECK_LIBCURL],
+[AC_CACHE_CHECK([for compatible libcurl], [pgac_cv_check_libcurl],
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+[#include <curl/curlver.h>
+#if LIBCURL_VERSION_MAJOR <= 8 && LIBCURL_VERSION_MINOR < 4
+choke me
+#endif], [])],
+[pgac_cv_check_libcurl=yes],
+[pgac_cv_check_libcurl=no])])
+
+if test "$pgac_cv_check_libcurl" != yes; then
+    AC_MSG_ERROR([
+*** The installed version of libcurl is too old to use with PostgreSQL.
+*** libcurl version 8.4.0 or later is required.])
+fi])
 
 # PGAC_CHECK_READLINE
 # -------------------
diff --git a/configure b/configure
index 864bb9b7c3..1a785006b9 100755
--- a/configure
+++ b/configure
@@ -13137,6 +13137,40 @@ else
   as_fn_error $? "library 'curl' is required for --with-oauth=curl" "$LINENO" 5
 fi
 
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for compatible libcurl" >&5
+$as_echo_n "checking for compatible libcurl... " >&6; }
+if ${pgac_cv_check_libcurl+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#include <curl/curlver.h>
+#if LIBCURL_VERSION_MAJOR <= 8 && LIBCURL_VERSION_MINOR < 4
+choke me
+#endif
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_check_libcurl=yes
+else
+  pgac_cv_check_libcurl=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_check_libcurl" >&5
+$as_echo "$pgac_cv_check_libcurl" >&6; }
+
+if test "$pgac_cv_check_libcurl" != yes; then
+    as_fn_error $? "
+*** The installed version of libcurl is too old to use with PostgreSQL.
+*** libcurl version 8.4.0 or later is required." "$LINENO" 5
+fi
 fi
 
 # for contrib/sepgsql
diff --git a/configure.ac b/configure.ac
index 6aceed898c..cdc6bea660 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1445,6 +1445,7 @@ AC_SUBST(LDAP_LIBS_BE)
 
 if test "$with_oauth" = curl ; then
   AC_CHECK_LIB(curl, curl_multi_init, [], [AC_MSG_ERROR([library 'curl' is required for --with-oauth=curl])])
+  PGAC_CHECK_LIBCURL
 fi
 
 # for contrib/sepgsql
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index 024f304e4d..ec1418c3fc 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -476,9 +476,9 @@ generate_error_response(struct oauth_ctx *ctx, char **output, int *outputlen)
 	initStringInfo(&buf);
 
 	/*
-	 * TODO: note that escaping here should be belt-and-suspenders, since
-	 * escapable characters aren't valid in either the issuer URI or the scope
-	 * list, but the HBA doesn't enforce that yet.
+	 * Escaping the string here is belt-and-suspenders defensive programming
+	 * since escapable characters aren't valid in either the issuer URI or the
+	 * scope list, but the HBA doesn't enforce that yet.
 	 */
 	appendStringInfoString(&buf, "{ \"status\": \"invalid_token\", ");
 
@@ -533,7 +533,9 @@ validate_token_format(const char *header)
 	if (!header || strlen(header) <= 7)
 	{
 		ereport(COMMERROR,
-				(errmsg("malformed OAuth bearer token 1")));
+				errcode(ERRCODE_PROTOCOL_VIOLATION),
+				errmsg("malformed OAuth bearer token"),
+				errdetail_log("Bearer token is less than 8 bytes."));
 		return NULL;
 	}
 
@@ -551,9 +553,9 @@ validate_token_format(const char *header)
 	if (!*token)
 	{
 		ereport(COMMERROR,
-				(errcode(ERRCODE_PROTOCOL_VIOLATION),
-				 errmsg("malformed OAuth bearer token 2"),
-				 errdetail("Bearer token is empty.")));
+				errcode(ERRCODE_PROTOCOL_VIOLATION),
+				errmsg("malformed OAuth bearer token"),
+				errdetail_log("Bearer token is empty."));
 		return NULL;
 	}
 
@@ -573,9 +575,9 @@ validate_token_format(const char *header)
 		 * of someone's password into the logs.
 		 */
 		ereport(COMMERROR,
-				(errcode(ERRCODE_PROTOCOL_VIOLATION),
-				 errmsg("malformed OAuth bearer token 3"),
-				 errdetail("Bearer token is not in the correct format.")));
+				errcode(ERRCODE_PROTOCOL_VIOLATION),
+				errmsg("malformed OAuth bearer token"),
+				errdetail_log("Bearer token is not in the correct format."));
 		return NULL;
 	}
 
@@ -617,10 +619,10 @@ validate(Port *port, const char *auth)
 	/* Make sure the validator authenticated the user. */
 	if (ret->authn_id == NULL || ret->authn_id[0] == '\0')
 	{
-		/* TODO: use logdetail; reduce message duplication */
 		ereport(LOG,
-				(errmsg("OAuth bearer authentication failed for user \"%s\": validator provided no identity",
-						port->user_name)));
+				errmsg("OAuth bearer authentication failed for user \"%s\"",
+					   port->user_name),
+				errdetail_log("Validator provided no identity"));
 		return false;
 	}
 
diff --git a/src/include/common/oauth-common.h b/src/include/common/oauth-common.h
index 5ff3488bfb..8fe5626778 100644
--- a/src/include/common/oauth-common.h
+++ b/src/include/common/oauth-common.h
@@ -3,7 +3,7 @@
  * oauth-common.h
  *		Declarations for helper functions used for OAuth/OIDC authentication
  *
- * Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * src/include/common/oauth-common.h
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 0e0369cb63..9a290782f2 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -116,6 +116,8 @@ backend_src = $(top_srcdir)/src/backend
 # which seems to insert references to that even in pure C code. Excluding
 # __tsan_func_exit is necessary when using ThreadSanitizer data race detector
 # which use this function for instrumentation of function exit.
+# libcurl registers an exit handler in the memory debugging code when running
+# with LeakSanitizer.
 # Skip the test when profiling, as gcc may insert exit() calls for that.
 # Also skip the test on platforms where libpq infrastructure may be provided
 # by statically-linked libraries, as we can't expect them to honor this
@@ -123,7 +125,7 @@ backend_src = $(top_srcdir)/src/backend
 libpq-refs-stamp: $(shlib)
 ifneq ($(enable_coverage), yes)
 ifeq (,$(filter solaris,$(PORTNAME)))
-	@if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \
+	@if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit -e _atexit | grep exit; then \
 		echo 'libpq must not be calling any function which invokes exit'; exit 1; \
 	fi
 endif
diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c
index 0504f96e4e..9dd8454cac 100644
--- a/src/interfaces/libpq/fe-auth-oauth-curl.c
+++ b/src/interfaces/libpq/fe-auth-oauth-curl.c
@@ -3,7 +3,7 @@
  * fe-auth-oauth-curl.c
  *	   The libcurl implementation of OAuth/OIDC authentication.
  *
- * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
@@ -31,6 +31,8 @@
 #include "libpq-int.h"
 #include "mb/pg_wchar.h"
 
+#define MAX_OAUTH_RESPONSE_SIZE (1024 * 1024)
+
 /*
  * Parsed JSON Representations
  *
@@ -143,7 +145,7 @@ free_token(struct token *tok)
 /* States for the overall async machine. */
 typedef enum
 {
-	OAUTH_STEP_INIT,
+	OAUTH_STEP_INIT = 0,
 	OAUTH_STEP_DISCOVERY,
 	OAUTH_STEP_DEVICE_AUTHORIZATION,
 	OAUTH_STEP_TOKEN_REQUEST,
@@ -151,8 +153,9 @@ typedef enum
 } OAuthStep;
 
 /*
- * The async_ctx holds onto state that needs to persist across multiple calls to
- * pg_fe_run_oauth_flow(). Almost everything interacts with this in some way.
+ * The async_ctx holds onto state that needs to persist across multiple calls
+ * to pg_fe_run_oauth_flow(). Almost everything interacts with this in some
+ * way.
  */
 struct async_ctx
 {
@@ -162,9 +165,10 @@ struct async_ctx
 	int			timerfd;		/* a timerfd for signaling async timeouts */
 #endif
 	pgsocket	mux;			/* the multiplexer socket containing all
-								 * descriptors tracked by cURL, plus the
+								 * descriptors tracked by libcurl, plus the
 								 * timerfd */
-	CURLM	   *curlm;			/* top-level multi handle for cURL operations */
+	CURLM	   *curlm;			/* top-level multi handle for libcurl
+								 * operations */
 	CURL	   *curl;			/* the (single) easy handle for serial
 								 * requests */
 
@@ -183,7 +187,7 @@ struct async_ctx
 	 *				actx_error[_str] to manipulate this. This must be filled
 	 *				with something useful on an error.
 	 *
-	 * - curl_err:	an optional static error buffer used by cURL to put
+	 * - curl_err:	an optional static error buffer used by libcurl to put
 	 *				detailed information about failures. Unfortunately
 	 *				untranslatable.
 	 *
@@ -195,7 +199,7 @@ struct async_ctx
 	 */
 	const char *errctx;			/* not freed; must point to static allocation */
 	PQExpBufferData errbuf;
-	char		curl_err[CURL_ERROR_SIZE];
+	PQExpBufferData curl_err;
 
 	/*
 	 * These documents need to survive over multiple calls, and are therefore
@@ -205,6 +209,8 @@ struct async_ctx
 	struct device_authz authz;
 
 	bool		user_prompted;	/* have we already sent the authz prompt? */
+
+	int			running;
 };
 
 /*
@@ -238,7 +244,7 @@ free_curl_async_ctx(PGconn *conn, void *ctx)
 
 		if (err)
 			libpq_append_conn_error(conn,
-									"cURL easy handle removal failed: %s",
+									"libcurl easy handle removal failed: %s",
 									curl_multi_strerror(err));
 	}
 
@@ -258,7 +264,7 @@ free_curl_async_ctx(PGconn *conn, void *ctx)
 
 		if (err)
 			libpq_append_conn_error(conn,
-									"cURL multi handle cleanup failed: %s",
+									"libcurl multi handle cleanup failed: %s",
 									curl_multi_strerror(err));
 	}
 
@@ -292,8 +298,8 @@ free_curl_async_ctx(PGconn *conn, void *ctx)
 	appendPQExpBufferStr(&(ACTX)->errbuf, S)
 
 /*
- * Macros for getting and setting state for the connection's two cURL handles,
- * so you don't have to write out the error handling every time.
+ * Macros for getting and setting state for the connection's two libcurl
+ * handles, so you don't have to write out the error handling every time.
  */
 
 #define CHECK_MSETOPT(ACTX, OPT, VAL, FAILACTION) \
@@ -622,19 +628,28 @@ parse_oauth_json(struct async_ctx *actx, const struct json_field *fields)
 		actx_error(actx, "no content type was provided");
 		goto cleanup;
 	}
-	else if (strcasecmp(content_type, "application/json") != 0)
+
+	/*
+	 * We only check the media-type and not the parameters, so we need to
+	 * perform a length limited comparison and not compare the whole string.
+	 */
+	if (pg_strncasecmp(content_type, "application/json", strlen("application/json")) != 0)
 	{
-		actx_error(actx, "unexpected content type \"%s\"", content_type);
-		goto cleanup;
+		actx_error(actx, "unexpected content type: \"%s\"", content_type);
+		return false;
 	}
 
 	if (strlen(resp->data) != resp->len)
 	{
 		actx_error(actx, "response contains embedded NULLs");
-		goto cleanup;
+		return false;
 	}
 
-	makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true);
+	if (!makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true))
+	{
+		actx_error(actx, "out of memory");
+		return false;
+	}
 
 	ctx.errbuf = &actx->errbuf;
 	ctx.fields = fields;
@@ -787,7 +802,11 @@ parse_device_authz(struct async_ctx *actx, struct device_authz *authz)
 		authz->interval = parse_interval(authz->interval_str);
 	else
 	{
-		/* TODO: handle default interval of 5 seconds */
+		/*
+		 * RFC 8628 specify 5 seconds as the default value if the server
+		 * doesn't provide an interval.
+		 */
+		authz->interval = 5;
 	}
 
 	return true;
@@ -838,7 +857,7 @@ parse_access_token(struct async_ctx *actx, struct token *tok)
 }
 
 /*
- * cURL Multi Setup/Callbacks
+ * libcurl Multi Setup/Callbacks
  */
 
 /*
@@ -894,7 +913,7 @@ setup_multiplexer(struct async_ctx *actx)
 
 /*
  * Adds and removes sockets from the multiplexer set, as directed by the
- * cURL multi handle.
+ * libcurl multi handle.
  */
 static int
 register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
@@ -925,7 +944,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
 			break;
 
 		default:
-			actx_error(actx, "unknown cURL socket operation (%d)", what);
+			actx_error(actx, "unknown libcurl socket operation: %d", what);
 			return -1;
 	}
 
@@ -997,7 +1016,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
 			break;
 
 		default:
-			actx_error(actx, "unknown cURL socket operation (%d)", what);
+			actx_error(actx, "unknown libcurl socket operation: %d", what);
 			return -1;
 	}
 
@@ -1018,7 +1037,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
 		/*
 		 * EV_RECEIPT should guarantee one EV_ERROR result for every change,
 		 * whether successful or not. Failed entries contain a non-zero errno
-		 * in the `data` field.
+		 * in the data field.
 		 */
 		Assert(ev_out[i].flags & EV_ERROR);
 
@@ -1043,9 +1062,8 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
 
 /*
  * Adds or removes timeouts from the multiplexer set, as directed by the
- * cURL multi handle. Rather than continually adding and removing the timer,
- * we keep it in the set at all times and just disarm it when it's not
- * needed.
+ * libcurl multi handle. Rather than continually adding and removing the timer,
+ * we keep it in the set at all times and just disarm it when it's not needed.
  */
 static int
 register_timer(CURLM *curlm, long timeout, void *ctx)
@@ -1061,9 +1079,9 @@ register_timer(CURLM *curlm, long timeout, void *ctx)
 	else if (timeout == 0)
 	{
 		/*
-		 * A zero timeout means cURL wants us to call back immediately. That's
-		 * not technically an option for timerfd, but we can make the timeout
-		 * ridiculously short.
+		 * A zero timeout means libcurl wants us to call back immediately.
+		 * That's not technically an option for timerfd, but we can make the
+		 * timeout ridiculously short.
 		 *
 		 * TODO: maybe just signal drive_request() to immediately call back in
 		 * this case?
@@ -1098,8 +1116,21 @@ register_timer(CURLM *curlm, long timeout, void *ctx)
 	return 0;
 }
 
+static int
+debug_callback(CURL *handle, curl_infotype *type, char *data, size_t size,
+			   void *clientp)
+{
+	struct async_ctx *actx = (struct async_ctx *) clientp;
+
+	/* For now we only store TEXT debug information, extending is a TODO */
+	if (type == CURLINFO_TEXT)
+		appendBinaryPQExpBuffer(&actx->curl_err, data, size);
+
+	return 0;
+}
+
 /*
- * Initializes the two cURL handles in the async_ctx. The multi handle,
+ * Initializes the two libcurl handles in the async_ctx. The multi handle,
  * actx->curlm, is what drives the asynchronous engine and tells us what to do
  * next. The easy handle, actx->curl, encapsulates the state for a single
  * request/response. It's added to the multi handle as needed, during
@@ -1108,17 +1139,17 @@ register_timer(CURLM *curlm, long timeout, void *ctx)
 static bool
 setup_curl_handles(struct async_ctx *actx)
 {
-	curl_version_info_data	*curl_info;
+	curl_version_info_data *curl_info;
 
 	/*
 	 * Create our multi handle. This encapsulates the entire conversation with
-	 * cURL for this connection.
+	 * libcurl for this connection.
 	 */
 	actx->curlm = curl_multi_init();
 	if (!actx->curlm)
 	{
 		/* We don't get a lot of feedback on the failure reason. */
-		actx_error(actx, "failed to create cURL multi handle");
+		actx_error(actx, "failed to create libcurl multi handle");
 		return false;
 	}
 
@@ -1143,7 +1174,7 @@ setup_curl_handles(struct async_ctx *actx)
 	actx->curl = curl_easy_init();
 	if (!actx->curl)
 	{
-		actx_error(actx, "failed to create cURL handle");
+		actx_error(actx, "failed to create libcurl handle");
 		return false;
 	}
 
@@ -1160,9 +1191,14 @@ setup_curl_handles(struct async_ctx *actx)
 		/* No alternative resolver, TODO: warn about timeouts */
 	}
 
-	/* TODO investigate using conn->Pfdebug and CURLOPT_DEBUGFUNCTION here */
+	/*
+	 * Set a callback for retrieving error information from libcurl, the
+	 * function only takes effect when CURLOPT_VERBOSE has been set so make
+	 * sure the order is kept.
+	 */
+	CHECK_SETOPT(actx, CURLOPT_DEBUGDATA, actx, return false);
+	CHECK_SETOPT(actx, CURLOPT_DEBUGFUNCTION, debug_callback, return false);
 	CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false);
-	CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err, return false);
 
 	/*
 	 * Only HTTP[S] is allowed. TODO: disallow HTTP without user opt-in
@@ -1175,7 +1211,12 @@ setup_curl_handles(struct async_ctx *actx)
 	 * pretty strict when it comes to provider behavior, so we have to check
 	 * what comes back anyway.)
 	 */
-	actx->headers = curl_slist_append(actx->headers, "Accept:");	/* TODO: check result */
+	actx->headers = curl_slist_append(actx->headers, "Accept:");
+	if (actx->headers == NULL)
+	{
+		actx_error(actx, "out of memory");
+		return false;
+	}
 	CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers, return false);
 
 	return true;
@@ -1186,8 +1227,10 @@ setup_curl_handles(struct async_ctx *actx)
  */
 
 /*
- * Response callback from cURL; appends the response body into actx->work_data.
- * See start_request().
+ * Response callback from libcurl which appends the response body into
+ * actx->work_data (see start_request()). The maximum size of the data is
+ * defined by CURL_MAX_WRITE_SIZE which by default is 16kb (and can only be
+ * changed by recompiling libcurl).
  */
 static size_t
 append_data(char *buf, size_t size, size_t nmemb, void *userdata)
@@ -1195,9 +1238,19 @@ append_data(char *buf, size_t size, size_t nmemb, void *userdata)
 	PQExpBuffer resp = userdata;
 	size_t		len = size * nmemb;
 
-	/* TODO: cap the maximum size */
+	/* In case we receive data over the threshold, abort the transfer */
+	if ((resp->len + len) > MAX_OAUTH_RESPONSE_SIZE)
+		return 0;
+
+	/* The data passed from libcurl is not null-terminated */
 	appendBinaryPQExpBuffer(resp, buf, len);
-	/* TODO: check for broken buffer */
+
+	/*
+	 * Signal an error in order to abort the transfer in case we ran out of
+	 * memory in accepting the data.
+	 */
+	if (PQExpBufferBroken(resp))
+		return 0;
 
 	return len;
 }
@@ -1214,7 +1267,6 @@ static bool
 start_request(struct async_ctx *actx)
 {
 	CURLMcode	err;
-	int			running;
 
 	resetPQExpBuffer(&actx->work_data);
 	CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false);
@@ -1228,7 +1280,7 @@ start_request(struct async_ctx *actx)
 		return false;
 	}
 
-	err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &running);
+	err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &actx->running);
 	if (err)
 	{
 		actx_error(actx, "asynchronous HTTP request failed: %s",
@@ -1237,19 +1289,11 @@ start_request(struct async_ctx *actx)
 	}
 
 	/*
-	 * Sanity check.
-	 *
-	 * TODO: even though this is nominally an asynchronous process, there are
-	 * apparently operations that can synchronously fail by this point, such
-	 * as connections to closed local ports. Maybe we need to let this case
-	 * fall through to drive_request instead, or else perform a
-	 * curl_multi_info_read immediately.
+	 * Even though this is nominally an asynchronous process, there are some
+	 * operations that can synchronously fail by this point like connections
+	 * to closed local ports. Fall through and leave the sanity check for the
+	 * next state consuming actx.
 	 */
-	if (running != 1)
-	{
-		actx_error(actx, "failed to queue HTTP request");
-		return false;
-	}
 
 	return true;
 }
@@ -1262,12 +1306,18 @@ static PostgresPollingStatusType
 drive_request(struct async_ctx *actx)
 {
 	CURLMcode	err;
-	int			running;
 	CURLMsg    *msg;
 	int			msgs_left;
 	bool		done;
 
-	err = curl_multi_socket_all(actx->curlm, &running);
+	/* Sanity check the previous operation */
+	if (actx->running != 1)
+	{
+		actx_error(actx, "failed to queue HTTP request");
+		return false;
+	}
+
+	err = curl_multi_socket_all(actx->curlm, &actx->running);
 	if (err)
 	{
 		actx_error(actx, "asynchronous HTTP request failed: %s",
@@ -1275,7 +1325,7 @@ drive_request(struct async_ctx *actx)
 		return PGRES_POLLING_FAILED;
 	}
 
-	if (running)
+	if (actx->running)
 	{
 		/* We'll come back again. */
 		return PGRES_POLLING_READING;
@@ -1287,7 +1337,7 @@ drive_request(struct async_ctx *actx)
 		if (msg->msg != CURLMSG_DONE)
 		{
 			/*
-			 * Future cURL versions may define new message types; we don't
+			 * Future libcurl versions may define new message types; we don't
 			 * know how to handle them, so we'll ignore them.
 			 */
 			continue;
@@ -1304,7 +1354,7 @@ drive_request(struct async_ctx *actx)
 		err = curl_multi_remove_handle(actx->curlm, msg->easy_handle);
 		if (err)
 		{
-			actx_error(actx, "cURL easy handle removal failed: %s",
+			actx_error(actx, "libcurl easy handle removal failed: %s",
 					   curl_multi_strerror(err));
 			return PGRES_POLLING_FAILED;
 		}
@@ -1489,7 +1539,12 @@ start_device_authz(struct async_ctx *actx, PGconn *conn)
 	appendPQExpBuffer(work_buffer, "client_id=%s", conn->oauth_client_id);
 	if (conn->oauth_scope)
 		appendPQExpBuffer(work_buffer, "&scope=%s", conn->oauth_scope);
-	/* TODO check for broken buffer */
+
+	if (PQExpBufferBroken(work_buffer))
+	{
+		actx_error(actx, "out of memory");
+		return false;
+	}
 
 	/* Make our request. */
 	CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri, return false);
@@ -1631,37 +1686,40 @@ finish_token_request(struct async_ctx *actx, struct token *tok)
 	CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false);
 
 	/*
-	 * Per RFC 6749, Section 5, a successful response uses 200 OK. An error
-	 * response uses either 400 Bad Request or 401 Unauthorized.
-	 *
-	 * TODO: there are references online to 403 appearing in the wild...
+	 * Per RFC 6749, Section 5, a successful response uses 200 OK.
 	 */
-	if (response_code != 200
-		&& response_code != 400
-		 /* && response_code != 401 TODO */ )
+	if (response_code == 200)
 	{
-		actx_error(actx, "unexpected response code %ld", response_code);
-		return false;
+		actx->errctx = "failed to parse access token response";
+		if (!parse_access_token(actx, tok))
+			return false;		/* error message already set */
+
+		return true;
 	}
 
 	/*
-	 * Pull the fields we care about from the document.
+	 * An error response uses either 400 Bad Request or 401 Unauthorized.
+	 * There are references online to implementations using 403 for error
+	 * return which would violate the specification. For now we stick to the
+	 * specification but we might have to revisit this.
 	 */
-	if (response_code == 200)
+	if (response_code == 400 || response_code == 401)
 	{
-		actx->errctx = "failed to parse access token response";
-		if (!parse_access_token(actx, tok))
-			return false;		/* error message already set */
+		if (!parse_token_error(actx, &tok->err))
+			return false;
+
+		return true;
 	}
-	else if (!parse_token_error(actx, &tok->err))
-		return false;
 
-	return true;
+	/* Any other response codes are considered invalid */
+	actx_error(actx, "unexpected response code %ld", response_code);
+	return false;
 }
 
+
 /*
- * The top-level, nonblocking entry point for the cURL implementation. This will
- * be called several times to pump the async engine.
+ * The top-level, nonblocking entry point for the libcurl implementation. This
+ * will be called several times to pump the async engine.
  *
  * The architecture is based on PQconnectPoll(). The first half drives the
  * connection state forward as necessary, returning if we're not ready to
@@ -1682,7 +1740,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 	struct token tok = {0};
 
 	/*
-	 * XXX This is not safe. cURL has stringent requirements for the thread
+	 * XXX This is not safe. libcurl has stringent requirements for the thread
 	 * context in which you call curl_global_init(), because it's going to try
 	 * initializing a bunch of other libraries (OpenSSL, Winsock...). And we
 	 * probably need to consider both the TLS backend libcurl is compiled
@@ -1691,16 +1749,16 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 	 * Recent versions of libcurl have improved the thread-safety situation,
 	 * but you apparently can't check at compile time whether the
 	 * implementation is thread-safe, and there's a chicken-and-egg problem
-	 * where you can't check the thread safety until you've initialized cURL,
-	 * which you can't do before you've made sure it's thread-safe...
+	 * where you can't check the thread safety until you've initialized
+	 * libcurl, which you can't do before you've made sure it's thread-safe...
 	 *
 	 * We know we've already initialized Winsock by this point, so we should
-	 * be able to safely skip that bit. But we have to tell cURL to initialize
-	 * everything else, because other pieces of our client executable may
-	 * already be using cURL for their own purposes. If we initialize libcurl
-	 * first, with only a subset of its features, we could break those other
-	 * clients nondeterministically, and that would probably be a nightmare to
-	 * debug.
+	 * be able to safely skip that bit. But we have to tell libcurl to
+	 * initialize everything else, because other pieces of our client
+	 * executable may already be using libcurl for their own purposes. If we
+	 * initialize libcurl first, with only a subset of its features, we could
+	 * break those other clients nondeterministically, and that would probably
+	 * be a nightmare to debug.
 	 */
 	curl_global_init(CURL_GLOBAL_ALL
 					 & ~CURL_GLOBAL_WIN32); /* we already initialized Winsock */
@@ -1729,6 +1787,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 
 		initPQExpBuffer(&actx->work_data);
 		initPQExpBuffer(&actx->errbuf);
+		initPQExpBuffer(&actx->curl_err);
 
 		if (!setup_multiplexer(actx))
 			goto error_return;
@@ -1873,16 +1932,20 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 				 * errors; anything else and we bail.
 				 */
 				err = &tok.err;
-				if (!err->error || (strcmp(err->error, "authorization_pending")
-									&& strcmp(err->error, "slow_down")))
+				if (!err->error)
+				{
+					actx_error(actx, "unknown error");
+					goto error_return;
+				}
+
+				if (strcmp(err->error, "authorization_pending") != 0 &&
+					strcmp(err->error, "slow_down") != 0)
 				{
-					/* TODO handle !err->error */
 					if (err->error_description)
 						appendPQExpBuffer(&actx->errbuf, "%s ",
 										  err->error_description);
 
 					appendPQExpBuffer(&actx->errbuf, "(%s)", err->error);
-
 					goto error_return;
 				}
 
@@ -1892,7 +1955,14 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 				 */
 				if (strcmp(err->error, "slow_down") == 0)
 				{
-					actx->authz.interval += 5;	/* TODO check for overflow? */
+					int			prev_interval = actx->authz.interval;
+
+					actx->authz.interval += 5;
+					if (actx->authz.interval < prev_interval)
+					{
+						actx_error(actx, "slow_down interval overflow");
+						goto error_return;
+					}
 				}
 
 				/*
@@ -1959,21 +2029,8 @@ error_return:
 	else
 		appendPQExpBufferStr(&conn->errorMessage, actx->errbuf.data);
 
-	if (actx->curl_err[0])
-	{
-		size_t		len;
-
-		appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err);
-
-		/* Sometimes libcurl adds a newline to the error buffer. :( */
-		len = conn->errorMessage.len;
-		if (len >= 2 && conn->errorMessage.data[len - 2] == '\n')
-		{
-			conn->errorMessage.data[len - 2] = ')';
-			conn->errorMessage.data[len - 1] = '\0';
-			conn->errorMessage.len--;
-		}
-	}
+	if (actx->curl_err.len > 0)
+		appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err.data);
 
 	appendPQExpBufferStr(&conn->errorMessage, "\n");
 
diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index 66ee8ff076..61de9ac451 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -3,7 +3,7 @@
  * fe-auth-oauth.c
  *	   The front-end (client) implementation of OAuth/OIDC authentication.
  *
- * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
@@ -247,7 +247,12 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen)
 		return false;
 	}
 
-	makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true);
+	if (!makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true))
+	{
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("out of memory"));
+		return false;
+	}
 
 	initPQExpBuffer(&ctx.errbuf);
 	sem.semstate = &ctx;
diff --git a/src/interfaces/libpq/fe-auth-oauth.h b/src/interfaces/libpq/fe-auth-oauth.h
index 8d4ea45aa8..6e5e946364 100644
--- a/src/interfaces/libpq/fe-auth-oauth.h
+++ b/src/interfaces/libpq/fe-auth-oauth.h
@@ -4,7 +4,7 @@
  *
  *	  Definitions for OAuth authentication implementations
  *
- * Portions Copyright (c) 2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 2024, PostgreSQL Global Development Group
  *
  * src/interfaces/libpq/fe-auth-oauth.h
  *
diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c
index 09a4bf61d2..7b4dc9c494 100644
--- a/src/test/modules/oauth_validator/validator.c
+++ b/src/test/modules/oauth_validator/validator.c
@@ -66,7 +66,7 @@ validate_token(ValidatorModuleState *state, const char *token, const char *role)
 	/* Check to make sure our private state still exists. */
 	if (state->private_data != PRIVATE_COOKIE)
 		elog(ERROR, "oauth_validator: private state cookie changed to %p",
-				state->private_data);
+			 state->private_data);
 
 	res = palloc(sizeof(ValidatorModuleResult));
 
diff --git a/src/test/perl/PostgreSQL/Test/OAuthServer.pm b/src/test/perl/PostgreSQL/Test/OAuthServer.pm
index d96733f531..9e18186f23 100644
--- a/src/test/perl/PostgreSQL/Test/OAuthServer.pm
+++ b/src/test/perl/PostgreSQL/Test/OAuthServer.pm
@@ -4,10 +4,10 @@ package PostgreSQL::Test::OAuthServer;
 
 use warnings;
 use strict;
-use threads;
 use Scalar::Util;
 use Socket;
 use IO::Select;
+use Test::More;
 
 local *server_socket;
 
@@ -34,9 +34,9 @@ sub run
 	my $port;
 
 	my $pid = open(my $read_fh, "-|", $ENV{PYTHON}, "t/oauth_server.py")
-		// die "failed to start OAuth server: $!";
+		or die "failed to start OAuth server: $!";
 
-	read($read_fh, $port, 7) // die "failed to read port number: $!";
+	read($read_fh, $port, 7) or die "failed to read port number: $!";
 	chomp $port;
 	die "server did not advertise a valid port"
 		unless Scalar::Util::looks_like_number($port);
@@ -45,14 +45,14 @@ sub run
 	$self->{'port'} = $port;
 	$self->{'child'} = $read_fh;
 
-	print("# OAuth provider (PID $pid) is listening on port $port\n");
+	diag("OAuth provider (PID $pid) is listening on port $port\n");
 }
 
 sub stop
 {
 	my $self = shift;
 
-	print("# Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n");
+	diag("Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n");
 
 	kill(15, $self->{'pid'});
 	$self->{'pid'} = undef;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b02ee48898..7e9b1f564a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3057,6 +3057,7 @@ VacuumStmt
 ValidIOData
 ValidateIndexState
 ValidatorModuleState
+ValidatorModuleResult
 ValuesScan
 ValuesScanState
 Var
-- 
2.34.1