Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,7 @@ NETRONOME__IPERF_PING_TIMEOUT=10 # Ping timeout (seconds)

# LibreSpeed settings
NETRONOME__LIBRESPEED_TIMEOUT=60 # LibreSpeed timeout (seconds)
NETRONOME__LIBRESPEED_SHARE_RESULTS=false # Submit results to librespeed.org for a shareable link (sends telemetry)
```

### Pagination
Expand Down
2 changes: 2 additions & 0 deletions config/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ timeout = 10

[speedtest.librespeed]
timeout = 60
# Submit results to librespeed.org for a shareable link (sends telemetry)
share_results = false

[geoip]
country_database_path = "./GeoLite2-Country.mmdb"
Expand Down
16 changes: 14 additions & 2 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,9 @@ type IperfConfig struct {
}

type LibrespeedConfig struct {
ServersPath string `toml:"-"`
Timeout int `toml:"timeout" env:"LIBRESPEED_TIMEOUT"`
ServersPath string `toml:"-"`
Timeout int `toml:"timeout" env:"LIBRESPEED_TIMEOUT"`
ShareResults bool `toml:"share_results" env:"LIBRESPEED_SHARE_RESULTS"`
}

type PingConfig struct {
Expand Down Expand Up @@ -540,6 +541,11 @@ func (c *Config) loadSpeedTestFromEnv() {
c.SpeedTest.Librespeed.Timeout = val
}
}
if v := getEnv("LIBRESPEED_SHARE_RESULTS"); v != "" {
if val, err := strconv.ParseBool(v); err == nil {
c.SpeedTest.Librespeed.ShareResults = val
}
}
}

func (c *Config) loadPaginationFromEnv() {
Expand Down Expand Up @@ -827,6 +833,12 @@ func (c *Config) WriteToml(w io.Writer) error {
if _, err := fmt.Fprintf(w, "timeout = %d\n", cfg.SpeedTest.Librespeed.Timeout); err != nil {
return err
}
if _, err := fmt.Fprintf(w, "# Submit results to librespeed.org for a shareable link (sends telemetry)\n"); err != nil {
return err
}
if _, err := fmt.Fprintf(w, "share_results = %v\n", cfg.SpeedTest.Librespeed.ShareResults); err != nil {
return err
}
if _, err := fmt.Fprintln(w, ""); err != nil {
return err
}
Expand Down
43 changes: 43 additions & 0 deletions internal/database/migrations/migrations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (c) 2024-2026, s0up and the autobrr contributors.
// SPDX-License-Identifier: GPL-2.0-or-later

package migrations

import (
"path"
"strconv"
"strings"
"testing"
)

// TestNoDuplicateMigrationVersions guards against two migration files sharing a
// numeric version prefix. The migrator (pkg/migrator) applies migrations by
// position relative to the count already applied, so a duplicate version makes
// the sorted order ambiguous and silently skips a migration on upgrade (the new
// file gets ordered before an already-applied one and is never run).
//
// The version is parsed from the file basename here rather than via the
// production getMigrationVersion, which does not strip the directory prefix.
func TestNoDuplicateMigrationVersions(t *testing.T) {
for _, dbType := range []DatabaseType{SQLite, Postgres} {
files, err := GetMigrationFiles(dbType)
if err != nil {
t.Fatalf("%s: failed to get migration files: %v", dbType, err)
}

seen := make(map[int]string, len(files))
for _, file := range files {
base := path.Base(file)
prefix, _, _ := strings.Cut(base, "_")
version, err := strconv.Atoi(prefix)
if err != nil {
t.Errorf("%s: migration %q has no numeric version prefix", dbType, file)
continue
}
if prev, ok := seen[version]; ok {
t.Errorf("%s: duplicate migration version %d: %q and %q", dbType, version, prev, file)
}
seen[version] = file
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Add result URL column for shareable speed test links (e.g. LibreSpeed share URLs)
ALTER TABLE speed_tests ADD COLUMN result_url TEXT;
2 changes: 2 additions & 0 deletions internal/database/migrations/sqlite/022_add_result_url.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Add result URL column for shareable speed test links (e.g. LibreSpeed share URLs)
ALTER TABLE speed_tests ADD COLUMN result_url TEXT;
3 changes: 3 additions & 0 deletions internal/database/speedtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ func (s *service) SaveSpeedTest(ctx context.Context, result types.SpeedTestResul
"latency": result.Latency,
"jitter": result.Jitter,
"is_scheduled": result.IsScheduled,
"result_url": result.ResultURL,
}

// Use provided created_at if available, otherwise default to current UTC time
Expand Down Expand Up @@ -120,6 +121,7 @@ func (s *service) GetSpeedTests(ctx context.Context, timeRange string, page, lim
"latency",
"jitter",
"is_scheduled",
"result_url",
"created_at",
).
OrderBy("created_at DESC").
Expand All @@ -146,6 +148,7 @@ func (s *service) GetSpeedTests(ctx context.Context, timeRange string, page, lim
&result.Latency,
&result.Jitter,
&result.IsScheduled,
&result.ResultURL,
&result.CreatedAt,
)
if err != nil {
Expand Down
48 changes: 48 additions & 0 deletions internal/database/speedtest_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,54 @@ func TestSpeedTest_ScheduledVsManual(t *testing.T) {
})
}

func TestSpeedTest_ResultURLRoundTrip(t *testing.T) {
RunTestWithBothDatabases(t, func(t *testing.T, td *TestDatabase) {
ctx := context.Background()

resultURL := "https://librespeed.org/results/?id=abc123"

withURL := types.SpeedTestResult{
ServerName: "Shared Server",
ServerID: "librespeed-shared",
TestType: "librespeed",
DownloadSpeed: 100.0,
UploadSpeed: 50.0,
ResultURL: &resultURL,
}
savedWithURL, err := td.Service.SaveSpeedTest(ctx, withURL)
require.NoError(t, err)

withoutURL := types.SpeedTestResult{
ServerName: "Unshared Server",
ServerID: "librespeed-unshared",
TestType: "librespeed",
DownloadSpeed: 100.0,
UploadSpeed: 50.0,
// ResultURL left nil
}
savedWithoutURL, err := td.Service.SaveSpeedTest(ctx, withoutURL)
require.NoError(t, err)

results, err := td.Service.GetSpeedTests(ctx, "all", 1, 10)
require.NoError(t, err)

var foundWithURL, foundWithoutURL bool
for _, test := range results.Data {
switch test.ID {
case savedWithURL.ID:
foundWithURL = true
require.NotNil(t, test.ResultURL, "result_url should round-trip as a non-nil pointer")
assert.Equal(t, resultURL, *test.ResultURL)
case savedWithoutURL.ID:
foundWithoutURL = true
assert.Nil(t, test.ResultURL, "absent result_url should read back as nil")
}
}
assert.True(t, foundWithURL, "should find the test saved with a result URL")
assert.True(t, foundWithoutURL, "should find the test saved without a result URL")
})
}

func TestSpeedTest_NullableFields(t *testing.T) {
RunTestWithBothDatabases(t, func(t *testing.T, td *TestDatabase) {
ctx := context.Background()
Expand Down
58 changes: 51 additions & 7 deletions internal/speedtest/librespeed.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
package speedtest

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"os"
"os/exec"
"strconv"
Expand Down Expand Up @@ -119,17 +121,26 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions)

cmd := exec.CommandContext(ctx, "librespeed-cli", args...)

output, err := cmd.CombinedOutput()
if err != nil {
log.Error().Err(err).Str("output", string(output)).Msg("librespeed-cli failed")
return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, string(output))
// Capture stdout and stderr separately: librespeed-cli writes the JSON
// report to stdout, while progress/UI lines and telemetry errors (emitted
// when --share is used) go to stderr. Keeping them apart avoids stderr
// noise corrupting the JSON we parse.
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr

if err := cmd.Run(); err != nil {
log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid logging raw LibreSpeed stdout/stderr payloads.

Line 133 and Line 139 log full process output. That output can include client-identifying JSON fields and share URLs, which is a privacy/compliance risk in logs.

Suggested hardening
-		log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed")
+		log.Error().
+			Err(err).
+			Int("stdout_bytes", stdout.Len()).
+			Int("stderr_bytes", stderr.Len()).
+			Str("stderr", strings.TrimSpace(stderr.String())).
+			Msg("librespeed-cli failed")
@@
-	log.Debug().Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli output")
+	log.Debug().
+		Int("stdout_bytes", stdout.Len()).
+		Int("stderr_bytes", stderr.Len()).
+		Msg("librespeed-cli output captured")

Also applies to: 139-139

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/speedtest/librespeed.go` at line 133, The log call currently emits
raw librespeed-cli stdout/stderr (log.Error().Err(err).Str("stdout",
stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed")),
which can leak client-identifying JSON and share URLs; update the error handling
in the function that invokes the LibreSpeed CLI to stop logging full
stdout/stderr — either remove those .Str("stdout", ...) and .Str("stderr", ...)
fields entirely, or replace them with a sanitized/limited value (e.g., a short
truncated string or a redacted/sanitizer function that strips URLs and known
client fields) and keep the Err(err) and contextual message; ensure the change
touches the same variables (stdout, stderr) and the LibreSpeed invocation site
so no raw payloads are written to logs.

return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String()))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify non-wrapping fmt.Errorf usages where an error variable is formatted with %v/%s.
rg -nP --type=go 'fmt\.Errorf\([^)]*%[vs][^)]*,\s*err\b'

