Skip to content

Commit 8cf292f

Browse files
EmyrkEmyrk
authored
feat: Guard search queries against common mistakes (coder#6404)
* feat: Error on excessive invalid search keys * feat: Guard search queries against common mistakes * Raise errors in FE on workspaces table * All errors should be on newlines
1 parent 1724cbf commit 8cf292f

19 files changed

Lines changed: 804 additions & 634 deletions

File tree

‎coderd/audit.go‎

Lines changed: 2 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ import (
88
"net"
99
"net/http"
1010
"net/netip"
11-
"net/url"
12-
"strings"
1311
"time"
1412

1513
"github.com/google/uuid"
@@ -22,6 +20,7 @@ import (
2220
"github.com/coder/coder/coderd/httpapi"
2321
"github.com/coder/coder/coderd/httpmw"
2422
"github.com/coder/coder/coderd/rbac"
23+
"github.com/coder/coder/coderd/searchquery"
2524
"github.com/coder/coder/codersdk"
2625
)
2726

@@ -49,7 +48,7 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) {
4948
}
5049

5150
queryStr := r.URL.Query().Get("q")
52-
filter, errs := auditSearchQuery(queryStr)
51+
filter, errs := searchquery.AuditLogs(queryStr)
5352
if len(errs) > 0 {
5453
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
5554
Message: "Invalid audit search query.",
@@ -373,125 +372,3 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit
373372
return ""
374373
}
375374
}
376-
377-
// auditSearchQuery takes a query string and returns the auditLog filter.
378-
// It also can return the list of validation errors to return to the api.
379-
func auditSearchQuery(query string) (database.GetAuditLogsOffsetParams, []codersdk.ValidationError) {
380-
searchParams := make(url.Values)
381-
if query == "" {
382-
// No filter
383-
return database.GetAuditLogsOffsetParams{}, nil
384-
}
385-
query = strings.ToLower(query)
386-
// Because we do this in 2 passes, we want to maintain quotes on the first
387-
// pass.Further splitting occurs on the second pass and quotes will be
388-
// dropped.
389-
elements := splitQueryParameterByDelimiter(query, ' ', true)
390-
for _, element := range elements {
391-
parts := splitQueryParameterByDelimiter(element, ':', false)
392-
switch len(parts) {
393-
case 1:
394-
// No key:value pair.
395-
searchParams.Set("resource_type", parts[0])
396-
case 2:
397-
searchParams.Set(parts[0], parts[1])
398-
default:
399-
return database.GetAuditLogsOffsetParams{}, []codersdk.ValidationError{
400-
{Field: "q", Detail: fmt.Sprintf("Query element %q can only contain 1 ':'", element)},
401-
}
402-
}
403-
}
404-
405-
// Using the query param parser here just returns consistent errors with
406-
// other parsing.
407-
parser := httpapi.NewQueryParamParser()
408-
const layout = "2006-01-02"
409-
410-
var (
411-
dateFromString = parser.String(searchParams, "", "date_from")
412-
dateToString = parser.String(searchParams, "", "date_to")
413-
parsedDateFrom, _ = time.Parse(layout, dateFromString)
414-
parsedDateTo, _ = time.Parse(layout, dateToString)
415-
)
416-
417-
if dateToString != "" {
418-
parsedDateTo = parsedDateTo.Add(23*time.Hour + 59*time.Minute + 59*time.Second) // parsedDateTo goes to 23:59
419-
}
420-
421-
if dateToString != "" && parsedDateTo.Before(parsedDateFrom) {
422-
return database.GetAuditLogsOffsetParams{}, []codersdk.ValidationError{
423-
{Field: "q", Detail: fmt.Sprintf("DateTo value %q cannot be before than DateFrom", parsedDateTo)},
424-
}
425-
}
426-
427-
filter := database.GetAuditLogsOffsetParams{
428-
ResourceType: resourceTypeFromString(parser.String(searchParams, "", "resource_type")),
429-
ResourceID: parser.UUID(searchParams, uuid.Nil, "resource_id"),
430-
Action: actionFromString(parser.String(searchParams, "", "action")),
431-
Username: parser.String(searchParams, "", "username"),
432-
Email: parser.String(searchParams, "", "email"),
433-
DateFrom: parsedDateFrom,
434-
DateTo: parsedDateTo,
435-
BuildReason: buildReasonFromString(parser.String(searchParams, "", "build_reason")),
436-
}
437-
438-
return filter, parser.Errors
439-
}
440-
441-
func resourceTypeFromString(resourceTypeString string) string {
442-
switch codersdk.ResourceType(resourceTypeString) {
443-
case codersdk.ResourceTypeTemplate:
444-
return resourceTypeString
445-
case codersdk.ResourceTypeTemplateVersion:
446-
return resourceTypeString
447-
case codersdk.ResourceTypeUser:
448-
return resourceTypeString
449-
case codersdk.ResourceTypeWorkspace:
450-
return resourceTypeString
451-
case codersdk.ResourceTypeWorkspaceBuild:
452-
return resourceTypeString
453-
case codersdk.ResourceTypeGitSSHKey:
454-
return resourceTypeString
455-
case codersdk.ResourceTypeAPIKey:
456-
return resourceTypeString
457-
case codersdk.ResourceTypeGroup:
458-
return resourceTypeString
459-
case codersdk.ResourceTypeLicense:
460-
return resourceTypeString
461-
}
462-
return ""
463-
}
464-
465-
func actionFromString(actionString string) string {
466-
switch codersdk.AuditAction(actionString) {
467-
case codersdk.AuditActionCreate:
468-
return actionString
469-
case codersdk.AuditActionWrite:
470-
return actionString
471-
case codersdk.AuditActionDelete:
472-
return actionString
473-
case codersdk.AuditActionStart:
474-
return actionString
475-
case codersdk.AuditActionStop:
476-
return actionString
477-
case codersdk.AuditActionLogin:
478-
return actionString
479-
case codersdk.AuditActionLogout:
480-
return actionString
481-
default:
482-
}
483-
return ""
484-
}
485-
486-
func buildReasonFromString(buildReasonString string) string {
487-
switch codersdk.BuildReason(buildReasonString) {
488-
case codersdk.BuildReasonInitiator:
489-
return buildReasonString
490-
case codersdk.BuildReasonAutostart:
491-
return buildReasonString
492-
case codersdk.BuildReasonAutostop:
493-
return buildReasonString
494-
default:
495-
}
496-
return ""
497-
}

