fix(gcp): validate Pub/Sub resource name in BuildMQLQuery#7468
fix(gcp): validate Pub/Sub resource name in BuildMQLQuery#7468rickbrouwer wants to merge 3 commits intokedacore:mainfrom
Conversation
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
|
/run-e2e gcp |
There was a problem hiding this comment.
Pull request overview
Adds input validation to the GCP Pub/Sub scaler’s MQL query builder to reject invalid Pub/Sub resource names before sending queries to Cloud Monitoring.
Changes:
- Add regex-based validation for Pub/Sub resource names used by
BuildMQLQuery. - Fail early from
BuildMQLQuerywhen the resource name is invalid. - Add an Unreleased changelog entry for the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/scalers/gcp/gcp_stackdriver_client.go | Introduces Pub/Sub resource name validation and applies it in BuildMQLQuery. |
| CHANGELOG.md | Documents the fix in the Unreleased “Fixes” section. |
Comments suppressed due to low confidence (2)
pkg/scalers/gcp/gcp_stackdriver_client.go:376
- The MQL query is built via fmt.Sprintf with unescaped %s substitutions (metric/resourceType). While resourceName is now validated, metric is derived from user-controlled metadata (mode) and can still contain quotes/pipes, enabling MQL injection and bypassing the intended fixed query shape. Consider validating metric/resourceType against strict allowlists/regexes (or escaping) before embedding them into the query string.
q := fmt.Sprintf(
"fetch pubsub_%s | metric '%s' | filter (resource.project_id == '%s' && resource.%s_id == '%s') | within %s",
resourceType, metric, pid, resourceType, resourceName, th,
)
CHANGELOG.md:84
- Changelog entry label is very broad (“GCP Scaler”), but the change only affects the GCP Pub/Sub scaler path (BuildMQLQuery used by gcp_pubsub_scaler). Consider renaming the entry to “GCP Pub/Sub Scaler” to match other changelog entries and make the impacted component clear.
- **GCP Scaler**: Validate Pub/Sub resource name in BuildMQLQuery ([#7468](https://github.com/kedacore/keda/pull/7468))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
| func validateResourceName(name string) error { | ||
| if !validPubSubName.MatchString(name) { | ||
| return fmt.Errorf("invalid Pub/Sub resource name %q", name) | ||
| } |
There was a problem hiding this comment.
do we also want to add a check for the forbidden goog prefix?
https://docs.cloud.google.com/pubsub/docs/pubsub-basics#resource_names
doesn't need to be as part of the regex because I think it would have to be through a negative lookahead but can be simple
strings.HasPrefix(strings.ToLower(name), "goog")wdyt?
There was a problem hiding this comment.
Yeah, that's true. It would to be through a negative lookahead, but that's possible.
I think:
var validPubSubName = regexp.MustCompile(`^(?!goog)[A-Za-z][A-Za-z0-9\-_.~+%]{2,254}$`)
Or would you prefer it separately?
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
/run-e2e gcp |
validate Pub/Sub resource name in BuildMQLQuery
Checklist