From 20b0c1c0fc08b5f61c2f531f0ca48057220c2e18 Mon Sep 17 00:00:00 2001
From: robnester-rh <rnester@redhat.com>
Date: Fri, 19 Jan 2024 10:00:53 -0500
Subject: [PATCH] Provide better error messaging around policy URLs

This comment addresses EC-285 and attempts to provide a better error
message to users when they provide a policy URL in a format that is
ambiguous or difficult for go-getter to parse.

Users will now receive suggestions if it appears they were trying to
reference a github or gitlab repository, but include a scheme ('https'
or 'http').

Signed-off-by: robnester-rh <rnester@redhat.com>
---
 cmd/inspect/inspect_policy_test.go       |  7 ++++++
 internal/evaluator/conftest_evaluator.go | 30 +++++++++++++++++++++---
 internal/opa/inspect.go                  |  7 ++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/cmd/inspect/inspect_policy_test.go b/cmd/inspect/inspect_policy_test.go
index 6a2488403..3ea14ce42 100644
--- a/cmd/inspect/inspect_policy_test.go
+++ b/cmd/inspect/inspect_policy_test.go
@@ -20,6 +20,7 @@ package inspect
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
 
 	"github.com/spf13/afero"
@@ -56,6 +57,9 @@ func TestFetchSourcesFromPolicy(t *testing.T) {
 		if err := fs.MkdirAll(dir, 0755); err != nil {
 			panic(err)
 		}
+		if err := afero.WriteFile(fs, fmt.Sprintf("%s/foo.rego", args.String(0)), []byte("package foo\n\nbar = 1"), 0644); err != nil {
+			panic(err)
+		}
 	}
 
 	downloader.On("Download", mock.Anything, "one", false).Return(nil).Run(createDir)
@@ -95,6 +99,9 @@ func TestFetchSources(t *testing.T) {
 		if err := fs.MkdirAll(dir, 0755); err != nil {
 			panic(err)
 		}
+		if err := afero.WriteFile(fs, fmt.Sprintf("%s/foo.rego", args.String(0)), []byte("package foo\n\nbar = 1"), 0644); err != nil {
+			panic(err)
+		}
 	}
 
 	downloader.On("Download", mock.Anything, "one", false).Return(nil).Run(createDir)
diff --git a/internal/evaluator/conftest_evaluator.go b/internal/evaluator/conftest_evaluator.go
index eb6c66509..8a1e73243 100644
--- a/internal/evaluator/conftest_evaluator.go
+++ b/internal/evaluator/conftest_evaluator.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/url"
 	"os"
 	"path"
 	"path/filepath"
@@ -332,10 +333,33 @@ func (c conftestEvaluator) Evaluate(ctx context.Context, inputs []string) ([]Out
 			return nil, nil, err
 		}
 
+		annotations := []*ast.AnnotationsRef{}
 		fs := utils.FS(ctx)
-		annotations, err := opa.InspectDir(fs, dir)
-		if err != nil {
-			return nil, nil, err
+		// We only want to inspect the directory of policy subdirs, not config or data subdirs.
+		if s.Subdir() == "policy" {
+			annotations, err = opa.InspectDir(fs, dir)
+			if err != nil {
+				errMsg := err
+				if err.Error() == "no rego files found in policy subdirectory" {
+					// Let's try to give some more robust messaging to the user.
+					policyURL, err := url.Parse(s.PolicyUrl())
+					if err != nil {
+						return nil, nil, errMsg
+					}
+					// Do we have a prefix at the end of the URL path?
+					// If not, this means we aren't trying to access a specific file.
+					// TODO: Determine if we want to check for a .git suffix as well?
+					pos := strings.LastIndex(policyURL.Path, ".")
+					if pos == -1 {
+						// Are we accessing a GitHub or GitLab URL? If so, are we beginning with 'https' or 'http'?
+						if (policyURL.Host == "github.com" || policyURL.Host == "gitlab.com") && (policyURL.Scheme == "https" || policyURL.Scheme == "http") {
+							log.Debug("Git Hub or GitLab, http transport, and no file extension, this could be a problem.")
+							errMsg = fmt.Errorf("%s.\nYou've specified a %s URL with an %s:// scheme.\nDid you mean: %s instead?", errMsg, policyURL.Hostname(), policyURL.Scheme, fmt.Sprint(policyURL.Host+policyURL.RequestURI()))
+						}
+					}
+				}
+				return nil, nil, errMsg
+			}
 		}
 
 		for _, a := range annotations {
diff --git a/internal/opa/inspect.go b/internal/opa/inspect.go
index 05d50f3ff..cc8362f7b 100644
--- a/internal/opa/inspect.go
+++ b/internal/opa/inspect.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/ast/json"
+	log "github.com/sirupsen/logrus"
 	"github.com/spf13/afero"
 )
 
@@ -198,6 +199,12 @@ func InspectDir(afs afero.Fs, dir string) ([]*ast.AnnotationsRef, error) {
 		return nil, err
 	}
 
+	// Ensure that we have actual rules, and a directory without rego files.
+	if len(regoPaths) == 0 {
+		log.Debug("No rego files found after cloning policy url.")
+		return nil, errors.New("no rego files found in policy subdirectory")
+	}
+
 	// Inspect all rego files found
 	allAnnotations, err := inspectMultiple(regoPaths, regoContents)
 	if err != nil {