Repository: oauth2-proxy/oauth2-proxy
Pull request #1951 (2022-12-28 20:24:43)

Issue 1878: Validate URL call does not correctly honor already set UR…
Fix validate URL - append query string marker (?) or a separator (&) depending on URL having or not query parameters

## Description

If the validate URL already contains URL parameters like in

https://my.provider.de/2.0/users/me?with_permissions=true&with_roles=true

oauth2-proxy doesn't honor this circumstance and appends a new query string marker (?) instead of a separator (&) and
the URL becomes

https://my.provider.de/2.0/users/me?with_permissions=true&with_roles=true?access_token=eyJ0eXAiOiJKV1...

which leads to problems depending on the upstream server implementation.

## Motivation and Context

Fix issue [1878](https://github.com/oauth2-proxy/oauth2-proxy/issues/1878).

## How Has This Been Tested?

Needs to be tested

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 88e81b16..6fe5ec52 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,9 +14,9 @@
 - [#1906](https://github.com/oauth2-proxy/oauth2-proxy/pull/1906) Fix PKCE code verifier generation to never use UTF-8 characters
 - [#1839](https://github.com/oauth2-proxy/oauth2-proxy/pull/1839) Add readiness checks for deeper health checks (@kobim)
 - [#1927](https://github.com/oauth2-proxy/oauth2-proxy/pull/1927) Fix default scope settings for none oidc providers
+- [#1951](https://github.com/oauth2-proxy/oauth2-proxy/pull/1951) Fix validate URL, check if query string marker (?) or separator (&) needs to be appended (@miguelborges99)
 - [#1920](https://github.com/oauth2-proxy/oauth2-proxy/pull/1920) Make sure emailClaim is not overriden if userIDClaim is not set
 
-
 # V7.4.0
 
 ## Release Highlights
diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md
index e36544e0..ea398506 100644
--- a/docs/docs/configuration/overview.md
+++ b/docs/docs/configuration/overview.md
@@ -99,7 +99,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--cookie-csrf-per-request` | bool | Enable having different CSRF cookies per request, making it possible to have parallel requests. | false |
 | `--cookie-csrf-expire` | duration | expire timeframe for CSRF cookie | 15m |
 | `--custom-templates-dir` | string | path to custom html templates | |
-| `--custom-sign-in-logo` | string | path or a URL to an custom image for the sign_in page logo. Use \"-\" to disable default logo. |
+| `--custom-sign-in-logo` | string | path or a URL to an custom image for the sign_in page logo. Use `"-"` to disable default logo. |
 | `--display-htpasswd-form` | bool | display username / password login form if an htpasswd file is provided | true |
 | `--email-domain` | string \| list  | authenticate emails with the specified domain (may be given multiple times). Use `*` to authenticate any email | |
 | `--errors-to-info-log` | bool | redirects error-level logging to default log channel instead of stderr | |
diff --git a/providers/internal_util.go b/providers/internal_util.go
index 346ee80d..52cfd0a7 100644
--- a/providers/internal_util.go
+++ b/providers/internal_util.go
@@ -53,7 +53,11 @@ func validateToken(ctx context.Context, p Provider, accessToken string, header h
 	endpoint := p.Data().ValidateURL.String()
 	if len(header) == 0 {
 		params := url.Values{"access_token": {accessToken}}
-		endpoint = endpoint + "?" + params.Encode()
+		if hasQueryParams(endpoint) {
+			endpoint = endpoint + "&" + params.Encode()
+		} else {
+			endpoint = endpoint + "?" + params.Encode()
+		}
 	}
 
 	result := requests.New(endpoint).
@@ -74,3 +78,13 @@ func validateToken(ctx context.Context, p Provider, accessToken string, header h
 	logger.Errorf("token validation request failed: status %d - %s", result.StatusCode(), result.Body())
 	return false
 }
+
+// hasQueryParams check if URL has query parameters
+func hasQueryParams(endpoint string) bool {
+	endpointURL, err := url.Parse(endpoint)
+	if err != nil {
+		return false
+	}
+
+	return len(endpointURL.RawQuery) != 0
+}


Test patch:
diff --git a/providers/internal_util_test.go b/providers/internal_util_test.go
index 53070017..c8ab3f4f 100644
--- a/providers/internal_util_test.go
+++ b/providers/internal_util_test.go
@@ -132,6 +132,13 @@ func TestValidateSessionExpiredToken(t *testing.T) {
 	assert.Equal(t, false, validateToken(context.Background(), vtTest.provider, "foobar", nil))
 }
 
+func TestValidateSessionValidateURLWithQueryParams(t *testing.T) {
+	vtTest := NewValidateSessionTest()
+	defer vtTest.Close()
+	vtTest.provider.Data().ValidateURL, _ = url.Parse(vtTest.provider.Data().ValidateURL.String() + "?query_param1=true&query_param2=test")
+	assert.Equal(t, true, validateToken(context.Background(), vtTest.provider, "foobar", nil))
+}
+
 func TestStripTokenNotPresent(t *testing.T) {
 	test := "http://local.test/api/test?a=1&b=2"
 	assert.Equal(t, test, stripToken(test))