Add --allow-fork-prs option. Default to false.

Operating on forked pull requests is dangerous if it's a public repo
because anyone can make a pull request to a public repo. We default to
false like CircleCI and TravisCI who don't allow credentials to be
available on fork PRs.
This commit is contained in:
Luke Kysow
2018-03-06 20:09:55 -08:00
parent 4941456cb1
commit 35d3e14124
8 changed files with 99 additions and 11 deletions

View File

@@ -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<path>.*?\.go),(?P<line>\d+),(?P<message>[^,]+,[^,]+,[^,]+)' --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<path>.*?\.go),(?P<line>\d+),(?P<message>[^,]+,[^,]+,[^,]+)' --deadline=300s --vendor -t --line-length=120 --skip server/static ./...
gometalint-install: ## Install gometalint
go get -u github.com/alecthomas/gometalinter

View File

@@ -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

View File

@@ -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")
}

View File

@@ -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
}

View File

@@ -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

View File

@@ -55,12 +55,16 @@ func setup(t *testing.T) {
GithubPullGetter: githubGetter,
GitlabMergeRequestGetter: gitlabGetter,
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)
}

View File

@@ -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,

View File

@@ -26,7 +26,7 @@ func TestNewServer(t *testing.T) {
Ok(t, err)
_, err = server.NewServer(server.Config{
DataDir: tmpDir,
})
}, server.FlagNames{})
Ok(t, err)
}