Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cmd/project/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/create"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
"github.com/spf13/cobra"
Expand All @@ -33,6 +34,7 @@ var createTemplateURLFlag string
var createGitBranchFlag string
var createAppNameFlag string
var createListFlag bool
var createSubdirFlag string

// Handle to client's create function used for testing
// TODO - Find best practice, such as using an Interface and Struct to create a client
Expand Down Expand Up @@ -66,6 +68,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`,
{Command: "create agent my-agent-app", Meaning: "Create a new AI Agent app"},
{Command: "create my-project -t slack-samples/deno-hello-world", Meaning: "Start a new project from a specific template"},
{Command: "create --name my-project", Meaning: "Create a project named 'my-project'"},
{Command: "create my-project -t org/monorepo --subdir apps/my-app", Meaning: "Create from a subdirectory of a template"},
}),
Args: cobra.MaximumNArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -79,6 +82,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`,
cmd.Flags().StringVarP(&createGitBranchFlag, "branch", "b", "", "name of git branch to checkout")
cmd.Flags().StringVarP(&createAppNameFlag, "name", "n", "", "name for your app (overrides the name argument)")
cmd.Flags().BoolVar(&createListFlag, "list", false, "list available app templates")
cmd.Flags().StringVar(&createSubdirFlag, "subdir", "", "subdirectory in the template to use as project")

return cmd
}
Expand Down Expand Up @@ -128,6 +132,12 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []
return listTemplates(ctx, clients, categoryShortcut)
}

// --subdir requires --template
if cmd.Flags().Changed("subdir") && !templateFlagProvided {
return slackerror.New(slackerror.ErrMismatchedFlags).
WithMessage("the --subdir flag requires the --template flag")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WithMessage("the --subdir flag requires the --template flag")
WithMessage("The --subdir flag requires the --template flag")

🏁 polish: To match surrounding error messages. Though I'm not confident in how consistent we are with this... Let's take note to include this in a Style Guide soon!

}

// Collect the template URL or select a starting template
template, err := promptTemplateSelection(cmd, clients, categoryShortcut)
if err != nil {
Expand All @@ -141,6 +151,7 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []
AppName: appNameArg,
Template: template,
GitBranch: createGitBranchFlag,
Subdir: createSubdirFlag,
}
clients.EventTracker.SetAppTemplate(template.GetTemplatePath())

Expand Down
57 changes: 57 additions & 0 deletions cmd/project/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,63 @@ func TestCreateCommand(t *testing.T) {
cm.IO.AssertNotCalled(t, "SelectPrompt", mock.Anything, "Select an app:", mock.Anything, mock.Anything)
},
},
"subdir without template flag returns error": {
CmdArgs: []string{"--subdir", "apps/my-app"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
createClientMock = new(CreateClientMock)
CreateFunc = createClientMock.Create
},
ExpectedErrorStrings: []string{"the --subdir flag requires the --template flag"},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
createClientMock.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
},
},
"passes subdir flag to create function": {
CmdArgs: []string{"--template", "slack-samples/bolt-js-starter-template", "--subdir", "apps/my-app"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.IO.On("SelectPrompt", mock.Anything, "Select an app:", mock.Anything, mock.Anything).
Return(
iostreams.SelectPromptResponse{
Flag: true,
Option: "slack-samples/bolt-js-starter-template",
},
nil,
)
cm.IO.On("SelectPrompt", mock.Anything, "Select a language:", mock.Anything, mock.Anything).
Return(
iostreams.SelectPromptResponse{
Flag: true,
Option: "slack-samples/bolt-js-starter-template",
},
nil,
)
createClientMock = new(CreateClientMock)
createClientMock.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil)
CreateFunc = createClientMock.Create
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
template, err := create.ResolveTemplateURL("slack-samples/bolt-js-starter-template")
require.NoError(t, err)
expected := create.CreateArgs{
Template: template,
Subdir: "apps/my-app",
}
createClientMock.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, expected)
},
},
"list flag ignores subdir": {
CmdArgs: []string{"--list", "--subdir", "foo"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
createClientMock = new(CreateClientMock)
CreateFunc = createClientMock.Create
},
ExpectedOutputs: []string{
"Getting started",
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
createClientMock.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
},
},
"lists all templates with --list flag": {
CmdArgs: []string{"--list"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
Expand Down
80 changes: 76 additions & 4 deletions internal/pkg/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,18 @@ import (
"github.com/spf13/afero"
)

// copyIgnoreDirectories are directories to skip when copying a template.
var copyIgnoreDirectories = []string{".git", ".venv", "node_modules"}

// copyIgnoreFiles are files to skip when copying a template.
var copyIgnoreFiles = []string{".DS_Store"}

// CreateArgs are the arguments passed into the Create function
type CreateArgs struct {
AppName string
Template Template
GitBranch string
Subdir string
}

// Create will create a new Slack app on the file system and app manifest on the Slack API.
Expand Down Expand Up @@ -121,8 +128,19 @@ func Create(ctx context.Context, clients *shared.ClientFactory, log *logger.Logg
}))

