From 980f905a71bf4c825e2bfb451f9f030a2f74a12f Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Sun, 10 Nov 2024 00:12:33 +0200 Subject: [PATCH 1/8] [github] Build and unit tests pull requests --- .github/workflows/pull_request.yaml | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 .github/workflows/pull_request.yaml diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml new file mode 100644 index 0000000..35a8f9a --- /dev/null +++ b/.github/workflows/pull_request.yaml @@ -0,0 +1,33 @@ +name: Pull Request Test + +on: + pull_request: + branches: + - main + +permissions: + contents: read + +jobs: + test: + name: Run Terraform Validate and Plan on PR + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: "1.23" + cache: false + + - name: Install Dependencies + run: go mod tidy + + - name: Build source + run: go build main.go + + - name: Unit tests + run: go test -v ./... From ee6f1dc930f6785f21adceb56393bb09bcba0183 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 21:07:45 +0200 Subject: [PATCH 2/8] [storage] Use safer database queries --- storage/storage.go | 51 ++++++++++++++++++++++++++--------------- storage/storage_test.go | 33 +++++++++++++++++--------- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index f98b64e..75def80 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -217,39 +217,46 @@ func (sq *Sqlite3) InsertSplit(ctx context.Context, records []SplitRecord) error return telemetry.Error(span, tx.Commit()) } -func sqlQuery(tables []string, fields []string, cond conditions, order *Order) string { +func sqlQuery(tables []string, fields []string, cond conditions, order *Order) (string, []interface{}) { where := []string{} + args := []string{} if len(tables) > 0 { for _, table := range tables[1:] { where = append(where, fmt.Sprintf("%s.StravaID=%s.StravaID", tables[0], table)) } } if len(cond.WorkoutTypes) > 0 { - where = append(where, "(workouttype='"+strings.Join(cond.WorkoutTypes, "' or workouttype='")+"')") + where = append(where, "(workouttype="+strings.Repeat("? or workouttype=", len(cond.WorkoutTypes)-1)+"?)") + args = append(args, cond.WorkoutTypes...) } if len(cond.Types) > 0 { - where = append(where, "(type='"+strings.Join(cond.Types, "' or type='")+"')") + where = append(where, "(type="+strings.Repeat("? or type=", len(cond.Types)-1)+"?)") + args = append(args, cond.Types...) } if cond.Month > 0 && cond.Day > 0 { - where = append(where, fmt.Sprintf("(month < %d or (month=%d and day<=%d))", cond.Month, cond.Month, cond.Day)) + where = append(where, "(month < ? or (month=? and day<=?))") + month := strconv.Itoa(cond.Month) + args = append(args, month, month, strconv.Itoa(cond.Day)) } if len(cond.Years) > 0 { - yearStr := []string{} + where = append(where, "(year="+strings.Repeat("? or year=", len(cond.Years)-1)+"?)") for _, y := range cond.Years { - yearStr = append(yearStr, strconv.Itoa(y)) + args = append(args, strconv.Itoa(y)) } - where = append(where, "(year="+strings.Join(yearStr, " or year=")+")") } if cond.BEName != "" { - where = append(where, "besteffort.name='"+cond.BEName+"'") + where = append(where, "besteffort.name=?") + args = append(args, cond.BEName) } if cond.StravaID > 0 { for _, t := range tables { - where = append(where, fmt.Sprintf("%s.stravaid=%d", t, cond.StravaID)) + where = append(where, t+".stravaid=") + args = append(args, strconv.FormatInt(cond.StravaID, 10)) } } if cond.Name != "" { - where = append(where, fmt.Sprintf("summary.name LIKE '%s'", cond.Name)) + where = append(where, "summary.name LIKE ?") + args = append(args, cond.Name) } condition := "" if len(where) > 0 { @@ -267,19 +274,23 @@ func sqlQuery(tables []string, fields []string, cond conditions, order *Order) s sorting += " limit " + strconv.FormatInt(int64(order.Limit), 10) } } + ifArgs := make([]interface{}, len(args)) + for i, v := range args { + ifArgs[i] = v + } return fmt.Sprintf( "select %s from %s%s%s", strings.Join(fields, ","), strings.Join(tables, ","), condition, sorting, - ) + ), ifArgs } func (sq *Sqlite3) QueryBestEffort(fields []string, name string, order *Order) (*sql.Rows, error) { if sq.db == nil { return nil, errors.New("database is nil") } - query := sqlQuery([]string{"besteffort", "summary"}, fields, conditions{BEName: name}, order) + query, values := sqlQuery([]string{"besteffort", "summary"}, fields, conditions{BEName: name}, order) // slog.Info("storage.Query", "query", query) - rows, err := sq.db.Query(query) + rows, err := sq.db.Query(query, values...) if err != nil { return nil, fmt.Errorf("%s failed: %w", query, err) } @@ -290,12 +301,12 @@ func (sq *Sqlite3) QueryBestEffortDistances() ([]string, error) { if sq.db == nil { return nil, errors.New("database is nil") } - query := sqlQuery( + query, values := sqlQuery( []string{"besteffort"}, []string{"distinct(name)"}, conditions{}, &Order{OrderBy: []string{"distance desc"}}, ) // slog.Info("storage.Query", "query", query) - rows, err := sq.db.Query(query) + rows, err := sq.db.Query(query, values...) if err != nil { return nil, fmt.Errorf("%s failed: %w", query, err) } @@ -316,9 +327,11 @@ func (sq *Sqlite3) QuerySplit(fields []string, id int64) (*sql.Rows, error) { if sq.db == nil { return nil, errors.New("database is nil") } - query := sqlQuery([]string{"split"}, fields, conditions{StravaID: id}, &Order{OrderBy: []string{"split"}}) + query, values := sqlQuery( + []string{"split"}, fields, conditions{StravaID: id}, &Order{OrderBy: []string{"split"}}, + ) // slog.Info("storage.Query", "query", query) - rows, err := sq.db.Query(query) + rows, err := sq.db.Query(query, values...) if err != nil { return nil, fmt.Errorf("%s failed: %w", query, err) } @@ -329,7 +342,7 @@ func (sq *Sqlite3) QuerySummary(fields []string, cond SummaryConditions, order * if sq.db == nil { return nil, errors.New("database is nil") } - query := sqlQuery( + query, values := sqlQuery( []string{"summary"}, fields, conditions{ Types: cond.Types, WorkoutTypes: cond.WorkoutTypes, @@ -339,7 +352,7 @@ func (sq *Sqlite3) QuerySummary(fields []string, cond SummaryConditions, order * order, ) // slog.Info("storage.Query", "query", query) - rows, err := sq.db.Query(query) + rows, err := sq.db.Query(query, values...) if err != nil { return nil, fmt.Errorf("%s failed: %w", query, err) } diff --git a/storage/storage_test.go b/storage/storage_test.go index 2f5a048..7674b8f 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1,6 +1,10 @@ package storage -import "testing" +import ( + "fmt" + "strings" + "testing" +) func TestSqlQuery(t *testing.T) { values := []struct { @@ -10,53 +14,60 @@ func TestSqlQuery(t *testing.T) { cond conditions order *Order query string + values []string }{ { "none", []string{"summary"}, []string{"field"}, conditions{}, nil, - "select field from summary"}, + "select field from summary", []string{}, + }, { "simple", []string{"summary"}, []string{"field"}, conditions{Types: []string{"Run"}}, nil, - "select field from summary where (type='Run')", + "select field from summary where (type=?)", []string{"Run"}, }, { "multi-field", []string{"summary"}, []string{"f1", "f2"}, conditions{Types: []string{"r1", "r2"}}, &Order{GroupBy: []string{"f3"}, OrderBy: []string{"f3 desc"}}, - "select f1,f2 from summary where (type='r1' or type='r2') group by f3 order by f3 desc", + "select f1,f2 from summary where (type=? or type=?) group by f3 order by f3 desc", []string{"r1", "r2"}, }, { "order", []string{"summary"}, []string{"k1", "k2"}, conditions{Types: []string{"c1"}, WorkoutTypes: []string{"c3"}}, &Order{GroupBy: []string{"k3", "k4"}, OrderBy: []string{"k5", "k6"}, Limit: 7}, - "select k1,k2 from summary where (workouttype='c3') and (type='c1') group by k3,k4 order by k5,k6 limit 7", + "select k1,k2 from summary where (workouttype=?) and (type=?) group by k3,k4 order by k5,k6 limit 7", + []string{"c3", "c1"}, }, { "one_year", []string{"summary"}, []string{"field"}, conditions{Types: []string{"Run"}, Years: []int{2023}}, nil, - "select field from summary where (type='Run') and (year=2023)", + "select field from summary where (type=?) and (year=?)", []string{"Run", "2023"}, }, { "multiple_years", []string{"summary"}, []string{"field"}, conditions{Types: []string{"Run"}, Years: []int{2019, 2023}}, nil, - "select field from summary where (type='Run') and (year=2019 or year=2023)", + "select field from summary where (type=?) and (year=? or year=?)", []string{"Run", "2019", "2023"}, }, { "ids", []string{"summary"}, []string{"StravaID"}, conditions{Types: []string{"Run"}}, &Order{OrderBy: []string{"StravaID desc"}}, - "select StravaID from summary where (type='Run') order by StravaID desc", + "select StravaID from summary where (type=?) order by StravaID desc", []string{"Run"}, }, { "besteffort", []string{"summary", "besteffort"}, []string{"summary.Name"}, conditions{BEName: "400m"}, nil, - "select summary.Name from summary,besteffort where summary.StravaID=besteffort.StravaID and besteffort.name='400m'", + "select summary.Name from summary,besteffort where summary.StravaID=besteffort.StravaID and besteffort.name=?", + []string{"400m"}, }, } for _, value := range values { t.Run(value.name, func(t *testing.T) { - cmd := sqlQuery(value.tables, value.fields, value.cond, value.order) + cmd, values := sqlQuery(value.tables, value.fields, value.cond, value.order) if cmd != value.query { - t.Errorf("mismatch got '%s' vs. expected '%s'", cmd, value.query) + t.Errorf("query mismatch got '%s' vs. expected '%s'", cmd, value.query) + } + if fmt.Sprintf("%v", values) != "["+strings.Join(value.values, " ")+"]" { + t.Errorf("values mismatch got '%s' vs. expected '%s'", fmt.Sprintf("%v", values), strings.Join(value.values, " ")) } }) } From 9f8baa3e705fd2a36ddcbda2e3a9e40de640c353 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 21:10:47 +0200 Subject: [PATCH 3/8] [github] Fix job name --- .github/workflows/pull_request.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 35a8f9a..a2464fd 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -10,7 +10,7 @@ permissions: jobs: test: - name: Run Terraform Validate and Plan on PR + name: Test compile golang and run go tests runs-on: ubuntu-latest steps: From a20d02b8da2e702d4bdc4b57cc6293c87a9d2707 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 22:15:52 +0200 Subject: [PATCH 4/8] [server] Fix tests --- pkg/telemetry/telemetry.go | 5 +++++ server/plot.go | 13 ++++++++++++- server/server_test.go | 4 +++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/pkg/telemetry/telemetry.go b/pkg/telemetry/telemetry.go index 5be4847..c078f85 100644 --- a/pkg/telemetry/telemetry.go +++ b/pkg/telemetry/telemetry.go @@ -2,6 +2,7 @@ package telemetry import ( "context" + "log" "os" "path/filepath" "strings" @@ -86,6 +87,10 @@ func Setup(ctx context.Context, name string) (context.Context, *sdktrace.TracerP } func NewSpan(ctx context.Context, name string) (context.Context, trace.Span) { + value := ctx.Value(otelCtxKey) + if value == nil { + log.Fatal("Telemetry has not been setup") + } return ctx.Value(otelCtxKey).(trace.Tracer).Start(ctx, name) } diff --git a/server/plot.go b/server/plot.go index d8b0979..befffa0 100644 --- a/server/plot.go +++ b/server/plot.go @@ -123,6 +123,10 @@ func (p *PlotPage) render( foundYears = append(foundYears, year) } } + if len(foundYears) == 0 { + slog.Error("No years found in plot.render()") + return nil + } refTime, err := time.Parse(time.DateOnly, fmt.Sprintf("%d-01-01", slices.Max(foundYears))) if err != nil { return err @@ -193,6 +197,9 @@ func scan(rows *sql.Rows, years []int) (numbers, error) { ys[year] = []float64{} } xmax := 0 + if rows == nil { + return ys, nil + } for rows.Next() { var year, month, day int var value float64 @@ -251,6 +258,10 @@ func getNumbers( if err != nil { return nil, fmt.Errorf("select caused: %w", err) } - defer rows.Close() + defer func() { + if rows != nil { + rows.Close() + } + }() return scan(rows, years) } diff --git a/server/server_test.go b/server/server_test.go index e5e5060..b810117 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/jylitalo/mystats/pkg/stats" + "github.com/jylitalo/mystats/pkg/telemetry" "github.com/jylitalo/mystats/storage" ) @@ -49,7 +50,8 @@ func TestTemplateRender(t *testing.T) { ) { return nil, nil, nil, nil } - err := p.Plot.render(context.TODO(), &testDB{}, map[string]bool{"Run": true}, nil, 6, 12, map[int]bool{2024: true}, "month") + ctx, _, _ := telemetry.Setup(context.TODO(), "test") + err := p.Plot.render(ctx, &testDB{}, map[string]bool{"Run": true}, nil, 6, 12, map[int]bool{2024: true}, "month") if err != nil { t.Error(err) } From cba86aa2b21413ce1ff58ec539329db5363d7922 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 22:24:52 +0200 Subject: [PATCH 5/8] [server plot] Limit measurement options to hard-coded list --- server/plot.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/plot.go b/server/plot.go index befffa0..0914ad7 100644 --- a/server/plot.go +++ b/server/plot.go @@ -246,12 +246,14 @@ func getNumbers( if err != nil { return nil, err } - m := "sum(" + measure + ")" + var m string switch measure { case "time": m = "sum(elapsedtime)/3600" case "distance": m = "sum(distance)/1000" + case "elevation": + m = "sum(elevation)" } o := []string{"year", "month", "day"} rows, err := db.QuerySummary(append(o, m), cond, &storage.Order{GroupBy: o, OrderBy: o}) From d5cc6ee7531e38def14e6d639f5632121612a4a6 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 22:34:16 +0200 Subject: [PATCH 6/8] [top] Fix SQL injection from top --- pkg/stats/top.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/pkg/stats/top.go b/pkg/stats/top.go index 8e8f0a4..0d5c0ac 100644 --- a/pkg/stats/top.go +++ b/pkg/stats/top.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "strconv" - "strings" "time" "github.com/jylitalo/mystats/pkg/telemetry" @@ -15,22 +14,21 @@ func Top(ctx context.Context, db Storage, measure, period string, types, workout _, span := telemetry.NewSpan(ctx, "stats.Top") defer span.End() - modifier := float64(1) - unit := "%4.0fm" - switch { - case strings.Contains(measure, "distance") && !strings.Contains(measure, "count"): - modifier = 1000 - unit = "%4.1fkm" - case strings.Contains(measure, "time") && !strings.Contains(measure, "count"): - modifier = 3600 + var m, unit string + switch measure { + case "time": + m = "sum(elapsedtime)/3600" unit = "%4.1fh" - } - if measure == "time" { - measure = "elapsedtime" + case "distance": + m = "sum(distance)/1000" + unit = "%4.1fkm" + case "elevation": + m = "sum(elevation)" + unit = "%4.0fm" } results := [][]string{} rows, err := db.QuerySummary( - []string{"sum(" + measure + ") as total", "year", period}, + []string{m + " as total", "year", period}, storage.SummaryConditions{Types: types, WorkoutTypes: workoutTypes, Years: years}, &storage.Order{ GroupBy: []string{"year", period}, From 153dad2a81dfdde9f78f4c4c5f485526eb395ab5 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 22:35:49 +0200 Subject: [PATCH 7/8] [top] Removed unnecessary variable --- pkg/stats/top.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/stats/top.go b/pkg/stats/top.go index 0d5c0ac..c23af85 100644 --- a/pkg/stats/top.go +++ b/pkg/stats/top.go @@ -45,7 +45,7 @@ func Top(ctx context.Context, db Storage, measure, period string, types, workout if err = rows.Scan(&measureValue, &year, &periodValue); err != nil { return nil, nil, telemetry.Error(span, err) } - value := fmt.Sprintf(unit, measureValue/modifier) + value := fmt.Sprintf(unit, measureValue) periodStr := strconv.FormatInt(int64(periodValue), 10) if period == "month" { periodStr = time.Month(periodValue).String() From 39df0437aa77980949dae35e96c85fb47b86cd47 Mon Sep 17 00:00:00 2001 From: Juha Ylitalo Date: Tue, 12 Nov 2024 22:43:37 +0200 Subject: [PATCH 8/8] [top] Fix SQL injection with period from top --- pkg/stats/top.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/stats/top.go b/pkg/stats/top.go index c23af85..bcaae77 100644 --- a/pkg/stats/top.go +++ b/pkg/stats/top.go @@ -14,7 +14,7 @@ func Top(ctx context.Context, db Storage, measure, period string, types, workout _, span := telemetry.NewSpan(ctx, "stats.Top") defer span.End() - var m, unit string + var m, unit, p string switch measure { case "time": m = "sum(elapsedtime)/3600" @@ -26,13 +26,21 @@ func Top(ctx context.Context, db Storage, measure, period string, types, workout m = "sum(elevation)" unit = "%4.0fm" } + switch period { + case "month": + p = "month" + case "week": + p = "week" + case "day": + p = "day" + } results := [][]string{} rows, err := db.QuerySummary( - []string{m + " as total", "year", period}, + []string{m + " as total", "year", p}, storage.SummaryConditions{Types: types, WorkoutTypes: workoutTypes, Years: years}, &storage.Order{ - GroupBy: []string{"year", period}, - OrderBy: []string{"total desc", "year desc", period + " desc"}, + GroupBy: []string{"year", p}, + OrderBy: []string{"total desc", "year desc", p + " desc"}, Limit: limit}, ) if err != nil { @@ -47,12 +55,12 @@ func Top(ctx context.Context, db Storage, measure, period string, types, workout } value := fmt.Sprintf(unit, measureValue) periodStr := strconv.FormatInt(int64(periodValue), 10) - if period == "month" { + if p == "month" { periodStr = time.Month(periodValue).String() } results = append( results, []string{value, strconv.FormatInt(int64(year), 10), periodStr}, ) } - return []string{measure, "year", period}, results, nil + return []string{measure, "year", p}, results, nil }