Skip to content

Commit

Permalink
fix: block local absolute locators (#1659)
Browse files Browse the repository at this point in the history
## Description:
block local absolute locators was not working appropriately 

## Is this change user facing?
YES

## References (if applicable):
Fix #1637
  • Loading branch information
leoporoli authored Oct 31, 2023
1 parent cbf1b6b commit a4daeb3
Show file tree
Hide file tree
Showing 33 changed files with 374 additions and 116 deletions.
2 changes: 1 addition & 1 deletion api/golang/core/lib/shared_utils/parsed_git_url.go
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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!"))
}
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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())
}
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit a4daeb3

Please sign in to comment.