Skip to content

Commit

Permalink
fix: make turbo command regex stricter (#4325)
Browse files Browse the repository at this point in the history
### Description

With #4223, we started checking and throwing on commands that look like
they invoke turbo. The regex is fairly permissive and would match any
command that contained a path that ended in `turbo` e.g. `rm ~/.turbo`
would get marked as invoking turbo.

This PR restricts the regex so it only matches commands that contain
`turbo` surrounded by whitespace. This means that we now won't throw if
the user directly invokes turbo via a path e.g.
`./node_modules/turbo/bin/turbo`. I think this is the correct tradeoff
as it's much more likely a user will invoke turbo via either `turbo foo`
or `pnpm turbo foo`.

Another possibility would be to attempt parse the scripts and do more
exact analysis, but to do this and correctly handle Windows would be
*fun*.

### Testing Instructions

See new unit tests
  • Loading branch information
chris-olszewski authored Mar 23, 2023
1 parent 3bd81e3 commit 5b32049
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 1 deletion.
2 changes: 1 addition & 1 deletion cli/internal/graph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func (g *CompleteGraph) getTaskGraphDescendants(taskGraph *dag.AcyclicGraph, tas
return stringDescendents, nil
}

var _isTurbo = regexp.MustCompile(fmt.Sprintf("(?:^|%v|\\s)turbo(?:$|\\s)", regexp.QuoteMeta(string(filepath.Separator))))
var _isTurbo = regexp.MustCompile(`(?:^|\s)turbo(?:$|\s)`)

func commandLooksLikeTurbo(command string) bool {
return _isTurbo.MatchString(command)
Expand Down
50 changes: 50 additions & 0 deletions cli/internal/graph/graph_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package graph

import (
"testing"

"gotest.tools/v3/assert"
)

func Test_CommandsInvokingTurbo(t *testing.T) {
type testCase struct {
command string
match bool
}
testCases := []testCase{
{
"turbo run foo",
true,
},
{
"rm -rf ~/Library/Caches/pnpm && turbo run foo && rm -rf ~/.npm",
true,
},
{
"FLAG=true turbo run foo",
true,
},
{
"npx turbo run foo",
true,
},
{
"echo starting; turbo foo; echo done",
true,
},
// We don't catch this as if people are going to try to invoke the turbo
// binary directly, they'll always be able to work around us.
{
"./node_modules/.bin/turbo foo",
false,
},
{
"rm -rf ~/Library/Caches/pnpm && rm -rf ~/Library/Caches/turbo && rm -rf ~/.npm && rm -rf ~/.pnpm-store && rm -rf ~/.turbo",
false,
},
}

for _, tc := range testCases {
assert.Equal(t, commandLooksLikeTurbo(tc.command), tc.match, tc.command)
}
}

0 comments on commit 5b32049

Please sign in to comment.