Verify SNS host for both Subscribe() and Unsubscribe()#3763
Verify SNS host for both Subscribe() and Unsubscribe()#3763
Subscribe() and Unsubscribe()#3763Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the SNS ingestor helper by validating that both SubscribeURL and UnsubscribeURL are HTTPS and on an expected SNS host before issuing confirmation requests.
Changes:
- Added URL parsing + scheme checks for
SubscribeURLconfirmation. - Added URL parsing + scheme checks for
UnsubscribeURLconfirmation (including empty-string guard). - Added host allowlist validation via
hostPatternfor both flows.
| if subscribeURL.Scheme != "https" { | ||
| return response, fmt.Errorf("url should be using https") | ||
| } | ||
|
|
||
| if !hostPattern.Match([]byte(subscribeURL.Host)) { | ||
| return response, fmt.Errorf("subscribe url is located on an invalid domain") |
There was a problem hiding this comment.
Domain validation is using subscribeURL.Host, which includes the port (and preserves casing). This can incorrectly reject valid URLs like sns.<region>.amazonaws.com:443, and it will also reject mixed-case hosts even though DNS is case-insensitive. Consider validating strings.ToLower(subscribeURL.Hostname()) and using hostPattern.MatchString(...) to avoid the extra []byte allocation.
| if unsubscribeURL.Scheme != "https" { | ||
| return response, fmt.Errorf("url should be using https") | ||
| } | ||
|
|
||
| if !hostPattern.Match([]byte(unsubscribeURL.Host)) { | ||
| return response, fmt.Errorf("unsubscribe url is located on an invalid domain") |
There was a problem hiding this comment.
Same as Subscribe(): validation uses unsubscribeURL.Host which includes the port and may preserve mixed-case hostnames. Prefer validating strings.ToLower(unsubscribeURL.Hostname()) and hostPattern.MatchString(...) so the check is resilient to :443 and avoids extra allocation.
| if payload.UnsubscribeURL == "" { | ||
| return response, errors.New("payload does not have an UnsubscribeURL") | ||
| } |
There was a problem hiding this comment.
New error string casing/punctuation is inconsistent with the existing errors in this file (e.g., Subscribe() uses Payload...!). Go conventions also recommend lowercase error messages without punctuation. Consider normalizing the error messages for Subscribe/Unsubscribe (and ideally the existing ones too) to a single style, and include context like "unsubscribe URL" in the https/domain validation errors to make debugging easier.
Description
Make sure we verify SNS payloads' subscribe and unsubscribe URLs.
Type of change