Copilot commented on code in PR #1492:
URL: https://github.com/apache/pulsar-client-go/pull/1492#discussion_r3305011025
##########
pulsar/negative_backoff_policy.go:
##########
@@ -41,11 +41,10 @@ func (nbp *defaultNackBackoffPolicy) Next(redeliveryCount
uint32) time.Duration
minNackTime := 1 * time.Second // 1sec
maxNackTime := 10 * time.Minute // 10min
- backoff := float64(minNackTime << redeliveryCount)
- if backoff == 0 {
- // Overflow so we assign the maximum value of the backoff.
- backoff = float64(maxNackTime)
+ if redeliveryCount >= 63 {
+ return maxNackTime
}
+ backoff := float64(minNackTime << redeliveryCount)
return time.Duration(math.Min(backoff, float64(maxNackTime)))
Review Comment:
The `redeliveryCount >= 63` guard doesn’t actually prevent overflow/invalid
durations for smaller counts. `minNackTime` is a signed `time.Duration`
(int64), so `minNackTime << redeliveryCount` can become negative well before 63
(e.g. shifting 1s by ~34 bits overflows int64). That negative value then flows
into `math.Min`, producing a negative `time.Duration` and causing immediate
redelivery. Consider clamping before shifting based on `maxNackTime` (e.g.,
return `maxNackTime` once the backoff would exceed max) or using a safe
multiplication/loop that caps at `maxNackTime` without relying on
potentially-overflowing shifts. Also add a regression test around the overflow
boundary (e.g. `Next(34)` should return `maxNackTime`).
##########
oauth2/oidc_endpoint_provider.go:
##########
@@ -40,6 +40,17 @@ func GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL
string) (*OIDCWellKnownEnd
if err != nil {
return nil, errors.Wrap(err, "could not parse issuer url to
build well known endpoints")
}
+
+ // Validate URL scheme - only allow HTTPS to prevent SSRF attacks
+ if u.Scheme != "https" {
+ return nil, errors.New("issuer URL must use HTTPS scheme")
Review Comment:
Requiring `https` here is a breaking behavior change: existing unit tests
call this with `httptest.NewServer(...)` (which is `http://...`), and common
local/dev OIDC setups also run over plain HTTP. If the intent is to enforce
HTTPS in production, consider making the HTTP client/transport injectable (so
tests can use `httptest.NewTLSServer` with its client) or relaxing the
restriction in a controlled way (e.g., allow caller-provided client/validation).
##########
oauth2/oidc_endpoint_provider.go:
##########
@@ -40,6 +40,17 @@ func GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL
string) (*OIDCWellKnownEnd
if err != nil {
return nil, errors.Wrap(err, "could not parse issuer url to
build well known endpoints")
}
+
+ // Validate URL scheme - only allow HTTPS to prevent SSRF attacks
+ if u.Scheme != "https" {
+ return nil, errors.New("issuer URL must use HTTPS scheme")
+ }
+
+ // Validate host is not empty
+ if u.Host == "" {
Review Comment:
The host validation via `u.Host == ""` is incomplete (e.g., `https://:443`
yields a non-empty `Host` but an empty hostname). This also changes the error
surfaced for inputs like `https://` from a wrapped `http.Get` error to a new
validation error, which will break existing expectations/tests. Consider
validating `u.Hostname()` (and that the URL is absolute) so invalid hosts are
caught consistently, and ensure tests/error expectations are updated
accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
