Skip to content
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.

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

Add missing control char on regex validation #9465

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,18 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {

k8s.SetDefaultNGINXPathType(ing)

for _, rule := range ing.Spec.Rules {
if rule.HTTP == nil {
continue
}

for _, path := range rule.HTTP.Paths {
if !utilingress.IsSafePath(ing, path.Path) {
return fmt.Errorf("ingress contains invalid path: %s", path.Path)
}
}
}

allIngresses := n.store.ListIngresses()

filter := func(toCheck *ingress.Ingress) bool {
Expand Down
17 changes: 17 additions & 0 deletions internal/ingress/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,23 @@ func TestCheckIngress(t *testing.T) {
t.Errorf("with a new ingress without error, no error should be returned")
}
})

t.Run("When invalid path is passed to Ingress", func(t *testing.T) {
nginx.store = fakeIngressStore{
ingresses: []*ingress.Ingress{},
}
nginx.command = testNginxTestCommand{
t: t,
err: nil,
}
ing.Spec.Rules[0].HTTP.Paths = append(ing.Spec.Rules[0].HTTP.Paths, networking.HTTPIngressPath{
Path: "/foo/bar/;xpto",
})
if err := nginx.CheckIngress(ing); err == nil {
t.Errorf("with an invalid path, ingress should be rejected")
}
})

rikatz marked this conversation as resolved.
Show resolved Hide resolved
})

t.Run("When the ingress is marked as deleted", func(t *testing.T) {
Expand Down
18 changes: 14 additions & 4 deletions pkg/util/ingress/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ import (

const (
alphaNumericChars = `\-\.\_\~a-zA-Z0-9/`
regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+`
regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+\?\|`
)

var (
// pathAlphaNumeric is a regex validation of something like "^/[a-zA-Z]+$" on path
pathAlphaNumeric = regexp.MustCompile("^/[" + alphaNumericChars + "]*$").MatchString
pathAlphaNumeric = regexp.MustCompile("^[/" + alphaNumericChars + "]*$").MatchString
// pathRegexEnabled is a regex validation of paths that may contain regex.
pathRegexEnabled = regexp.MustCompile("^/[" + alphaNumericChars + regexEnabledChars + "]*$").MatchString
pathRegexEnabled = regexp.MustCompile("^[/" + alphaNumericChars + regexEnabledChars + "]*$").MatchString
)

func GetRemovedHosts(rucfg, newcfg *ingress.Configuration) []string {
Expand Down Expand Up @@ -251,7 +251,17 @@ func BuildRedirects(servers []*ingress.Server) []*redirect {
// It will behave differently if regex is enabled or not
func IsSafePath(copyIng *networkingv1.Ingress, path string) bool {
isRegex, _ := parser.GetBoolAnnotation("use-regex", copyIng)
if isRegex {
isRewrite, _ := parser.GetBoolAnnotation("rewrite-target", copyIng)
isImplSpecific := false
for _, rules := range copyIng.Spec.Rules {
for _, paths := range rules.HTTP.Paths {
if paths.PathType != nil && *paths.PathType == networkingv1.PathTypeImplementationSpecific {
isImplSpecific = true
break
}
}
}
if isRegex || isRewrite || isImplSpecific {
return pathRegexEnabled(path)
}
return pathAlphaNumeric(path)
Expand Down
24 changes: 24 additions & 0 deletions pkg/util/ingress/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,18 @@ func TestIsSafePath(t *testing.T) {
path string
want bool
}{
{
name: "should accept empty path",
want: true,
copyIng: generateDumbIngressforPathTest(false),
path: "",
},
{
name: "should accept / path",
want: true,
copyIng: generateDumbIngressforPathTest(false),
path: "/",
},
{
name: "should accept valid path with regex disabled",
want: true,
Expand Down Expand Up @@ -194,6 +206,18 @@ func TestIsSafePath(t *testing.T) {
copyIng: generateDumbIngressforPathTest(true),
path: "/foo/bar/(.+)",
},
{
name: "should accept another regex path when regex is enabled",
want: true,
copyIng: generateDumbIngressforPathTest(true),
path: "/v1/?(.*)",
},
{
name: "should accept more regex path when regex is enabled",
want: true,
copyIng: generateDumbIngressforPathTest(true),
path: "/something(/|$)(.*)",
},
{
name: "should reject regex path when regex is enabled but the path is invalid",
want: false,
Expand Down