Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: block local absolute locators #1659

Merged
merged 9 commits into from Oct 31, 2023
Merged

fix: block local absolute locators #1659

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c0a59db
should be fixed with this changes
leoporoli Oct 29, 2023
b57dce7
Merge branch 'main' into lporoli/ticket-1637
leoporoli Oct 30, 2023
f022df0
add Kevin tests
leoporoli Oct 30, 2023
9d196be
add relative locator test
leoporoli Oct 31, 2023
bce1ea8
Merge branch 'main' into lporoli/ticket-1637
leoporoli Oct 31, 2023
9954a3c
Merge branch 'main' into lporoli/ticket-1637
leoporoli Oct 31, 2023
101dfca
fix test
leoporoli Oct 31, 2023
f2028a5
Internal tests added
leoporoli Oct 31, 2023
2057866
Merge branch 'main' into lporoli/ticket-1637
leoporoli Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/golang/core/lib/shared_utils/parsed_git_url.go
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@ func ParseGitURL(packageURL string) (*ParsedGitURL, error) {
if err != nil {
return nil, stacktrace.Propagate(err, "Error parsing the URL with scheme for module '%v'", packageURLPrefixedWithHttps)
}
if parsedURL.Host != GithubDomainPrefix {
if strings.ToLower(parsedURL.Host) != GithubDomainPrefix {
return nil, stacktrace.NewError("Error parsing the URL of module. We only support modules on Github for now but got '%v'", packageURL)
}

Expand Down
4 changes: 3 additions & 1 deletion core/server/api_container/server/startosis_engine/builtins/import_module/import_module.go
View file
Open in desktop
Expand Up @@ -55,6 +55,7 @@ func NewImportModule(
},

Capabilities: &importModuleCapabilities{
packageId: packageId,
packageContentProvider: packageContentProvider,
recursiveInterpret: recursiveInterpret,
moduleGlobalCache: moduleGlobalCache,
Expand All @@ -64,6 +65,7 @@ func NewImportModule(
}

type importModuleCapabilities struct {
packageId string
packageContentProvider startosis_packages.PackageContentProvider
recursiveInterpret func(moduleId string, scriptContent string) (starlark.StringDict, *startosis_errors.InterpretationError)
moduleGlobalCache map[string]*startosis_packages.ModuleCacheEntry
Expand All @@ -76,7 +78,7 @@ func (builtin *importModuleCapabilities) Interpret(locatorOfModuleInWhichThisBui
return nil, explicitInterpretationError(err)
}
relativeOrAbsoluteModuleLocator := moduleLocatorArgValue.GoString()
absoluteModuleLocator, relativePathParsingInterpretationErr := builtin.packageContentProvider.GetAbsoluteLocatorForRelativeLocator(locatorOfModuleInWhichThisBuiltInIsBeingCalled, relativeOrAbsoluteModuleLocator, builtin.packageReplaceOptions)
absoluteModuleLocator, relativePathParsingInterpretationErr := builtin.packageContentProvider.GetAbsoluteLocator(builtin.packageId, locatorOfModuleInWhichThisBuiltInIsBeingCalled, relativeOrAbsoluteModuleLocator, builtin.packageReplaceOptions)
if relativePathParsingInterpretationErr != nil {
return nil, relativePathParsingInterpretationErr
}
Expand Down
4 changes: 3 additions & 1 deletion core/server/api_container/server/startosis_engine/builtins/read_file/read_file.go
View file
Open in desktop
Expand Up @@ -36,13 +36,15 @@ func NewReadFileHelper(
},

Capabilities: &readFileCapabilities{
packageId: packageId,
packageContentProvider: packageContentProvider,
packageReplaceOptions: packageReplaceOptions,
},
}
}

type readFileCapabilities struct {
packageId string
packageContentProvider startosis_packages.PackageContentProvider
packageReplaceOptions map[string]string
}
Expand All @@ -53,7 +55,7 @@ func (builtin *readFileCapabilities) Interpret(locatorOfModuleInWhichThisBuiltIn
return nil, startosis_errors.WrapWithInterpretationError(err, "Unable to extract value for arg '%s'", srcValue)
}
fileToReadStr := srcValue.GoString()
fileToReadStr, relativePathParsingInterpretationErr := builtin.packageContentProvider.GetAbsoluteLocatorForRelativeLocator(locatorOfModuleInWhichThisBuiltInIsBeingCalled, fileToReadStr, builtin.packageReplaceOptions)
fileToReadStr, relativePathParsingInterpretationErr := builtin.packageContentProvider.GetAbsoluteLocator(builtin.packageId, locatorOfModuleInWhichThisBuiltInIsBeingCalled, fileToReadStr, builtin.packageReplaceOptions)
if relativePathParsingInterpretationErr != nil {
return nil, relativePathParsingInterpretationErr
}
Expand Down
4 changes: 3 additions & 1 deletion ...r/api_container/server/startosis_engine/kurtosis_instruction/upload_files/upload_files.go
View file
Open in desktop
Expand Up @@ -68,6 +68,7 @@ func NewUploadFiles(
archivePathOnDisk: "", // populated at interpretation time
filesArtifactMd5: nil, // populated at interpretation time
packageReplaceOptions: packageReplaceOptions,
packageId: packageId,
}
},

Expand All @@ -87,6 +88,7 @@ type UploadFilesCapabilities struct {
archivePathOnDisk string
filesArtifactMd5 []byte
packageReplaceOptions map[string]string
packageId string
}

func (builtin *UploadFilesCapabilities) Interpret(locatorOfModuleInWhichThisBuiltInIsBeingCalled string, arguments *builtin_argument.ArgumentValuesSet) (starlark.Value, *startosis_errors.InterpretationError) {
Expand All @@ -109,7 +111,7 @@ func (builtin *UploadFilesCapabilities) Interpret(locatorOfModuleInWhichThisBuil
return nil, startosis_errors.WrapWithInterpretationError(err, "Unable to extract value for '%s' argument", SrcArgName)
}

absoluteLocator, interpretationErr := builtin.packageContentProvider.GetAbsoluteLocatorForRelativeLocator(locatorOfModuleInWhichThisBuiltInIsBeingCalled, src.GoString(), builtin.packageReplaceOptions)
absoluteLocator, interpretationErr := builtin.packageContentProvider.GetAbsoluteLocator(builtin.packageId, locatorOfModuleInWhichThisBuiltInIsBeingCalled, src.GoString(), builtin.packageReplaceOptions)
if interpretationErr != nil {
return nil, startosis_errors.WrapWithInterpretationError(interpretationErr, "Tried to convert locator '%v' into absolute locator but failed", src.GoString())
}
Expand Down
2 changes: 1 addition & 1 deletion .../startosis_engine/kurtosis_starlark_framework/test_engine/import_module_framework_test.go
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ func (suite *KurtosisHelperTestSuite) TestImportFile() {
moduleGlobalCache := map[string]*startosis_packages.ModuleCacheEntry{}

suite.packageContentProvider.EXPECT().GetModuleContents(testModuleFileName).Return("Hello World!", nil)
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions).Return(testModuleFileName, nil)
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions).Return(testModuleFileName, nil)

suite.run(&importModuleTestCase{
T: suite.T(),
Expand Down
4 changes: 2 additions & 2 deletions ...tarlark_framework/test_engine/import_module_with_local_absolute_locator_framework_test.go
View file
Open in desktop
Expand Up @@ -33,13 +33,13 @@ type importModuleWithLocalAbsoluteLocatorTestCase struct {
}

func (suite *KurtosisHelperTestSuite) TestImportFileWithLocalAbsoluteLocatorShouldNotBeValid() {
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleFileName, testNoPackageReplaceOptions).Return("", startosis_errors.NewInterpretationError(importModuleWithLocalAbsoluteLocatorExpectedErrorMsg))
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleMainFileLocator, 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,
testModuleMainFileLocator,
&importModuleWithLocalAbsoluteLocatorTestCase{
T: suite.T(),
moduleGlobalCache: moduleGlobalCache,
Expand Down
4 changes: 2 additions & 2 deletions ...rver/startosis_engine/kurtosis_starlark_framework/test_engine/read_file_framework_test.go
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ type readFileTestCase struct {
}

func (suite *KurtosisHelperTestSuite) TestReadFile() {
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions).Return(testModuleFileName, nil)
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions).Return(testModuleFileName, nil)
suite.packageContentProvider.EXPECT().GetModuleContents(testModuleFileName).Return("Hello World!", nil)

suite.run(&readFileTestCase{
Expand All @@ -40,7 +40,7 @@ func (t *readFileTestCase) GetStarlarkCodeForAssertion() string {
}

func (t *readFileTestCase) Assert(result starlark.Value) {
t.packageContentProvider.AssertCalled(t, "GetAbsoluteLocatorForRelativeLocator", startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions)
t.packageContentProvider.AssertCalled(t, "GetAbsoluteLocator", testModulePackageId, startosis_constants.PackageIdPlaceholderForStandaloneScript, testModuleRelativeLocator, testNoPackageReplaceOptions)
t.packageContentProvider.AssertCalled(t, "GetModuleContents", testModuleFileName)
require.Equal(t, result, starlark.String("Hello World!"))
}
4 changes: 2 additions & 2 deletions ...is_starlark_framework/test_engine/read_file_with_local_absolute_locator_framework_test.go
View file
Open in desktop
Expand Up @@ -21,10 +21,10 @@ type readFileWithLocalAbsoluteLocatorTestCase struct {
}

func (suite *KurtosisHelperTestSuite) TestReadFileWithLocalAbsoluteLocatorShouldNotBeValid() {
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleFileName, testNoPackageReplaceOptions).Return("", startosis_errors.NewInterpretationError(readFileWithLocalAbsoluteLocatorExpectedErrorMsg))
suite.packageContentProvider.EXPECT().GetAbsoluteLocatorForRelativeLocator(testModulePackageId, testModuleMainFileLocator, testModuleFileName, testNoPackageReplaceOptions).Return("", startosis_errors.NewInterpretationError(readFileWithLocalAbsoluteLocatorExpectedErrorMsg))

suite.runShouldFail(
testModulePackageId,
testModuleMainFileLocator,
&readFileWithLocalAbsoluteLocatorTestCase{
T: suite.T(),
packageContentProvider: suite.packageContentProvider,
Expand Down
1 change: 1 addition & 0 deletions ...ainer/server/startosis_engine/kurtosis_starlark_framework/test_engine/static_constants.go
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ var (
testSrcPath = "/path/to/file.txt"

testModulePackageId = "github.com/kurtosistech/test-package"
testModuleMainFileLocator = "github.com/kurtosistech/test-package/main.star"
testModuleFileName = "github.com/kurtosistech/test-package/helpers.star"
testModuleRelativeLocator = "./helpers.star"

Expand Down
4 changes: 2 additions & 2 deletions ...er/startosis_engine/kurtosis_starlark_framework/test_engine/suite_kurtosis_helper_test.go
View file
Open in desktop
Expand Up @@ -45,13 +45,13 @@ func (suite *KurtosisHelperTestSuite) run(builtin KurtosisHelperBaseTest) {
builtin.Assert(result)
}

func (suite *KurtosisHelperTestSuite) runShouldFail(packageId string, builtin KurtosisHelperBaseTest, expectedErrMsg string) {
func (suite *KurtosisHelperTestSuite) runShouldFail(moduleName 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, packageId, codeToExecute(starlarkCode), suite.starlarkEnv)
_, err := starlark.ExecFile(suite.starlarkThread, moduleName, 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())
}
24 changes: 14 additions & 10 deletions ...is_engine/startosis_packages/git_package_content_provider/git_package_content_provider.go
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@ const (
)

type GitPackageContentProvider struct {
// Where to temporarily store packages while
packagesTmpDir string
packagesDir string
packageReplaceOptionsRepository *packageReplaceOptionsRepository
Expand Down Expand Up @@ -205,28 +206,31 @@ func (provider *GitPackageContentProvider) StorePackageContents(packageId string
return packageAbsolutePathOnDisk, nil
}

func (provider *GitPackageContentProvider) GetAbsoluteLocatorForRelativeLocator(
parentModuleId string,
maybeRelativeLocator string,
func (provider *GitPackageContentProvider) GetAbsoluteLocator(
packageId string,
sourceModuleLocator string,
relativeOrAbsoluteLocator string,
packageReplaceOptions map[string]string,
) (string, *startosis_errors.InterpretationError) {
var absoluteLocator string

if isSamePackageLocalAbsoluteLocator(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)
if shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage(relativeOrAbsoluteLocator, sourceModuleLocator, packageId) {
return "", startosis_errors.NewInterpretationError("Locator '%s' is referencing a file within the same package using absolute import syntax, but only relative import syntax (path starting with '/' or '.') is allowed for within-package imports", relativeOrAbsoluteLocator)
}

// maybe it's not a relative url in which case we return the url
_, errorParsingUrl := shared_utils.ParseGitURL(maybeRelativeLocator)
_, errorParsingUrl := shared_utils.ParseGitURL(relativeOrAbsoluteLocator)
if errorParsingUrl == nil {
absoluteLocator = maybeRelativeLocator
// Parsing succeeded, meaning this is already an absolute locator and no relative -> absolute translation is needed
absoluteLocator = relativeOrAbsoluteLocator
} else {
parsedParentModuleId, errorParsingPackageId := shared_utils.ParseGitURL(parentModuleId)
// Parsing did not succeed, meaning this is a relative locator
sourceModuleParsedGitUrl, errorParsingPackageId := shared_utils.ParseGitURL(sourceModuleLocator)
if errorParsingPackageId != nil {
return "", startosis_errors.NewInterpretationError("Parent package id '%v' isn't a valid locator; relative URLs don't work with standalone scripts", parentModuleId)
return "", startosis_errors.NewInterpretationError("Source module locator '%v' isn't a valid locator; relative URLs don't work with standalone scripts", sourceModuleLocator)
}

absoluteLocator = parsedParentModuleId.GetAbsoluteLocatorRelativeToThisURL(maybeRelativeLocator)
absoluteLocator = sourceModuleParsedGitUrl.GetAbsoluteLocatorRelativeToThisURL(relativeOrAbsoluteLocator)
}

replacedAbsoluteLocator := replaceAbsoluteLocator(absoluteLocator, packageReplaceOptions)
Expand Down