Fixed query parameters causing route matching problems (#37)

Fixed query parameters causing route matching problems in original request headers case.

Author: kaancfidan

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 There are currently no unreleased changes.
 
+## [v0.0.2] - 2021-03-25
+### Fixed
+- Query parameters in request causing route matching failures when request paths are received through headers.
+
 ## [v0.0.1] - 2020-06-12
 ### Added
 - Support for original request path and method specification through headers (see [nginx docs](https://docs.nginx.com/nginx/admin-guide/security-controls/configuring-subrequest-authentication/)).
@@ -24,5 +28,6 @@ This is the first version that includes the following functionality:
 - Claims-based authorization
 - Pure authorization server and reverse proxy modes
 
-[Unreleased]: https://github.com/kaancfidan/bouncer/compare/v0.0.1...master
+[Unreleased]: https://github.com/kaancfidan/bouncer/compare/v0.0.2...master
+[v0.0.2]: https://github.com/kaancfidan/bouncer/compare/v0.0.1...v0.0.2
 [v0.0.1]: https://github.com/kaancfidan/bouncer/compare/v0.0.0...v0.0.1

services/route_matcher.go

@@ -1,6 +1,8 @@
 package services
 
 import (
+	"fmt"
+	"net/url"
 	"strings"
 
 	"github.com/gobwas/glob"
@@ -29,12 +31,17 @@ func NewRouteMatcher(routePolicies []models.RoutePolicy) *RouteMatcherImpl {
 func (g RouteMatcherImpl) MatchRoutePolicies(path string, method string) ([]models.RoutePolicy, error) {
 	matches := make([]models.RoutePolicy, 0)
 	for _, rp := range g.routePolicies {
-		normalizedPath := "/" + strings.Trim(path, " \t\n/") + "/"
+		parsed, err := url.Parse(path)
+		if err != nil {
+			return nil, fmt.Errorf("could not parse path: %v", err)
+		}
+
+		normalizedPath := "/" + strings.Trim(parsed.Path, " \t\n/") + "/"
 		normalizedPolicyPath := "/" + strings.Trim(rp.Path, " \t\n/") + "/"
 
 		g, err := glob.Compile(normalizedPolicyPath, '/')
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("could not compile policy glob: %v", err)
 		}
 
 		// check if route matches

services/route_matcher_test.go

@@ -31,6 +31,15 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
+		{
+			name: "path error",
+			routePolicies: []models.RoutePolicy{
+				{Path: "/test", Methods: []string{"GET"}},
+			},
+			path:    ".::this is not a valid path::.",
+			want:    nil,
+			wantErr: true,
+		},
 		{
 			name: "exact matched route",
 			routePolicies: []models.RoutePolicy{
@@ -44,7 +53,19 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "exact matched route with trailing separator",
+			name: "exact matched route with trailing separator in request path",
+			routePolicies: []models.RoutePolicy{
+				{Path: "/test", Methods: []string{"GET"}},
+			},
+			path:   "/test/",
+			method: "GET",
+			want: []models.RoutePolicy{
+				{Path: "/test", Methods: []string{"GET"}},
+			},
+			wantErr: false,
+		},
+		{
+			name: "exact matched route with trailing separator in route policy",
 			routePolicies: []models.RoutePolicy{
 				{Path: "/test/", Methods: []string{"GET"}},
 			},
@@ -55,6 +76,18 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "exact matched route with query parameters",
+			routePolicies: []models.RoutePolicy{
+				{Path: "/test/", Methods: []string{"GET"}},
+			},
+			path:   "/test?someBool=true&someString=test",
+			method: "GET",
+			want: []models.RoutePolicy{
+				{Path: "/test/", Methods: []string{"GET"}},
+			},
+			wantErr: false,
+		},
 		{
 			name: "exact matched route with spaces all around",
 			routePolicies: []models.RoutePolicy{

services/server.go

@@ -3,6 +3,7 @@ package services
 import (
 	"log"
 	"net/http"
+	"net/url"
 	"reflect"
 
 	"github.com/google/uuid"
@@ -50,7 +51,14 @@ func (s Server) Handle(writer http.ResponseWriter, request *http.Request) {
 		path = request.URL.Path
 		method = request.Method
 	} else {
-		path = request.Header.Get(s.config.OriginalRequestHeaders.Path)
+		parsed, err := url.Parse(request.Header.Get(s.config.OriginalRequestHeaders.Path))
+		if err != nil {
+			log.Printf("[%v] Request path read from header could not be parsed: %v", requestID, err)
+			writer.WriteHeader(http.StatusBadRequest)
+			return
+		}
+
+		path = parsed.Path
 		method = request.Header.Get(s.config.OriginalRequestHeaders.Method)
 	}
 

services/server_test.go

@@ -18,13 +18,16 @@ import (
 func TestServer_Handle(t *testing.T) {
 	tests := []struct {
 		name               string
+		requestPath        string
+		config             models.ServerConfig
 		proxyEnabled       bool
 		expectations       func(*http.Request, *mocks.RouteMatcher, *mocks.Authenticator, *mocks.Authorizer)
 		wantUpstreamCalled bool
 		wantStatusCode     int
 	}{
 		{
 			name:         "route matching failed",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -38,8 +41,28 @@ func TestServer_Handle(t *testing.T) {
 			wantUpstreamCalled: false,
 			wantStatusCode:     http.StatusInternalServerError,
 		},
+		{
+			name:        "invalid path in original request headers",
+			requestPath: "**-.::invalid path::.-**",
+			config: models.ServerConfig{
+				OriginalRequestHeaders: &models.OriginalRequestHeaders{
+					Method: "X-Original-Method",
+					Path:   "X-Original-URI",
+				},
+			},
+			proxyEnabled: false,
+			expectations: func(
+				request *http.Request,
+				routeMatcher *mocks.RouteMatcher,
+				authenticator *mocks.Authenticator,
+				authorizer *mocks.Authorizer) {
+			},
+			wantUpstreamCalled: false,
+			wantStatusCode:     http.StatusBadRequest,
+		},
 		{
 			name:         "allow anonymous",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -62,8 +85,43 @@ func TestServer_Handle(t *testing.T) {
 			wantUpstreamCalled: false,
 			wantStatusCode:     http.StatusOK,
 		},
+		{
+			name:        "allow anonymous through original request headers",
+			requestPath: "/some/path",
+			config: models.ServerConfig{
+				OriginalRequestHeaders: &models.OriginalRequestHeaders{
+					Method: "X-Original-Method",
+					Path:   "X-Original-URI",
+				},
+			},
+			proxyEnabled: false,
+			expectations: func(
+				request *http.Request,
+				routeMatcher *mocks.RouteMatcher,
+				authenticator *mocks.Authenticator,
+				authorizer *mocks.Authorizer) {
+
+				matchedRoutes := []models.RoutePolicy{
+					{
+						Path:           "/",
+						AllowAnonymous: true,
+					},
+				}
+
+				routeMatcher.On("MatchRoutePolicies",
+					request.Header.Get("X-Original-URI"),
+					request.Header.Get("X-Original-Method")).Return(matchedRoutes, nil)
+
+				authorizer.On("IsAnonymousAllowed",
+					matchedRoutes,
+					request.Header.Get("X-Original-Method")).Return(true)
+			},
+			wantUpstreamCalled: false,
+			wantStatusCode:     http.StatusOK,
+		},
 		{
 			name:         "proxy enabled, allow anonymous",
+			requestPath:  "/some/path",
 			proxyEnabled: true,
 			expectations: func(
 				request *http.Request,
@@ -88,6 +146,7 @@ func TestServer_Handle(t *testing.T) {
 		},
 		{
 			name:         "authentication - success, authorization - success",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -122,6 +181,7 @@ func TestServer_Handle(t *testing.T) {
 		},
 		{
 			name:         "proxy enabled, authentication - success, authorization - success",
+			requestPath:  "/some/path",
 			proxyEnabled: true,
 			expectations: func(
 				request *http.Request,
@@ -156,6 +216,7 @@ func TestServer_Handle(t *testing.T) {
 		},
 		{
 			name:         "authentication - failed",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -179,6 +240,7 @@ func TestServer_Handle(t *testing.T) {
 		},
 		{
 			name:         "authentication - success, authorization - failed",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -218,6 +280,7 @@ func TestServer_Handle(t *testing.T) {
 		},
 		{
 			name:         "error while authorization",
+			requestPath:  "/some/path",
 			proxyEnabled: false,
 			expectations: func(
 				request *http.Request,
@@ -260,10 +323,24 @@ func TestServer_Handle(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			header := http.Header{}
 
-			request, err := http.NewRequest("GET", "/", nil)
-			if err != nil {
-				t.Errorf("could not be create request: %v", err)
-				return
+			var request *http.Request
+			var err error
+
+			if tt.config.OriginalRequestHeaders != nil {
+				request, err = http.NewRequest("GET", "/", nil)
+				if err != nil {
+					t.Errorf("could not be create request: %v", err)
+					return
+				}
+
+				request.Header.Set("X-Original-URI", tt.requestPath)
+				request.Header.Set("X-Original-Method", "POST")
+			} else {
+				request, err = http.NewRequest("POST", tt.requestPath, nil)
+				if err != nil {
+					t.Errorf("could not be create request: %v", err)
+					return
+				}
 			}
 
 			var upstream *mocks.Handler
@@ -282,11 +359,16 @@ func TestServer_Handle(t *testing.T) {
 				routeMatcher,
 				authorizer,
 				authenticator,
-				models.ServerConfig{})
+				tt.config)
 
 			tt.expectations(request, routeMatcher, authenticator, authorizer)
 
 			if tt.wantUpstreamCalled {
+				if upstream == nil {
+					t.Errorf("invalid test case: proxy must be enabled to be called")
+					return
+				}
+
 				upstream.On("ServeHTTP", responseWriter, request).Return()
 			} else {
 				responseWriter.On("WriteHeader", tt.wantStatusCode).Return()
@@ -302,7 +384,7 @@ func TestServer_Handle(t *testing.T) {
 				assert.Equal(t, "Bearer", header["Www-Authenticate"][0])
 			}
 
-			if tt.proxyEnabled {
+			if upstream != nil {
 				upstream.AssertExpectations(t)
 			}