fix: improve absolute locator checks (#1498)

## Description:
if we had something like

github.com/x/y calling github.com/a/b which uploads
github.com/x/y/foo.star; we would have failures as absolute checks would
be from running package; this fixes that and catches absolute calls for
children packages as well

further improved the branch/tag/file parsing

## Is this change user facing?
YES

core/server/api_container/server/startosis_engine/builtins/import_module/import_module.go

@@ -48,7 +48,7 @@ func NewImportModule(
 					IsOptional:        false,
 					ZeroValueProvider: builtin_argument.ZeroValueProvider[starlark.String],
 					Validator: func(value starlark.Value) *startosis_errors.InterpretationError {
-						return builtin_argument.RelativeOrRemoteAbsoluteLocator(value, packageId, ModuleFileArgName)
+						return builtin_argument.NonEmptyString(value, ModuleFileArgName)
 					},
 				},
 			},

core/server/api_container/server/startosis_engine/builtins/read_file/read_file.go

@@ -29,7 +29,7 @@ func NewReadFileHelper(
 					IsOptional:        false,
 					ZeroValueProvider: builtin_argument.ZeroValueProvider[starlark.String],
 					Validator: func(value starlark.Value) *startosis_errors.InterpretationError {
-						return builtin_argument.RelativeOrRemoteAbsoluteLocator(value, packageId, SrcArgName)
+						return builtin_argument.NonEmptyString(value, SrcArgName)
 					},
 				},
 			},

core/server/api_container/server/startosis_engine/kurtosis_instruction/upload_files/upload_files.go

@@ -46,7 +46,7 @@ func NewUploadFiles(
 					IsOptional:        false,
 					ZeroValueProvider: builtin_argument.ZeroValueProvider[starlark.String],
 					Validator: func(value starlark.Value) *startosis_errors.InterpretationError {
-						return builtin_argument.RelativeOrRemoteAbsoluteLocator(value, packageId, SrcArgName)
+						return builtin_argument.NonEmptyString(value, SrcArgName)
 					},
 				},
 				{

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/builtin_argument/validators.go

@@ -110,26 +110,3 @@ func DurationOrNone(value starlark.Value, attributeName string) *startosis_error
 	}
 	return nil
 }
-
-func RelativeOrRemoteAbsoluteLocator(locatorStarlarkValue starlark.Value, packageId string, argNameForLogging string) *startosis_errors.InterpretationError {
-
-	if err := NonEmptyString(locatorStarlarkValue, argNameForLogging); err != nil {
-		return err
-	}
-
-	locatorStarlarkStr, ok := locatorStarlarkValue.(starlark.String)
-	if !ok {
-		return startosis_errors.NewInterpretationError("Value for '%s' was expected to be a starlark.String but was '%s'", argNameForLogging, reflect.TypeOf(locatorStarlarkValue))
-	}
-	locatorStr := locatorStarlarkStr.GoString()
-
-	// if absolute and local return error
-	if isLocalAbsoluteLocator(locatorStr, packageId) {
-		return startosis_errors.NewInterpretationError("The locator '%s' set in attribute '%v' is not a 'local relative locator'. Local absolute locators are not allowed you should modified it to be a valid 'local relative locator'", locatorStarlarkStr, argNameForLogging)
-	}
-	return nil
-}
-
-func isLocalAbsoluteLocator(locatorStr string, packageId string) bool {
-	return strings.HasPrefix(locatorStr, packageId)
-}

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/import_module_with_local_absolute_locator_framework_test.go

