Skip to content

Commit

Permalink
fix: isloopback returns true incorrectly (#765)
Browse files Browse the repository at this point in the history
Closes #750
  • Loading branch information
james-d-elliott authored Aug 22, 2023
1 parent 1df109b commit 44fd2cc
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 2 deletions.
7 changes: 5 additions & 2 deletions authorize_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,14 @@ func isMatchingAsLoopback(requested *url.URL, registeredURI string) bool {
return false
}

var (
regexLoopbackAddress = regexp.MustCompile(`^(127\.0\.0\.1|\[::1])(:\d+)?$`)

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

Hm, loopback addresses can be anything under 127.*?

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor

The change specifically fixes the linked issue. I'm under the impression this may be a naming issue as I believe the spec only has the special matching rules for 127.0.0.1 and [::1]. On that basis I think it's unwise to change the underlying intent behind the pattern without evidence to the contrary.

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

So Section 7.3 starts with:

Native apps that are able to open a port on the loopback network interface without needing special permissions (typically, those on desktop operating systems) can use the loopback interface to receive the OAuth redirect.

Bold is mine. So they talk about interface, not an address. So I feel the 127.0.0.1 below is just an example.

BTW, I do not have strong feelings here. I just noticed this and thought to raise the question.

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor

Yeah makes sense. I think if we want to do this we need to use a proper solution by parsing the host portion as a net.IP and comparing it to a valid subnet. I can relatively easily put that solution together either way.

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor
isLoopbackAddress(requested.Hostname())

.....


var ipv4loopback = net.IPNet{IP: net.ParseIP("127.0.0.1"), Mask: net.CIDRMask(8, net.IPv4len)}

var ipv6loopback = net.IPNet{IP: net.ParseIP("[::1]"), Mask: net.CIDRMask(128, net.IPv6len)}

func isLoopbackAddress(hostname string) bool {
	ip := net.ParseIP(hostname)

	if ip == nil {
		return false
	}

	return ipv4loopback.Contains(ip) || ipv6loopback.Contains(ip)
}

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

I think you need net.SplitHostPort first?

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor
	host, _, err := net.SplitHostPort(hostname)
	if err != nil {
		host = hostname
	}
	return net.ParseIP(host).IsLoopback()

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor

We would just use the Hostname() part of url.URL as the example above. Since the function is internal we can easily make a breaking change to what the parameter expected is.

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

Perfect. Then we have simple net.ParseIP(requested.Hostname()).IsLoopback() and this is it? :-)

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor

You probably need to check the nillyness of the IP first, as a non-IP will be nil after parsing. Otherwise I agree, looks good.

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

It works without the check: https://go.dev/play/p/rqKcvet3NPG

This comment has been minimized.

Copy link
@james-d-elliott

james-d-elliott Feb 13, 2024

Author Contributor

That's interesting, guessing because the type it returns is not a pointer. Nice!

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

Even if it is nil, you can have pointer receiver methods which check if the pointer is nil and return something. So IsLoopback could have explicit if value == nil { return false} in there and also work.

This comment has been minimized.

Copy link
@mitar

mitar Feb 13, 2024

Contributor

I made: #795

)

// Check if address is either an IPv4 loopback or an IPv6 loopback-
// An optional port is ignored
func isLoopbackAddress(address string) bool {
match, _ := regexp.MatchString("^(127.0.0.1|\\[::1\\])(:?)(\\d*)$", address)
return match
return regexLoopbackAddress.MatchString(address)
}

// IsValidRedirectURI validates a redirect_uri as specified in:
Expand Down
69 changes: 69 additions & 0 deletions authorize_helper_whitebox_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright © 2023 Ory Corp
// SPDX-License-Identifier: Apache-2.0

package fosite

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestIsLookbackAddress(t *testing.T) {
testCases := []struct {
name string
have string
expected bool
}{
{
"ShouldReturnTrueIPv4Loopback",
"127.0.0.1",
true,
},
{
"ShouldReturnTrueIPv4LoopbackWithPort",
"127.0.0.1:1230",
true,
},
{
"ShouldReturnTrueIPv6Loopback",
"[::1]",
true,
},
{
"ShouldReturnTrueIPv6LoopbackWithPort",
"[::1]:1230",
true,
}, {
"ShouldReturnFalse12700255",
"127.0.0.255",
false,
},
{
"ShouldReturnFalse12700255WithPort",
"127.0.0.255:1230",
false,
},
{
"ShouldReturnFalseInvalidFourthOctet",
"127.0.0.11230",
false,
},
{
"ShouldReturnFalseInvalidIPv4",
"127x0x0x11230",
false,
},
{
"ShouldReturnFalseInvalidIPv6",
"[::1]1230",
false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.expected, isLoopbackAddress(tc.have))
})
}
}

0 comments on commit 44fd2cc

Please sign in to comment.