// Create the project from a templateURL
if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
subdir, err := normalizeSubdir(createArgs.Subdir)
if err != nil {
return "", err
}

if subdir != "" {
if err := createAppFromSubdir(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, subdir, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
}
} else {
if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil {
return "", slackerror.Wrap(err, slackerror.ErrAppCreate)
}
Comment on lines +136 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: clean implementation choice!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪬 question(non-blocking): Would passing subdir to createApp in all instances - either a default "." or the normalized path - reduce this implementation more? I'm concerned that a change to the temporary directory logic might require repeated change if we keep separate setups for this.

}

// Change into the project directory to configure defaults and dependencies
Expand Down Expand Up @@ -325,8 +343,8 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch
copyDirectoryOpts := goutils.CopyDirectoryOpts{
Src: template.path,
Dst: dirPath,
IgnoreDirectories: []string{".git", ".venv", "node_modules"},
IgnoreFiles: []string{".DS_Store"},
IgnoreDirectories: copyIgnoreDirectories,
IgnoreFiles: copyIgnoreFiles,
}
if err := goutils.CopyDirectory(copyDirectoryOpts); err != nil {
return slackerror.Wrap(err, "error copying local template")
Expand All @@ -343,6 +361,60 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch
return nil
}

// normalizeSubdir cleans the subdir path and returns "" if it resolves to root.
func normalizeSubdir(subdir string) (string, error) {
Comment on lines +364 to +365
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: very nice! 👌🏻

if subdir == "" {
return "", nil
}
cleaned := filepath.Clean(subdir)
if cleaned == "." || cleaned == "/" {
return "", nil
}
Comment on lines +369 to +372
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: After filepath.Clean() maybe we should use filepath.isLocal(cleaned).

This function appears to check if the file path is within the subtree and prevent traversal attacks where the path tries to access files outside of the root directory (template directory).

I haven't used it personally, but it seems like a good security measure.

Image

if !filepath.IsLocal(cleaned) {
return "", slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("subdirectory path %q must be relative and within the template", subdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WithMessage("subdirectory path %q must be relative and within the template", subdir)
WithMessage("Subdirectory path %q must be relative and within the template", subdir)

🧮 quibble: Casing to errors as a complete sentence!

}
return cleaned, nil
}

// createAppFromSubdir clones the full template into a temp directory, then copies
// only the specified subdirectory to the final project path.
func createAppFromSubdir(ctx context.Context, dirPath string, template Template, gitBranch string, subdir string, log *logger.Logger, fs afero.Fs) error {
tmpDirRoot := afero.GetTempDir(fs, "")
tmpDir, err := afero.TempDir(fs, tmpDirRoot, "slack-create-")
if err != nil {
return slackerror.Wrap(err, "failed to create temporary directory")
}
defer func() { _ = fs.RemoveAll(tmpDir) }()

cloneDir := filepath.Join(tmpDir, "repo")
if err := createApp(ctx, cloneDir, template, gitBranch, log, fs); err != nil {
return err
}

subdirPath := filepath.Join(cloneDir, subdir)
info, err := fs.Stat(subdirPath)
if err != nil {
if os.IsNotExist(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I think this is okay, but if you want to mock the result during tests (to test both exist and not exist stats) then you can use the clients.Os.IsNotExist(err) method. Usually we do this by passing clients.Os in function similar to fs (which is clients.Fs).

return slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("subdirectory %q was not found in the template", subdir).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WithMessage("subdirectory %q was not found in the template", subdir).
WithMessage("Subdirectory %q was not found in the template", subdir).

🚢 quibble: Casings to the sentence structure!

WithRemediation("Check that the path exists in the template at %q", template.GetTemplatePath())
Comment on lines +399 to +401
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: %q vs %s

}
return slackerror.Wrap(err, "failed to access subdirectory")
}
if !info.IsDir() {
return slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("path %q in the template is not a directory", subdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WithMessage("path %q in the template is not a directory", subdir)
WithMessage("Path %q in the template is not a directory", subdir)

📠 quibble: Forbid this error be reached, but let's start with a capital path here.

}

return goutils.CopyDirectory(goutils.CopyDirectoryOpts{
Src: subdirPath,
Dst: dirPath,
IgnoreDirectories: copyIgnoreDirectories,
IgnoreFiles: copyIgnoreFiles,
})
}

// InstallProjectDependencies installs the project runtime dependencies or
// continues with next steps if that fails. You can specify the manifestSource
// for the project configuration file (default: ManifestSourceLocal)
Expand Down
123 changes: 123 additions & 0 deletions internal/pkg/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/experiment"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackhttp"
Expand Down Expand Up @@ -184,6 +185,128 @@ func TestCreateGitArgs(t *testing.T) {
assert.Equal(t, expectedArgs, testGitArgs)
}

func TestNormalizeSubdir(t *testing.T) {
tests := map[string]struct {
input string
expected string
expectError bool
}{
"empty string returns empty": {
input: "",
expected: "",
},
"dot returns empty": {
input: ".",
expected: "",
},
"slash returns empty": {
input: "/",
expected: "",
},
"simple subdir": {
input: "pydantic-ai/",
expected: "pydantic-ai",
},
"dot-prefixed subdir": {
input: "./my-app",
expected: "my-app",
},
"nested subdir": {
input: "apps/my-app",
expected: "apps/my-app",
},
"parent traversal is rejected": {
input: "../escape",
expectError: true,
},
"nested parent traversal is rejected": {
input: "foo/../../escape",
expectError: true,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
result, err := normalizeSubdir(tc.input)
if tc.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tc.expected, result)
}
})
}
}

