From 50db1e25376e7ecc98096d1365fc1c327e45f1f5 Mon Sep 17 00:00:00 2001
From: Justin Hiemstra <jhiemstra@wisc.edu>
Date: Mon, 4 Nov 2024 22:32:19 +0000
Subject: [PATCH] Fix nil pointer bug caused by trying to list namespaces with
 no coll URL

Previously, the code in `listHttp` didn't check whether or not the director response it had
actually contained a valid collections URL. Now it checks whether the URL is populated,
and if not returns an error with a hint.

I've also added very basic unit test coverage for this, but noticed both `listHttp` and
`statHttp` could use a lot more. Rather than do that in this bug fix PR, I'll open a followup
ticket that targets only these two functions for unit test development.
---
 client/handle_http.go      |  4 +++
 client/handle_http_test.go | 58 ++++++++++++++++++++++++++++++++++++++
 client/main.go             |  2 +-
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/client/handle_http.go b/client/handle_http.go
index 7e338cfe4..faa88eadf 100644
--- a/client/handle_http.go
+++ b/client/handle_http.go
@@ -2591,6 +2591,10 @@ func (te *TransferEngine) walkDirUpload(job *clientTransferJob, transfers []tran
 // This function performs the ls command by walking through the specified collections and printing the contents of the files
 func listHttp(remoteUrl *pelican_url.PelicanURL, dirResp server_structs.DirectorResponse, token *tokenGenerator) (fileInfos []FileInfo, err error) {
 	// Get our collection listing host
+	if dirResp.XPelNsHdr.CollectionsUrl == nil {
+		return nil, errors.Errorf("Collections URL not found in director response. Are you sure there's an origin for prefix %s that supports listings?", dirResp.XPelNsHdr.Namespace)
+	}
+
 	collectionsUrl := dirResp.XPelNsHdr.CollectionsUrl
 	log.Debugln("Collections URL: ", collectionsUrl.String())
 
diff --git a/client/handle_http_test.go b/client/handle_http_test.go
index a1ec2c307..d067964c5 100644
--- a/client/handle_http_test.go
+++ b/client/handle_http_test.go
@@ -42,6 +42,7 @@ import (
 
 	"github.com/pelicanplatform/pelican/config"
 	"github.com/pelicanplatform/pelican/error_codes"
+	"github.com/pelicanplatform/pelican/pelican_url"
 	"github.com/pelicanplatform/pelican/server_structs"
 	"github.com/pelicanplatform/pelican/server_utils"
 	"github.com/pelicanplatform/pelican/test_utils"
@@ -930,3 +931,60 @@ func TestNewTransferEngine(t *testing.T) {
 		assert.NoError(t, err)
 	})
 }
+
+func TestListHttp(t *testing.T) {
+	type test struct {
+		name          string
+		pUrl          *pelican_url.PelicanURL
+		dirResp       server_structs.DirectorResponse
+		expectedError string
+	}
+	tests := []test{
+		{
+			name: "valid-collections-url",
+			pUrl: &pelican_url.PelicanURL{
+				Scheme: "pelican",
+				Host:   "something.com",
+				Path:   "/foo/bar/baz",
+			},
+			dirResp: server_structs.DirectorResponse{
+				XPelNsHdr: server_structs.XPelNs{
+					RequireToken: false,
+					Namespace:    "/foo/bar",
+					CollectionsUrl: &url.URL{
+						Scheme: "https",
+						Host:   "collections.example.com",
+					},
+				},
+			},
+			expectedError: "no such host", // punt on setting up a real server, and accept this "success" looks like a connection error
+		},
+		{
+			name: "no-collections-url",
+			pUrl: &pelican_url.PelicanURL{
+				Scheme: "pelican",
+				Host:   "something.com",
+				Path:   "/foo/bar/baz",
+			},
+			dirResp: server_structs.DirectorResponse{
+				XPelNsHdr: server_structs.XPelNs{
+					RequireToken: false,
+					Namespace:    "/foo/bar",
+				},
+			},
+			expectedError: "Collections URL not found in director response. Are you sure there's an origin for prefix /foo/bar that supports listings?",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			_, err := listHttp(test.pUrl, test.dirResp, nil)
+			if test.expectedError != "" {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
diff --git a/client/main.go b/client/main.go
index 403bdb61d..62f90af17 100644
--- a/client/main.go
+++ b/client/main.go
@@ -309,7 +309,7 @@ func DoList(ctx context.Context, remoteObject string, options ...TransferOption)
 
 	fileInfos, err = listHttp(pUrl, dirResp, token)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to do the list")
+		return nil, errors.Wrap(err, "failed to perform list request")
 	}
 
 	return fileInfos, nil