Repository: autobrr/netronome

Length of output: 1178


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the error site and nearby stdout/stderr handling
sed -n '110,170p' internal/speedtest/librespeed.go

# Also show any logging around where stdout/stderr is handled, if farther away
rg -n 'stdout|stderr|TrimSpace\(stderr\.String\(\)\)|TrimSpace\(stdout\.String\(\)\)|librespeed-cli' internal/speedtest/librespeed.go -n

Repository: autobrr/netronome

Length of output: 4013


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show LibrespeedResult definition
rg -n "type LibrespeedResult|LibrespeedResult struct" internal/speedtest/librespeed.go
sed -n '170,260p' internal/speedtest/librespeed.go
sed -n '260,420p' internal/speedtest/librespeed.go

Repository: autobrr/netronome

Length of output: 6431


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the LibrespeedResult struct and related types near the top of the file
sed -n '1,120p' internal/speedtest/librespeed.go

# Confirm which client-identifying fields exist in the JSON struct
rg -n --fixed-strings 'json:"ip"' internal/speedtest/librespeed.go
rg -n --fixed-strings 'json:"hostname"' internal/speedtest/librespeed.go
rg -n --fixed-strings 'json:"location"' internal/speedtest/librespeed.go
rg -n 'Share|share' internal/speedtest/librespeed.go