@@ -12,7 +12,7 @@ import (
 )
 
 const (
-	importModuleWithLocalAbsoluteLocatorExpectedErrorMsg = "Cannot construct 'import_module' from the provided arguments.\n\tCaused by: The following argument(s) could not be parsed or did not pass validation: {\"module_file\":\"The locator '\\\"github.com/kurtosistech/test-package/helpers.star\\\"' set in attribute 'module_file' is not a 'local relative locator'. Local absolute locators are not allowed you should modified it to be a valid 'local relative locator'\"}"
+	importModuleWithLocalAbsoluteLocatorExpectedErrorMsg = "Cannot use absolute locators"
 )
 
 var (
@@ -33,10 +33,13 @@ type importModuleWithLocalAbsoluteLocatorTestCase struct {
 }
 
 func (suite *KurtosisHelperTestSuite) TestImportFileWithLocalAbsoluteLocatorShouldNotBeValid() {
+	suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleFileName, testNoPackageReplaceOptions).Return("", startosis_errors.NewInterpretationError(importModuleWithLocalAbsoluteLocatorExpectedErrorMsg))
+
 	// start with an empty cache to validate it gets populated
 	moduleGlobalCache := map[string]*startosis_packages.ModuleCacheEntry{}
 
 	suite.runShouldFail(
+		testModulePackageId,
 		&importModuleWithLocalAbsoluteLocatorTestCase{
 			T:                      suite.T(),
 			moduleGlobalCache:      moduleGlobalCache,

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/read_file_with_local_absolute_locator_framework_test.go

@@ -4,24 +4,27 @@ import (
 	"fmt"
 	"github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/builtins/read_file"
 	"github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/kurtosis_helper"
+	"github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/startosis_errors"
 	"github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/startosis_packages"
 	"go.starlark.net/starlark"
 	"testing"
 )
 
 const (
-	readFileWithLocalAbsoluteLocatorExpectedErrorMsg = "Cannot construct 'read_file' from the provided arguments.\n\tCaused by: The following argument(s) could not be parsed or did not pass validation: {\"src\":\"The locator '\\\"github.com/kurtosistech/test-package/helpers.star\\\"' set in attribute 'src' is not a 'local relative locator'. Local absolute locators are not allowed you should modified it to be a valid 'local relative locator'\"}"
+	readFileWithLocalAbsoluteLocatorExpectedErrorMsg = "Cannot use absolute locators"
 )
 
 type readFileWithLocalAbsoluteLocatorTestCase struct {
 	*testing.T
 
-	packageContentProvider *startosis_packages.MockPackageContentProvider
+	packageContentProvider startosis_packages.PackageContentProvider
 }
 
 func (suite *KurtosisHelperTestSuite) TestReadFileWithLocalAbsoluteLocatorShouldNotBeValid() {
+	suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleFileName, testNoPackageReplaceOptions).Return("", startosis_errors.NewInterpretationError(readFileWithLocalAbsoluteLocatorExpectedErrorMsg))
 
 	suite.runShouldFail(
+		testModulePackageId,
 		&readFileWithLocalAbsoluteLocatorTestCase{
 			T:                      suite.T(),
 			packageContentProvider: suite.packageContentProvider,

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_helper_test.go

@@ -45,13 +45,13 @@ func (suite *KurtosisHelperTestSuite) run(builtin KurtosisHelperBaseTest) {
 	builtin.Assert(result)
 }
 
-func (suite *KurtosisHelperTestSuite) runShouldFail(builtin KurtosisHelperBaseTest, expectedErrMsg string) {
+func (suite *KurtosisHelperTestSuite) runShouldFail(packageId string, builtin KurtosisHelperBaseTest, expectedErrMsg string) {
 	// Add the KurtosisPlanInstruction that is being tested
 	helper := builtin.GetHelper()
 	suite.starlarkEnv[helper.GetName()] = starlark.NewBuiltin(helper.GetName(), helper.CreateBuiltin())
 
 	starlarkCode := builtin.GetStarlarkCode()
-	_, err := starlark.ExecFile(suite.starlarkThread, startosis_constants.PackageIdPlaceholderForStandaloneScript, codeToExecute(starlarkCode), suite.starlarkEnv)
+	_, err := starlark.ExecFile(suite.starlarkThread, packageId, codeToExecute(starlarkCode), suite.starlarkEnv)
 	suite.Require().Error(err, "Expected to fail running starlark code %s, but it didn't fail", builtin.GetStarlarkCode())
 	suite.Require().Equal(expectedErrMsg, err.Error())
 }

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_plan_instruction_test.go

@@ -86,7 +86,7 @@ func (suite *KurtosisPlanInstructionTestSuite) run(builtin KurtosisPlanInstructi
 	suite.Require().Equal(starlarkCodeForAssertion, serializedInstruction)
 }
 
-func (suite *KurtosisPlanInstructionTestSuite) runShouldFail(builtin KurtosisPlanInstructionBaseTest, expectedErrMsg string) {
+func (suite *KurtosisPlanInstructionTestSuite) runShouldFail(packageId string, builtin KurtosisPlanInstructionBaseTest, expectedErrMsg string) {
 	instructionsPlan := instructions_plan.NewInstructionsPlan()
 
 	// Add the KurtosisPlanInstruction that is being tested
@@ -97,7 +97,7 @@ func (suite *KurtosisPlanInstructionTestSuite) runShouldFail(builtin KurtosisPla
 	suite.starlarkEnv[instructionWrapper.GetName()] = starlark.NewBuiltin(instructionWrapper.GetName(), instructionWrapper.CreateBuiltin())
 
 	starlarkCode := builtin.GetStarlarkCode()
-	_, err := starlark.ExecFile(suite.starlarkThread, startosis_constants.PackageIdPlaceholderForStandaloneScript, codeToExecute(starlarkCode), suite.starlarkEnv)
+	_, err := starlark.ExecFile(suite.starlarkThread, packageId, codeToExecute(starlarkCode), suite.starlarkEnv)
 	suite.Require().Error(err, "Expected to fail running starlark code %s, but it didn't fail", builtin.GetStarlarkCode())
 	suite.Require().Equal(expectedErrMsg, err.Error())
 }

core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/upload_files_with_local_absolute_locator_framework_test.go

@@ -11,7 +11,7 @@ import (
 )
 
 const (
-	uploadFilesWithLocalAbsoluteLocatorExpectedErrorMsg = "Cannot construct 'upload_files' from the provided arguments.\n\tCaused by: The following argument(s) could not be parsed or did not pass validation: {\"src\":\"The locator '\\\"github.com/kurtosistech/test-package/helpers.star\\\"' set in attribute 'src' is not a 'local relative locator'. Local absolute locators are not allowed you should modified it to be a valid 'local relative locator'\"}"
+	uploadFilesWithLocalAbsoluteLocatorExpectedErrorMsg = "Tried to convert locator 'github.com/kurtosistech/test-package/helpers.star' into absolute locator but failed\n\tCaused by: Cannot use local absolute locators"
 )
 
 type uploadFilesWithLocalAbsoluteLocatorTestCase struct {
@@ -24,6 +24,7 @@ func (suite *KurtosisPlanInstructionTestSuite) TestUploadFilesWithLocalAbsoluteL
 	suite.Require().Nil(suite.packageContentProvider.AddFileContent(testModuleFileName, "Hello World!"))
 
 	suite.runShouldFail(
+		testModulePackageId,
 		&uploadFilesWithLocalAbsoluteLocatorTestCase{
 			T:                      suite.T(),
 			serviceNetwork:         suite.serviceNetwork,

core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider.go

@@ -207,6 +207,10 @@ func (provider *GitPackageContentProvider) GetAbsoluteLocatorForRelativeLocator(
 ) (string, *startosis_errors.InterpretationError) {
 	var absoluteLocator string
 
+	if isLocalAbsoluteLocator(maybeRelativeLocator, parentModuleId) {
+		return "", startosis_errors.NewInterpretationError("The locator '%s' set in attribute is not a 'local relative locator'. Local absolute locators are not allowed you should modified it to be a valid 'local relative locator'", maybeRelativeLocator)
+	}
+
 	// maybe it's not a relative url in which case we return the url
 	_, errorParsingUrl := parseGitURL(maybeRelativeLocator)
 	if errorParsingUrl == nil {
@@ -491,3 +495,7 @@ func getKurtosisYamlPathForFileUrlInternal(absPathToFile string, packagesDir str
 	}
 	return filePathToKurtosisYamlNotFound, nil
 }
+
+func isLocalAbsoluteLocator(locator string, parentPackageId string) bool {
+	return strings.HasPrefix(locator, parentPackageId)
+}

core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url.go

@@ -20,6 +20,8 @@ const (
 	emptyTagBranchOrCommit     = ""
 
 	packageRootPrefixIndicatorInRelativeLocators = "/"
+	substrNotPresent                             = -1
+	extensionCharacter                           = "."
 )
 
 // ParsedGitURL an object representing a parsed moduleURL
@@ -130,16 +132,28 @@ func parseOutTagBranchOrCommit(input string) (string, string) {
 	cleanInput := path.Clean(input)
 	pathWithoutVersion, maybeTagBranchOrCommitWithFile, _ := strings.Cut(cleanInput, tagBranchOrCommitDelimiter)
 
-	// input can have been set with version in two diff ways
+	// input can have been set with version in few diff ways
 	// 1- github.com/kurtosis-tech/sample-dependency-package/main.star@branch-or-version (when is called from cli run command)
 	// 2- github.com/kurtosis-tech/sample-dependency-package@branch-or-version/main.star (when is declared in the replace section of the kurtosis.yml file)
+	// 3- github.com/kurtosis-tech/sample-dependency-package/main.star@foo/bar - here the tag is foo/bar;
+	// 4- github.com/kurtosis-tech/sample-dependency-package@foo/bar/mains.tar - here the tag is foo/bar; while file is /kurtosis-tech/sample-dependency-package/main.star
 	// we check if there is a file in maybeTagBranchOrCommitWithFile and then add it to pathWithoutVersion
-	maybeTagBranchOrCommit, maybeFileNameAndExtension, _ := strings.Cut(maybeTagBranchOrCommitWithFile, urlPathSeparator)
+	maybeTagBranchOrCommit, lastSectionOfTagBranchCommitWithFile, _ := cutLast(maybeTagBranchOrCommitWithFile, urlPathSeparator)
 
-	if maybeFileNameAndExtension != "" {
+	if lastSectionOfTagBranchCommitWithFile != "" && strings.Contains(lastSectionOfTagBranchCommitWithFile, extensionCharacter) {
 		// we assume pathWithoutVersion does not contain a file inside yet
-		pathWithoutVersion = path.Join(pathWithoutVersion, maybeFileNameAndExtension)
+		pathWithoutVersion = path.Join(pathWithoutVersion, lastSectionOfTagBranchCommitWithFile)
+	} else if lastSectionOfTagBranchCommitWithFile != "" && !strings.Contains(lastSectionOfTagBranchCommitWithFile, extensionCharacter) {
+		maybeTagBranchOrCommit = path.Join(maybeTagBranchOrCommit, lastSectionOfTagBranchCommitWithFile)
 	}
 
 	return pathWithoutVersion, maybeTagBranchOrCommit
 }
+
+func cutLast(pathToCut string, separator string) (string, string, bool) {
+	lastIndex := strings.LastIndex(pathToCut, separator)
+	if lastIndex == substrNotPresent {
+		return pathToCut, "", false
+	}
+	return pathToCut[:lastIndex], pathToCut[lastIndex+1:], false
+}

core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url_test.go

@@ -13,6 +13,8 @@ const (
 	githubSampleURL                                        = "github.com/" + testModuleAuthor + "/" + testModuleName + "/" + testFileName
 	githubSampleUrlWithTag                                 = githubSampleURL + "@5.33.2"
 	githubSampleUrlWithBranchContainingVersioningDelimiter = githubSampleURL + "@my@favorite-branch"
+	githubSampleUrlWithVersionWithSlash                    = "github.com/kurtosis-tech/sample-startosis-load/sample.star@foo/bar"
+	githubSampleUrlWithVersionWithSlashAndFile             = "github.com/kurtosis-tech/sample-startosis-load@foo/bar/main.star"
 )
 
 func TestParsedGitURL_SimpleParse(t *testing.T) {
@@ -153,3 +155,15 @@ func TestParsedGitUrl_ResolvesRelativeUrlForUrlWithTag(t *testing.T) {
 	expected = "github.com/kurtosis-tech/sample-startosis-load/src/lib.star"
 	require.Equal(t, expected, absoluteUrl)
 }
+
+func TestParsedGitUrl_ResolvesWithUrlWithVersionBranchWithSlash(t *testing.T) {
+	parsedUrl, err := parseGitURL(githubSampleUrlWithVersionWithSlash)
+	require.Nil(t, err)
+
+	require.Equal(t, "foo/bar", parsedUrl.tagBranchOrCommit)
+
+	parsedUrl, err = parseGitURL(githubSampleUrlWithVersionWithSlashAndFile)
+	require.Nil(t, err)
+	require.Equal(t, "foo/bar", parsedUrl.tagBranchOrCommit)
+	require.Equal(t, "kurtosis-tech/sample-startosis-load/main.star", parsedUrl.relativeFilePath)
+}

core/server/api_container/server/startosis_engine/startosis_packages/mock_package_content_provider/mock_package_content_provider.go

@@ -72,7 +72,11 @@ func (provider *MockPackageContentProvider) GetModuleContents(fileInsidePackageU
 	return string(fileContent), nil
 }
 
-func (provider *MockPackageContentProvider) GetAbsoluteLocatorForRelativeLocator(_ string, relativeOrAbsoluteModulePath string, packageReplaceOptions map[string]string) (string, *startosis_errors.InterpretationError) {
+func (provider *MockPackageContentProvider) GetAbsoluteLocatorForRelativeLocator(parentModuleId string, relativeOrAbsoluteModulePath string, packageReplaceOptions map[string]string) (string, *startosis_errors.InterpretationError) {
+	if strings.HasPrefix(relativeOrAbsoluteModulePath, parentModuleId) {
+		return "", startosis_errors.NewInterpretationError("Cannot use local absolute locators")
+	}
+
 	if strings.HasPrefix(relativeOrAbsoluteModulePath, startosis_constants.GithubDomainPrefix) {
 		return relativeOrAbsoluteModulePath, nil
 	}