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
4 changes: 4 additions & 0 deletions cmd/project/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,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 +67,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 +81,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 within the template to use as project root")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: An attempt to shorten the description so that it doesn't wrap on 80 character terminals

Suggested change
cmd.Flags().StringVar(&createSubdirFlag, "subdir", "", "subdirectory within the template to use as project root")
cmd.Flags().StringVar(&createSubdirFlag, "subdir", "", "subdirectory in the template to use as project")

Before:
Image

After:
Image


return cmd
}
Expand Down Expand Up @@ -141,6 +144,7 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []
AppName: appNameArg,
Template: template,
GitBranch: createGitBranchFlag,
Subdir: createSubdirFlag,
Copy link
Member

Choose a reason for hiding this comment

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

question: Right now --subdir without --template is silently accepted. For example, slack create --subdir src will use the src/ of whatever template is selected in the prompts.

Personally, I'd learn toward erroring if --subdir is used without --template because it avoids unexpected behaviour or exploits and we can "loosen" that contraint later if we decided we want that experience.

}
clients.EventTracker.SetAppTemplate(template.GetTemplatePath())

Expand Down
46 changes: 46 additions & 0 deletions cmd/project/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,52 @@ func TestCreateCommand(t *testing.T) {
cm.IO.AssertNotCalled(t, "SelectPrompt", mock.Anything, "Select an app:", 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
70 changes: 68 additions & 2 deletions internal/pkg/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ 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 +122,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 +130 to +137
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!

}

// Change into the project directory to configure defaults and dependencies
Expand Down Expand Up @@ -343,6 +355,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 +358 to +359
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 +363 to +366
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 strings.HasPrefix(cleaned, "..") || filepath.IsAbs(cleaned) {
return "", slackerror.New(slackerror.ErrSubdirNotFound).
WithMessage("subdirectory path %q must be relative and within the template", subdir)
}
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 {
tmpDir, err := os.MkdirTemp("", "slack-create-*")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: We should not use os. calls directly whenever possible. These calls are very hard to test because we can't mock it - it will create a temp directory during the unit test.

Instead, we should use our fs afero.Fs filesystem library. This uses os in production and a memory-based file system in tests.

I imagine we can use afero.GetTempDir() and afero.TempDir() to look something like:

tempDirRoot := afero.GetTempDir(fs, "")
tempDirPath, err := afero.TempDir(fs, tmpDirRoot, "slack-create-")

if err != nil {
return slackerror.Wrap(err, "failed to create temporary directory")
}
// Remove so createApp can create it fresh (go-git requires non-existent target)
os.Remove(tmpDir)
Comment on lines +381 to +382
Copy link
Member

Choose a reason for hiding this comment

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

question: Why do we make the temp directory on L377 and then immediately delete it?

defer os.RemoveAll(tmpDir)
Comment on lines +382 to +383
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Likewise, here I think we should use fs so that we can write unit tests that do not modify the actual file system.


if err := createApp(ctx, tmpDir, template, gitBranch, log, fs); err != nil {
return err
}
Comment on lines +385 to +387
Copy link
Member

Choose a reason for hiding this comment

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

praise: Clever and clean approach!


subdirPath := filepath.Join(tmpDir, subdir)
info, err := os.Stat(subdirPath)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: If we're using the fs then it's important that we'll need to use fs.Stat(subdirPath) so that it reads the real file system in production and memory-based file system during tests.

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).
WithRemediation("Check that the path exists in the template at %q", template.GetTemplatePath())
Comment on lines +393 to +395
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)
}

return goutils.CopyDirectory(goutils.CopyDirectoryOpts{
Src: subdirPath,
Dst: dirPath,
IgnoreDirectories: []string{".git", ".venv", "node_modules"},
IgnoreFiles: []string{".DS_Store"},
})
Comment on lines +404 to +409
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 noticed that we have the same IgnoreDirectories and IgnoreFiles on L337. Thoughts on pulling those two slices out to use in both places (less error prone if we need to ignore more files/directories). Alternatively, this could become a function used in both places.

}

// 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
124 changes: 124 additions & 0 deletions internal/pkg/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ package create
import (
"fmt"
"net/http"
"os"
"path/filepath"
"testing"

"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 @@ -183,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) string
subdir string
expectError bool
errorContains string
expectFiles []string
}{
"extracts subdirectory from local template": {
setupTemplate: func(t *testing.T) string {
tmpDir := t.TempDir()
// Create a subdirectory with a file
subdir := filepath.Join(tmpDir, "apps", "my-app")
require.NoError(t, os.MkdirAll(subdir, 0755))
require.NoError(t, os.WriteFile(filepath.Join(subdir, "manifest.json"), []byte(`{}`), 0644))
// Create a file at root that should NOT be copied
require.NoError(t, os.WriteFile(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) 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) string {
tmpDir := t.TempDir()
require.NoError(t, os.WriteFile(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) {
templateDir := tc.setupTemplate(t)
outputDir := t.TempDir()
// Remove output dir so CopyDirectory can create it
require.NoError(t, os.Remove(outputDir))

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

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 := os.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"
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