Skip to content

fix: database query built from user-controlled sources #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
odaysec wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: database query built from user-controlled sources #290

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 14 additions & 1 deletion cmd/metrics-scraper/app/routes/metrics/handlerqueries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"net/http"
"strings"
"time"

"regexp"
"github.com/gin-gonic/gin"

"github.com/karmada-io/dashboard/cmd/metrics-scraper/app/scrape"
Expand Down Expand Up @@ -74,6 +74,11 @@ func QueryMetrics(c *gin.Context) {
}

func queryMetricNames(c *gin.Context, tx *sql.Tx, sanitizedPodName string) {
if !isValidTableName(sanitizedPodName) {
log.Printf("Invalid table name: %v", sanitizedPodName)
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid table name"})
return
}
Comment on lines +77 to +81
Copy link
Copy Markdown
Member

@RainbowMango RainbowMango Oct 28, 2025

Choose a reason for hiding this comment

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

Right, this is a typical SQL injection issue, like an attacker could exploit this by providing a malicious table name (e.g., podName; DROP TABLE users; --), which could result in data deletion...

rows, err := tx.Query(fmt.Sprintf("SELECT DISTINCT name FROM %s", sanitizedPodName))
if err != nil {
log.Printf("Error querying metric names: %v, SQL Error: %v", err, err)
Expand All @@ -96,6 +101,14 @@ func queryMetricNames(c *gin.Context, tx *sql.Tx, sanitizedPodName string) {
c.JSON(http.StatusOK, gin.H{"metricNames": metricNames})
}

// isValidTableName checks if the given table name is a valid identifier.
func isValidTableName(name string) bool {
// Only allow table names that start with a letter or underscore, then letters, numbers, underscores, up to 64 chars
// Adjust length as per your table name limits (e.g., SQLite, MySQL usually 64)
validTableName := regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]{0,63}$`)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Oct 28, 2025

Choose a reason for hiding this comment

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

high

regexp.MustCompile is a relatively expensive operation. Compiling the regular expression on every call to isValidTableName is inefficient. It's a best practice in Go to compile regular expressions once at the package level (e.g., in a var block at the top of the file) and reuse the compiled object. This will improve performance significantly if the function is called frequently.

For example:

var (
	validTableName = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]{0,63}$`)
)

// ... in isValidTableName ...
return validTableName.MatchString(name)

Copy link
Copy Markdown
Member

@RainbowMango RainbowMango Oct 28, 2025

Choose a reason for hiding this comment

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

It makes sense. @odaysec what do you think?

Copy link
Copy Markdown
Contributor

@warjiang warjiang Oct 28, 2025

Choose a reason for hiding this comment

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

+1

return validTableName.MatchString(name)
}

func queryMetricDetailsByName(c *gin.Context, tx *sql.Tx, sanitizedPodName, metricName string) {
if metricName == "" {
c.JSON(http.StatusBadRequest, gin.H{"error": "Metric name required for details"})
Expand Down