‎coderd/audit_test.go‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ func TestAuditLogsFilter(t *testing.T) {
151151
Name string
152152
SearchQuery string
153153
ExpectedResult int
154+
ExpectedError bool
154155
}{
155156
{
156157
Name: "FilterByCreateAction",
@@ -188,19 +189,19 @@ func TestAuditLogsFilter(t *testing.T) {
188189
ExpectedResult: 2,
189190
},
190191
{
191-
Name: "FilterInvalidSingleValue",
192-
SearchQuery: "invalid",
193-
ExpectedResult: 5,
192+
Name: "FilterInvalidSingleValue",
193+
SearchQuery: "invalid",
194+
ExpectedError: true,
194195
},
195196
{
196-
Name: "FilterWithInvalidResourceType",
197-
SearchQuery: "resource_type:invalid",
198-
ExpectedResult: 5,
197+
Name: "FilterWithInvalidResourceType",
198+
SearchQuery: "resource_type:invalid",
199+
ExpectedError: true,
199200
},
200201
{
201-
Name: "FilterWithInvalidAction",
202-
SearchQuery: "action:invalid",
203-
ExpectedResult: 5,
202+
Name: "FilterWithInvalidAction",
203+
SearchQuery: "action:invalid",
204+
ExpectedError: true,
204205
},
205206
{
206207
Name: "FilterOnCreateSingleDay",
@@ -229,7 +230,7 @@ func TestAuditLogsFilter(t *testing.T) {
229230
},
230231
{
231232
Name: "FilterOnWorkspaceBuildStartByInitiator",
232-
SearchQuery: "resource_type:workspace_build action:start build_reason:start",
233+
SearchQuery: "resource_type:workspace_build action:start build_reason:initiator",
233234
ExpectedResult: 1,
234235
},
235236
}
@@ -245,9 +246,13 @@ func TestAuditLogsFilter(t *testing.T) {
245246
Limit: 25,
246247
},
247248
})
248-
require.NoError(t, err, "fetch audit logs")
249-
require.Len(t, auditLogs.AuditLogs, testCase.ExpectedResult, "expected audit logs returned")
250-
require.Equal(t, testCase.ExpectedResult, int(auditLogs.Count), "expected audit log count returned")
249+
if testCase.ExpectedError {
250+
require.Error(t, err, "expected error")
251+
} else {
252+
require.NoError(t, err, "fetch audit logs")
253+
require.Len(t, auditLogs.AuditLogs, testCase.ExpectedResult, "expected audit logs returned")
254+
require.Equal(t, testCase.ExpectedResult, int(auditLogs.Count), "expected audit log count returned")
255+
}
251256
})
252257
}
253258
})