Repository: autobrr/netronome

Length of output: 3476


Fix error wrapping and stop logging sensitive librespeed output

  • Wrap err with %w (line 134) so callers can unwrap the original failure.
  • Don’t log raw librespeed-cli stdout/stderr (lines 133 and 139): the CLI JSON includes client details (ip, hostname, city/region/country, loc, etc.) and share, which can leak into logs.
Suggested fix
-		return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String()))
+		return nil, fmt.Errorf("librespeed-cli failed: %w: %s", err, strings.TrimSpace(stderr.String()))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String()))
return nil, fmt.Errorf("librespeed-cli failed: %w: %s", err, strings.TrimSpace(stderr.String()))
🧰 Tools
🪛 golangci-lint (2.12.2)

[error] 134-134: non-wrapping format verb for fmt.Errorf. Use %w to format errors

(errorlint)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/speedtest/librespeed.go` at line 134, Wrap the returned error using
%w so callers can unwrap the original error (replace fmt.Errorf("...%v...", err,
...) with fmt.Errorf("...: %w", err) in the function that invokes the
librespeed-cli return), and remove any logging that prints raw libre­speed-cli
stdout/stderr (do not log strings.TrimSpace(stderr.String()) or stdout
contents); instead log a non-sensitive message such as the command exit status
or an anonymized failure note from the same function (identify and update the
error return and the logging statements around the librespeed-cli invocation in
internal/speedtest/librespeed.go).

}

log.Debug().Str("output", string(output)).Msg("librespeed-cli output")
output := stdout.Bytes()

log.Debug().Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli output")

var librespeedResults []LibrespeedResult
if err := json.Unmarshal(output, &librespeedResults); err != nil {
log.Error().Err(err).Str("output", string(output)).Msg("failed to parse librespeed-cli output")
if err := json.Unmarshal(extractJSONArray(output), &librespeedResults); err != nil {
log.Error().Err(err).Str("stdout", stdout.String()).Msg("failed to parse librespeed-cli output")
return nil, fmt.Errorf("failed to parse librespeed-cli output: %w", err)
}

Expand All @@ -151,6 +162,7 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions)
UploadSpeed: librespeedResult.Upload,
Latency: fmt.Sprintf("%.2f", librespeedResult.Ping),
Jitter: librespeedResult.Jitter,
ResultURL: sanitizeResultURL(librespeedResult.Share),
}

// Final completion update
Expand All @@ -172,6 +184,10 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions)
func (r *LibrespeedRunner) buildArgs(opts *types.TestOptions) []string {
args := []string{"--json"}

if r.config.ShareResults {
args = append(args, "--share")
}

if opts.IsPublicServer {
// Keep CLI server IDs in sync with our fetched public list.
args = append(args, "--server-json", librespeedPublicServersURL)
Expand Down Expand Up @@ -326,3 +342,31 @@ func parseCountryFromName(name string) string {
}
return "Unknown"
}

// extractJSONArray trims any leading non-JSON output (e.g. progress/log lines
// librespeed-cli may print before the JSON report) so the result can be
// unmarshalled. It returns b starting at the first '['; if there is no '[' or
// it is already at the start, b is returned unchanged.
func extractJSONArray(b []byte) []byte {
if idx := bytes.IndexByte(b, '['); idx > 0 {
return b[idx:]
}
return b
}

// sanitizeResultURL validates a share URL from librespeed-cli output.
// Returns the normalized URL if valid (http/https with a host), empty string otherwise.
func sanitizeResultURL(raw string) string {
raw = strings.TrimSpace(raw)
if raw == "" {
return ""
}
u, err := url.Parse(raw)
if err != nil {
return ""
}
if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
return ""
}
return u.String()
}
76 changes: 76 additions & 0 deletions internal/speedtest/librespeed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,82 @@ func TestBuildArgsUsesServerJSONForPublicServers(t *testing.T) {
}, args)
}

func TestBuildArgsShareFlag(t *testing.T) {
tests := []struct {
name string
shareResults bool
wantShare bool
}{
{"included when enabled", true, true},
{"excluded when disabled", false, false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
runner := NewLibrespeedRunner(config.LibrespeedConfig{
ServersPath: "/tmp/local-servers.json",
ShareResults: tt.shareResults,
})

args := runner.buildArgs(&types.TestOptions{
IsPublicServer: true,
ServerIDs: []string{"123"},
})

if tt.wantShare {
assert.Contains(t, args, "--share")
} else {
assert.NotContains(t, args, "--share")
}
})
}
}

func TestSanitizeResultURL(t *testing.T) {
tests := []struct {
name string
raw string
want string
}{
{"valid https URL", "https://librespeed.org/results/?id=abc123", "https://librespeed.org/results/?id=abc123"},
{"valid http URL", "http://librespeed.org/results/?id=abc123", "http://librespeed.org/results/?id=abc123"},
{"uppercase scheme normalized", "HTTPS://librespeed.org/results/?id=abc123", "https://librespeed.org/results/?id=abc123"},
{"surrounding whitespace trimmed", " https://librespeed.org/results/?id=abc123 ", "https://librespeed.org/results/?id=abc123"},
{"empty string", "", ""},
{"no scheme", "librespeed.org/results", ""},
{"javascript scheme", "javascript:alert(1)", ""},
{"ftp scheme", "ftp://example.com/file", ""},
{"no host", "https:///path", ""},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, sanitizeResultURL(tt.raw))
})
}
}

func TestExtractJSONArray(t *testing.T) {
tests := []struct {
name string
in string
want string
}{
{"clean array", `[{"download":1}]`, `[{"download":1}]`},
{"array at offset zero unchanged", `[1,2,3]`, `[1,2,3]`},
{"leading log line", "Selected server: foo\n[{\"download\":1}]", `[{"download":1}]`},
{"multiple leading lines", "retrieving servers\nrunning test\n[{\"download\":1}]", `[{"download":1}]`},
{"no array present", "no json here", "no json here"},
{"empty", "", ""},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, string(extractJSONArray([]byte(tt.in))))
})
}
}

func TestBuildArgsUsesLocalJSONForCustomServers(t *testing.T) {
runner := NewLibrespeedRunner(config.LibrespeedConfig{
ServersPath: "/tmp/local-servers.json",
Expand Down
6 changes: 6 additions & 0 deletions internal/speedtest/result_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func (h *DefaultResultHandler) SaveResult(ctx context.Context, result *Result, t
jitterPtr = &result.Jitter
}

var resultURL *string
if result.ResultURL != "" {
resultURL = &result.ResultURL
}

// Give the DB write up to 10s while still respecting upstream cancellations and values.
saveCtx, saveCancel := context.WithTimeout(ctx, 10*time.Second)
defer saveCancel()
Expand All @@ -73,6 +78,7 @@ func (h *DefaultResultHandler) SaveResult(ctx context.Context, result *Result, t
Latency: result.Latency,
Jitter: jitterPtr,
IsScheduled: opts.IsScheduled,
ResultURL: resultURL,
CreatedAt: createdAt,
})
if err != nil {
Expand Down
Loading
Loading