diff --git a/Makefile b/Makefile index 43a82a6eb..21376b75b 100644 --- a/Makefile +++ b/Makefile @@ -55,9 +55,10 @@ fmt: ## Run goimports (which also formats) gometalint: ## Run every linter ever # gotype and gotypex are disabled because they don't pass on CI and https://github.com/alecthomas/gometalinter/issues/206 + # maligned is disabled because I'd rather have alphabetical struct fields than save a few bytes # gocyclo is temporarily disabled because we don't pass it right now # golint is temporarily disabled because we need to add comments everywhere first - gometalinter --disable gotype --disable gotypex --disable=gocyclo --disable golint --enable=megacheck --enable=unparam --linter='gas:gas -exclude=G104 -fmt=csv:^(?P.*?\.go),(?P\d+),(?P[^,]+,[^,]+,[^,]+)' --deadline=300s --vendor -t --line-length=120 --skip server/static ./... + gometalinter --disable gotype --disable gotypex --disable maligned --disable gocyclo --disable golint --enable=megacheck --enable=unparam --linter='gas:gas -exclude=G104 -fmt=csv:^(?P.*?\.go),(?P\d+),(?P[^,]+,[^,]+,[^,]+)' --deadline=300s --vendor -t --line-length=120 --skip server/static ./... gometalint-install: ## Install gometalint go get -u github.com/alecthomas/gometalinter diff --git a/README.md b/README.md index a53872e19..1ff5b580e 100644 --- a/README.md +++ b/README.md @@ -284,8 +284,8 @@ resource "null_resource" "null" { #### Don't Use On Public Repos Because anyone can comment on public pull requests, even with all the security mitigations available, it's still dangerous to run Atlantis on public repos until Atlantis gets an authentication system. -#### Don't Use `--allow-fork-prs=true` -If you're running on a public repo (which isn't recommended, see above) you shouldn't set `--allow-fork-prs=true` (defaults to false) +#### Don't Use `--allow-fork-prs` +If you're running on a public repo (which isn't recommended, see above) you shouldn't set `--allow-fork-prs` (defaults to false) because anyone can open up a pull request from their fork to your repo. #### Webhook Secrets diff --git a/cmd/server.go b/cmd/server.go index 6d6901688..0e855f9f0 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -19,6 +19,7 @@ import ( // 3. Add your flag's description etc. to the stringFlags, intFlags, or boolFlags slices. const ( AtlantisURLFlag = "atlantis-url" + AllowForkPRsFlag = "allow-fork-prs" ConfigFlag = "config" DataDirFlag = "data-dir" GHHostnameFlag = "gh-hostname" @@ -112,6 +113,11 @@ var stringFlags = []stringFlag{ }, } var boolFlags = []boolFlag{ + { + name: AllowForkPRsFlag, + description: "Allow Atlantis to run on pull requests from forks. A security issue for public repos.", + value: false, + }, { name: RequireApprovalFlag, description: "Require pull requests to be \"Approved\" before allowing the apply command to be run.", @@ -156,7 +162,7 @@ type ServerCmd struct { // ServerCreator creates servers. // It's an abstraction to help us test. type ServerCreator interface { - NewServer(config server.Config) (ServerStarter, error) + NewServer(config server.Config, flagNames server.FlagNames) (ServerStarter, error) } // DefaultServerCreator is the concrete implementation of ServerCreator. @@ -169,8 +175,8 @@ type ServerStarter interface { } // NewServer returns the real Atlantis server object. -func (d *DefaultServerCreator) NewServer(config server.Config) (ServerStarter, error) { - return server.NewServer(config) +func (d *DefaultServerCreator) NewServer(config server.Config, flagNames server.FlagNames) (ServerStarter, error) { + return server.NewServer(config, flagNames) } // Init returns the runnable cobra command. @@ -252,7 +258,9 @@ func (s *ServerCmd) run() error { s.trimAtSymbolFromUsers(&config) // Config looks good. Start the server. - server, err := s.ServerCreator.NewServer(config) + server, err := s.ServerCreator.NewServer(config, server.FlagNames{ + AllowForkPRsFlag: AllowForkPRsFlag, + }) if err != nil { return errors.Wrap(err, "initializing server") } diff --git a/cmd/server_test.go b/cmd/server_test.go index ca57ca5b0..bb855d8f4 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -21,7 +21,7 @@ var passedConfig server.Config type ServerCreatorMock struct{} -func (s *ServerCreatorMock) NewServer(config server.Config) (cmd.ServerStarter, error) { +func (s *ServerCreatorMock) NewServer(config server.Config, flagNames server.FlagNames) (cmd.ServerStarter, error) { passedConfig = config return &ServerStarterMock{}, nil } diff --git a/server/events/command_handler.go b/server/events/command_handler.go index 8658cc9a1..091755544 100644 --- a/server/events/command_handler.go +++ b/server/events/command_handler.go @@ -51,9 +51,19 @@ type CommandHandler struct { AtlantisWorkspaceLocker AtlantisWorkspaceLocker MarkdownRenderer *MarkdownRenderer Logger logging.SimpleLogging + // AllowForkPRs controls whether we operate on pull requests from forks. + AllowForkPRs bool + // AllowForkPRsFlag is the name of the flag that controls fork PR's. We use + // this in our error message back to the user on a forked PR so they know + // how to enable this functionality. + AllowForkPRsFlag string } // ExecuteCommand executes the command. +// If vcsHost is GitHub, we don't use headRepo and instead make an API call +// to get the headRepo. This is because the caller is unable to pass in a +// headRepo since there's not enough data available on the initial webhook +// payload. func (c *CommandHandler) ExecuteCommand(baseRepo models.Repo, headRepo models.Repo, user models.User, pullNum int, cmd *Command, vcsHost vcs.Host) { var err error var pull models.PullRequest @@ -122,6 +132,12 @@ func (c *CommandHandler) run(ctx *CommandContext) { ctx.Log = log defer c.logPanics(ctx) + if !c.AllowForkPRs && ctx.HeadRepo.Owner != ctx.BaseRepo.Owner { + ctx.Log.Info("command was run on a fork pull request which is disallowed") + c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, fmt.Sprintf("Atlantis commands can't be run on fork pull requests. To enable, set --%s", c.AllowForkPRsFlag), ctx.VCSHost) // nolint: errcheck + return + } + if ctx.Pull.State != models.Open { ctx.Log.Info("command was run on closed pull request") c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, "Atlantis commands can't be run on closed pull requests", ctx.VCSHost) // nolint: errcheck diff --git a/server/events/command_handler_test.go b/server/events/command_handler_test.go index a583c2802..9d9f0ed82 100644 --- a/server/events/command_handler_test.go +++ b/server/events/command_handler_test.go @@ -54,13 +54,17 @@ func setup(t *testing.T) { MarkdownRenderer: &events.MarkdownRenderer{}, GithubPullGetter: githubGetter, GitlabMergeRequestGetter: gitlabGetter, - Logger: logger, + Logger: logger, + AllowForkPRs: false, + AllowForkPRsFlag: "allow-fork-prs-flag", } } func TestExecuteCommand_LogPanics(t *testing.T) { t.Log("if there is a panic it is commented back on the pull request") setup(t) + ch.AllowForkPRs = true // Lets us get to the panic code. + defer func() { ch.AllowForkPRs = false }() When(ghStatus.Update(fixtures.Repo, fixtures.Pull, vcs.Pending, nil, vcs.Github)).ThenPanic("panic") ch.ExecuteCommand(fixtures.Repo, fixtures.Repo, fixtures.User, 1, nil, vcs.Github) _, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments() @@ -110,6 +114,24 @@ func TestExecuteCommand_GithubPullParseErr(t *testing.T) { Equals(t, "[ERROR] runatlantis/atlantis#1: Extracting required fields from comment data: err\n", logBytes.String()) } +func TestExecuteCommand_ForkPRDisabled(t *testing.T) { + t.Log("if a command is run on a forked pull request and this is disabled atlantis should" + + " comment saying that this is not allowed") + setup(t) + ch.AllowForkPRs = false // by default it's false so don't need to reset + var pull github.PullRequest + modelPull := models.PullRequest{State: models.Open} + When(githubGetter.GetPullRequest(fixtures.Repo, fixtures.Pull.Num)).ThenReturn(&pull, nil) + + headRepo := fixtures.Repo + headRepo.FullName = "forkrepo/atlantis" + headRepo.Owner = "forkrepo" + When(eventParsing.ParseGithubPull(&pull)).ThenReturn(modelPull, headRepo, nil) + + ch.ExecuteCommand(fixtures.Repo, models.Repo{} /* this isn't used */, fixtures.User, fixtures.Pull.Num, nil, vcs.Github) + vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, modelPull.Num, "Atlantis commands can't be run on fork pull requests. To enable, set --"+ch.AllowForkPRsFlag, vcs.Github) +} + func TestExecuteCommand_ClosedPull(t *testing.T) { t.Log("if a command is run on a closed pull request atlantis should" + " comment saying that this is not allowed") @@ -182,3 +204,34 @@ func TestExecuteCommand_FullRun(t *testing.T) { workspaceLocker.VerifyWasCalledOnce().Unlock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num) } } + +func TestExecuteCommand_ForkPREnabled(t *testing.T) { + t.Log("when running a plan on a fork PR, it should succeed") + setup(t) + + // Enable forked PRs. + ch.AllowForkPRs = true + defer func() { ch.AllowForkPRs = false }() // Reset after test. + + var pull github.PullRequest + cmdResponse := events.CommandResponse{} + cmd := events.Command{ + Name: events.Plan, + Workspace: "workspace", + } + When(githubGetter.GetPullRequest(fixtures.Repo, fixtures.Pull.Num)).ThenReturn(&pull, nil) + headRepo := fixtures.Repo + headRepo.FullName = "forkrepo/atlantis" + headRepo.Owner = "forkrepo" + When(eventParsing.ParseGithubPull(&pull)).ThenReturn(fixtures.Pull, headRepo, nil) + When(workspaceLocker.TryLock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num)).ThenReturn(true) + When(planner.Execute(matchers.AnyPtrToEventsCommandContext())).ThenReturn(cmdResponse) + + ch.ExecuteCommand(fixtures.Repo, models.Repo{} /* this isn't used */, fixtures.User, fixtures.Pull.Num, &cmd, vcs.Github) + + ghStatus.VerifyWasCalledOnce().Update(fixtures.Repo, fixtures.Pull, vcs.Pending, &cmd, vcs.Github) + _, response := ghStatus.VerifyWasCalledOnce().UpdateProjectResult(matchers.AnyPtrToEventsCommandContext(), matchers.AnyEventsCommandResponse()).GetCapturedArguments() + Equals(t, cmdResponse, response) + vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()) + workspaceLocker.VerifyWasCalledOnce().Unlock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num) +} diff --git a/server/server.go b/server/server.go index 0ed76c360..a426815cb 100644 --- a/server/server.go +++ b/server/server.go @@ -53,6 +53,7 @@ type Server struct { // The mapstructure tags correspond to flags in cmd/server.go and are used when // the config is parsed from a YAML file. type Config struct { + AllowForkPRs bool `mapstructure:"allow-fork-prs"` AtlantisURL string `mapstructure:"atlantis-url"` DataDir string `mapstructure:"data-dir"` GithubHostname string `mapstructure:"gh-hostname"` @@ -74,6 +75,13 @@ type Config struct { Webhooks []WebhookConfig `mapstructure:"webhooks"` } +// FlagNames contains the names of the flags available to atlantis server. +// They're useful because sometimes we comment back asking the user to enable +// a certain flag. +type FlagNames struct { + AllowForkPRsFlag string +} + // WebhookConfig is nested within Config. It's used to configure webhooks. type WebhookConfig struct { // Event is the type of event we should send this webhook for, ex. apply. @@ -92,7 +100,7 @@ type WebhookConfig struct { // NewServer returns a new server. If there are issues starting the server or // its dependencies an error will be returned. This is like the main() function // for the server CLI command because it injects all the dependencies. -func NewServer(config Config) (*Server, error) { +func NewServer(config Config, flagNames FlagNames) (*Server, error) { var supportedVCSHosts []vcs.Host var githubClient *vcs.GithubClient var gitlabClient *vcs.GitlabClient @@ -214,6 +222,8 @@ func NewServer(config Config) (*Server, error) { AtlantisWorkspaceLocker: workspaceLocker, MarkdownRenderer: markdownRenderer, Logger: logger, + AllowForkPRs: config.AllowForkPRs, + AllowForkPRsFlag: flagNames.AllowForkPRsFlag, } eventsController := &EventsController{ CommandRunner: commandHandler, diff --git a/server/server_test.go b/server/server_test.go index 1ccf79e26..a7d8e5f7a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -26,7 +26,7 @@ func TestNewServer(t *testing.T) { Ok(t, err) _, err = server.NewServer(server.Config{ DataDir: tmpDir, - }) + }, server.FlagNames{}) Ok(t, err) }