func TestCreateAppFromSubdir(t *testing.T) {
tests := map[string]struct {
setupTemplate func(t *testing.T, fs afero.Fs) string
subdir string
expectError bool
errorContains string
expectFiles []string
}{
"extracts subdirectory from local template": {
setupTemplate: func(t *testing.T, fs afero.Fs) string {
tmpDir := t.TempDir()
// Create a subdirectory with a file
subdir := filepath.Join(tmpDir, "apps", "my-app")
require.NoError(t, fs.MkdirAll(subdir, 0755))
require.NoError(t, afero.WriteFile(fs, filepath.Join(subdir, "manifest.json"), []byte(`{}`), 0644))
// Create a file at root that should NOT be copied
require.NoError(t, afero.WriteFile(fs, filepath.Join(tmpDir, "README.md"), []byte("root readme"), 0644))
return tmpDir
},
subdir: "apps/my-app",
expectFiles: []string{"manifest.json"},
},
"returns error for nonexistent subdirectory": {
setupTemplate: func(t *testing.T, fs afero.Fs) string {
return t.TempDir()
},
subdir: "nonexistent",
expectError: true,
errorContains: "was not found in the template",
},
"returns error when subdir path is a file": {
setupTemplate: func(t *testing.T, fs afero.Fs) string {
tmpDir := t.TempDir()
require.NoError(t, afero.WriteFile(fs, filepath.Join(tmpDir, "not-a-dir"), []byte("file"), 0644))
return tmpDir
},
subdir: "not-a-dir",
expectError: true,
errorContains: "is not a directory",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
fs := afero.NewOsFs()
templateDir := tc.setupTemplate(t, fs)
outputDir := t.TempDir()
// Remove output dir so CopyDirectory can create it
require.NoError(t, fs.Remove(outputDir))

template := Template{path: templateDir, isLocal: true}
log := logger.New(func(event *logger.LogEvent) {})

err := createAppFromSubdir(t.Context(), outputDir, template, "", tc.subdir, log, fs)

if tc.expectError {
assert.Error(t, err)
if tc.errorContains != "" {
assert.Contains(t, err.Error(), tc.errorContains)
}
} else {
assert.NoError(t, err)
for _, f := range tc.expectFiles {
_, statErr := fs.Stat(filepath.Join(outputDir, f))
assert.NoError(t, statErr, "expected file %s to exist", f)
}
}
})
}
}

func Test_Create_installProjectDependencies(t *testing.T) {
tests := map[string]struct {
experiments []string
Expand Down
6 changes: 6 additions & 0 deletions internal/slackerror/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ const (
ErrSocketConnection = "socket_connection_error"
ErrScopesExceedAppConfig = "scopes_exceed_app_config"
ErrStreamingActivityLogs = "streaming_activity_logs_error"
ErrSubdirNotFound = "subdir_not_found"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 thought(non-blocking): Would it make sense to group this alongside other template errors? To me the referenced subdir wouldn't be clear at a glance. I'd suggest:

template_subdir_not_found

Which is so close to template_path_not_found!

ErrSurveyConfigNotFound = "survey_config_not_found"
ErrSystemConfigIDNotFound = "system_config_id_not_found"
ErrSystemRequirementsFailed = "system_requirements_failed"
Expand Down Expand Up @@ -1391,6 +1392,11 @@ Otherwise start your app for local development with: %s`,
Message: "Failed to stream the most recent activity logs",
},

ErrSubdirNotFound: {
Code: ErrSubdirNotFound,
Message: "The specified subdirectory was not found in the template repository",
},

ErrSurveyConfigNotFound: {
Code: ErrSurveyConfigNotFound,
Message: "Survey config not found",
Expand Down
Loading