‎coderd/database/dbfake/databasefake.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -966,13 +966,13 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
966966
return nil, xerrors.Errorf("get provisioner job: %w", err)
967967
}
968968

969-
switch arg.Status {
970-
case "pending":
969+
switch database.WorkspaceStatus(arg.Status) {
970+
case database.WorkspaceStatusPending:
971971
if !job.StartedAt.Valid {
972972
continue
973973
}
974974

975-
case "starting":
975+
case database.WorkspaceStatusStarting:
976976
if !job.StartedAt.Valid &&
977977
!job.CanceledAt.Valid &&
978978
job.CompletedAt.Valid &&
@@ -981,15 +981,15 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
981981
continue
982982
}
983983

984-
case "running":
984+
case database.WorkspaceStatusRunning:
985985
if !job.CompletedAt.Valid &&
986986
job.CanceledAt.Valid &&
987987
job.Error.Valid ||
988988
build.Transition != database.WorkspaceTransitionStart {
989989
continue
990990
}
991991

992-
case "stopping":
992+
case database.WorkspaceStatusStopping:
993993
if !job.StartedAt.Valid &&
994994
!job.CanceledAt.Valid &&
995995
job.CompletedAt.Valid &&
@@ -998,31 +998,31 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
998998
continue
999999
}
10001000

1001-
case "stopped":
1001+
case database.WorkspaceStatusStopped:
10021002
if !job.CompletedAt.Valid &&
10031003
job.CanceledAt.Valid &&
10041004
job.Error.Valid ||
10051005
build.Transition != database.WorkspaceTransitionStop {
10061006
continue
10071007
}
10081008

1009-
case "failed":
1009+
case database.WorkspaceStatusFailed:
10101010
if (!job.CanceledAt.Valid && !job.Error.Valid) ||
10111011
(!job.CompletedAt.Valid && !job.Error.Valid) {
10121012
continue
10131013
}
10141014

1015-
case "canceling":
1015+
case database.WorkspaceStatusCanceling:
10161016
if !job.CanceledAt.Valid && job.CompletedAt.Valid {
10171017
continue
10181018
}
10191019

1020-
case "canceled":
1020+
case database.WorkspaceStatusCanceled:
10211021
if !job.CanceledAt.Valid && !job.CompletedAt.Valid {
10221022
continue
10231023
}
10241024

1025-
case "deleted":
1025+
case database.WorkspaceStatusDeleted:
10261026
if !job.StartedAt.Valid &&
10271027
job.CanceledAt.Valid &&
10281028
!job.CompletedAt.Valid &&
@@ -1031,7 +1031,7 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
10311031
continue
10321032
}
10331033

1034-
case "deleting":
1034+
case database.WorkspaceStatusDeleting:
10351035
if !job.CompletedAt.Valid &&
10361036
job.CanceledAt.Valid &&
10371037
job.Error.Valid &&

‎coderd/database/modelmethods.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,33 @@ import (
77
"github.com/coder/coder/coderd/rbac"
88
)
99

10+
type WorkspaceStatus string
11+
12+
const (
13+
WorkspaceStatusPending WorkspaceStatus = "pending"
14+
WorkspaceStatusStarting WorkspaceStatus = "starting"
15+
WorkspaceStatusRunning WorkspaceStatus = "running"
16+
WorkspaceStatusStopping WorkspaceStatus = "stopping"
17+
WorkspaceStatusStopped WorkspaceStatus = "stopped"
18+
WorkspaceStatusFailed WorkspaceStatus = "failed"
19+
WorkspaceStatusCanceling WorkspaceStatus = "canceling"
20+
WorkspaceStatusCanceled WorkspaceStatus = "canceled"
21+
WorkspaceStatusDeleting WorkspaceStatus = "deleting"
22+
WorkspaceStatusDeleted WorkspaceStatus = "deleted"
23+
)
24+
25+
func (s WorkspaceStatus) Valid() bool {
26+
switch s {
27+
case WorkspaceStatusPending, WorkspaceStatusStarting, WorkspaceStatusRunning,
28+
WorkspaceStatusStopping, WorkspaceStatusStopped, WorkspaceStatusFailed,
29+
WorkspaceStatusCanceling, WorkspaceStatusCanceled, WorkspaceStatusDeleting,
30+
WorkspaceStatusDeleted:
31+
return true
32+
default:
33+
return false
34+
}
35+
}
36+
1037
type AuditableGroup struct {
1138
Group
1239
Members []GroupMember `json:"members"`

0 commit comments

Comments
 (0)