refactor(component,generic,http): replace env-based URL validation with constructor injection #1121
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| package http | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net" | ||
| "net/url" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/instill-ai/pipeline-backend/config" | ||
| errorsx "github.com/instill-ai/x/errors" | ||
| ) | ||
|
|
||
| // URLValidator defines the interface for validating HTTP input | ||
| type URLValidator interface { | ||
| ValidateInput(input *httpInput) error | ||
| } | ||
|
|
||
| // urlValidator provides common validation logic | ||
| type urlValidator struct { | ||
| whitelistedEndpoints []string | ||
| allowLocalhost bool | ||
| allowPrivateIPs bool | ||
| } | ||
|
|
||
| // NewURLValidator creates a validator for production use | ||
| func NewURLValidator() URLValidator { | ||
| return &urlValidator{allowPrivateIPs: false} | ||
| } | ||
|
|
||
| // NewTestURLValidator creates a validator for testing | ||
| func NewTestURLValidator(whitelistedEndpoints []string, allowLocalhost bool) URLValidator { | ||
| return &urlValidator{ | ||
| whitelistedEndpoints: whitelistedEndpoints, | ||
| allowLocalhost: allowLocalhost, | ||
| allowPrivateIPs: true, // Test mode allows external URLs by default | ||
| } | ||
| } | ||
|
Comment on lines
+31
to
+38
Collaborator
There was a problem hiding this comment. Should we define this in the non-production code ( |
||
|
|
||
| // ValidateInput implements the consolidated validation logic | ||
| func (v *urlValidator) ValidateInput(input *httpInput) error { | ||
| if input == nil { | ||
| return fmt.Errorf("input cannot be nil") | ||
| } | ||
|
|
||
| endpointURL := input.EndpointURL | ||
| if endpointURL == "" { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("endpoint URL is required"), | ||
| "Endpoint URL must be provided", | ||
| ) | ||
| } | ||
|
|
||
| parsedURL, err := url.Parse(endpointURL) | ||
| if err != nil { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("parsing endpoint URL: %w", err), | ||
| "Couldn't parse the endpoint URL as a valid URI reference", | ||
| ) | ||
| } | ||
|
|
||
| host := parsedURL.Hostname() | ||
| if host == "" { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("missing hostname"), | ||
| "Endpoint URL must have a hostname", | ||
| ) | ||
| } | ||
|
|
||
| // Check explicit whitelist first | ||
| for _, allowedEndpoint := range v.whitelistedEndpoints { | ||
| if strings.HasPrefix(endpointURL, allowedEndpoint) { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // Production-specific whitelisted hosts | ||
| if !v.allowPrivateIPs { | ||
| prodWhitelist := []string{ | ||
| // Pipeline's public port is exposed to call pipelines from pipelines. | ||
| // When a `pipeline` component is implemented, this won't be necessary. | ||
| fmt.Sprintf("%s:%d", config.Config.Server.InstanceID, config.Config.Server.PublicPort), | ||
| // Model's public port is exposed until the model component allows | ||
| // triggering models in the custom mode. | ||
| fmt.Sprintf("%s:%d", config.Config.ModelBackend.Host, config.Config.ModelBackend.PublicPort), | ||
| } | ||
| // Certain pipelines used by artifact-backend need to trigger pipelines and | ||
| // models via this component. | ||
| // TODO jvallesm: Remove this after INS-8119 is completed. | ||
| if slices.Contains(prodWhitelist, parsedURL.Host) { | ||
| return nil | ||
| } | ||
| } | ||
|
Comment on lines
+78
to
+93
Collaborator
There was a problem hiding this comment. note: this validator gives us the possibility to move the the model and pipeline whitelisting away from the CE repo, as it is used only in production. However, I'm fine with having this here out of simplicity and fairness with CE users (they might also want to trigger pipelines or models from other pipelines) |
||
|
|
||
| // Check localhost allowance | ||
| if v.allowLocalhost && (host == "localhost" || host == "127.0.0.1" || strings.HasPrefix(host, "127.")) { | ||
| return nil | ||
| } | ||
|
|
||
| // Production mode: check if IP is private and block private IPs | ||
| if !v.allowPrivateIPs { | ||
| ips, err := net.LookupIP(host) | ||
| if err != nil { | ||
| return fmt.Errorf("looking up IP: %w", err) | ||
| } | ||
|
|
||
| for _, ip := range ips { | ||
| if ip.IsPrivate() || ip.IsLoopback() { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("endpoint URL resolves to private/internal IP address"), | ||
| "URL must point to a publicly available endpoint (no private/internal addresses)", | ||
| ) | ||
| } | ||
| } | ||
| // Production mode: allow public IPs | ||
| return nil | ||
| } | ||
|
|
||
| // Test mode: apply whitelist and localhost restrictions | ||
| if len(v.whitelistedEndpoints) > 0 { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("endpoint URL not in test whitelist"), | ||
| fmt.Sprintf("URL %s is not in the whitelisted endpoints for testing", endpointURL), | ||
| ) | ||
| } | ||
|
|
||
| if !v.allowLocalhost { | ||
| return errorsx.AddMessage( | ||
| fmt.Errorf("endpoint URL not allowed"), | ||
| "No endpoints are whitelisted for testing and localhost access is disabled", | ||
| ) | ||
| } | ||
|
|
||
| // Test mode: allow if localhost is enabled | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd put this away from the production code