From cda001d08b9d332a18d661ac8eae2c081511c538 Mon Sep 17 00:00:00 2001 From: Gyanendra Mishra Date: Mon, 9 Oct 2023 15:15:48 +0100 Subject: [PATCH] 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 --- .../builtins/import_module/import_module.go | 2 +- .../builtins/read_file/read_file.go | 2 +- .../upload_files/upload_files.go | 2 +- .../builtin_argument/validators.go | 23 ------------------- ...h_local_absolute_locator_framework_test.go | 5 +++- ...h_local_absolute_locator_framework_test.go | 7 ++++-- .../test_engine/suite_kurtosis_helper_test.go | 4 ++-- .../suite_kurtosis_plan_instruction_test.go | 4 ++-- ...h_local_absolute_locator_framework_test.go | 3 ++- .../git_package_content_provider.go | 8 +++++++ .../parsed_git_url.go | 22 ++++++++++++++---- .../parsed_git_url_test.go | 14 +++++++++++ .../mock_package_content_provider.go | 6 ++++- 13 files changed, 63 insertions(+), 39 deletions(-) diff --git a/core/server/api_container/server/startosis_engine/builtins/import_module/import_module.go b/core/server/api_container/server/startosis_engine/builtins/import_module/import_module.go index 83f4bac05f..40daa464b8 100644 --- a/core/server/api_container/server/startosis_engine/builtins/import_module/import_module.go +++ b/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) }, }, }, diff --git a/core/server/api_container/server/startosis_engine/builtins/read_file/read_file.go b/core/server/api_container/server/startosis_engine/builtins/read_file/read_file.go index c4173a5bab..6ab3d5197a 100644 --- a/core/server/api_container/server/startosis_engine/builtins/read_file/read_file.go +++ b/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) }, }, }, diff --git a/core/server/api_container/server/startosis_engine/kurtosis_instruction/upload_files/upload_files.go b/core/server/api_container/server/startosis_engine/kurtosis_instruction/upload_files/upload_files.go index eed4e32cae..cee28132b0 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_instruction/upload_files/upload_files.go +++ b/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) }, }, { diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/builtin_argument/validators.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/builtin_argument/validators.go index 8f12e8abd3..2ec22580d4 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/builtin_argument/validators.go +++ b/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) -} diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/import_module_with_local_absolute_locator_framework_test.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/import_module_with_local_absolute_locator_framework_test.go index 67f9404ca2..e4980692a2 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/import_module_with_local_absolute_locator_framework_test.go +++ b/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, diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/read_file_with_local_absolute_locator_framework_test.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/read_file_with_local_absolute_locator_framework_test.go index 20babbbe75..661a98e71c 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/read_file_with_local_absolute_locator_framework_test.go +++ b/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, diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_helper_test.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_helper_test.go index ea4ad16d9b..423fbaeb8c 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_helper_test.go +++ b/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()) } diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_plan_instruction_test.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_plan_instruction_test.go index 2d6bc4c255..31db6acde1 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_plan_instruction_test.go +++ b/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()) } diff --git a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/upload_files_with_local_absolute_locator_framework_test.go b/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/upload_files_with_local_absolute_locator_framework_test.go index 2a4ac1557a..62ef6da768 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_starlark_framework/test_engine/upload_files_with_local_absolute_locator_framework_test.go +++ b/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, diff --git a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider.go b/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider.go index a0c4dccbfe..fcc483b0a2 100644 --- a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider.go +++ b/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) +} diff --git a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url.go b/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url.go index 32136cb138..89e006079e 100644 --- a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url.go +++ b/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 +} diff --git a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url_test.go b/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url_test.go index c22a929df9..60ea2b0d25 100644 --- a/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/parsed_git_url_test.go +++ b/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) +} diff --git a/core/server/api_container/server/startosis_engine/startosis_packages/mock_package_content_provider/mock_package_content_provider.go b/core/server/api_container/server/startosis_engine/startosis_packages/mock_package_content_provider/mock_package_content_provider.go index 433408bc03..acff0a36a3 100644 --- a/core/server/api_container/server/startosis_engine/startosis_packages/mock_package_content_provider/mock_package_content_provider.go +++ b/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 }