Repository: oauth2-proxy/oauth2-proxy
Pull request #1921 (2022-12-06 15:04:28)

Bugfix/check json path
## Description

Add a small check for jsonpath before splitting and interpreting a claim string as jsonpath.
This is improving the jsonpath handling for claims and also will fix the usage of custom claims in webdomain format like mentioned here: https://github.com/oauth2-proxy/oauth2-proxy/issues/1730

## Motivation and Context

Not every string with a period is a valid jsonpath and thus should not interpreted as such one.

## How Has This Been Tested?

Manually tested

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG. -> no
- [x] I have updated the documentation/CHANGELOG accordingly. -> no
- [x] I have created a feature (non-master) branch for my PR. -> yes


eloo: Please check my comment here
https://github.com/oauth2-proxy/oauth2-proxy/issues/1730#issuecomment-1364344348
github-actions[bot]: This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.
eloo-abi: not stale
eloo-abi: @JoelSpeed can may have a look here?
JoelSpeed: Please add a changelog entry for this and update against the latest master branch. I think in general the change should be ok given the existing library should already take into account paths within the JSON structure.

I'd like to see some tests added for cases that pass with this new logic that do not pass for the old logic before we merge please
eloo-abi: @JoelSpeed test added and changelog is updated
mruoss: @JoelSpeed any chance to get this reviewed/merged? Thanks a lot.
tuunit: @JoelSpeed Rechecked the code and tests. LGTM. Can be merged.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3a6c17e5..681cfe59 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,7 @@
 - [#1988](https://github.com/oauth2-proxy/oauth2-proxy/pull/1988) Ensure sign-in page background is uniform throughout the page
 - [#2013](https://github.com/oauth2-proxy/oauth2-proxy/pull/2013) Upgrade alpine to version 3.17.2 and library dependencies (@miguelborges99)
 - [#2047](https://github.com/oauth2-proxy/oauth2-proxy/pull/2047) CVE-2022-41717: DoS in Go net/http may lead to DoS (@miguelborges99)
+- [#1921](https://github.com/oauth2-proxy/oauth2-proxy/pull/1921) Check jsonpath syntax before interpretation
 
 # V7.4.0
 
diff --git a/go.mod b/go.mod
index 2954b2b5..485911f3 100644
--- a/go.mod
+++ b/go.mod
@@ -19,6 +19,7 @@ require (
 	github.com/mitchellh/mapstructure v1.1.2
 	github.com/oauth2-proxy/mockoidc v0.0.0-20220221072942-e3afe97dec43
 	github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404
+	github.com/ohler55/ojg v1.14.5
 	github.com/onsi/ginkgo v1.16.5
 	github.com/onsi/gomega v1.27.6
 	github.com/pierrec/lz4/v4 v4.1.17
diff --git a/go.sum b/go.sum
index 46b8bfc5..48b052e5 100644
--- a/go.sum
+++ b/go.sum
@@ -269,6 +269,8 @@ github.com/oauth2-proxy/mockoidc v0.0.0-20220221072942-e3afe97dec43 h1:V9YiO92tY
 github.com/oauth2-proxy/mockoidc v0.0.0-20220221072942-e3afe97dec43/go.mod h1:rW25Kyd08Wdn3UVn0YBsDTSvReu0jqpmJKzxITPSjks=
 github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404 h1:ZpzR4Ou1nhldBG/vEzauoqyaUlofaUcLkv1C/gBK8ls=
 github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404/go.mod h1:YpORG8zs14vNlpXvuHYnnDvWazIRaDk02MaY8lafqdI=
+github.com/ohler55/ojg v1.14.5 h1:xCX2oyh/ZaoesbLH6fwVHStSJpk4o4eJs8ttXutzdg0=
+github.com/ohler55/ojg v1.14.5/go.mod h1:7Ghirupn8NC8hSSDpI0gcjorPxj+vSVIONDWfliHR1k=
 github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
 github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk=
diff --git a/pkg/providers/util/claim_extractor.go b/pkg/providers/util/claim_extractor.go
index c2bd3d1b..969fe097 100644
--- a/pkg/providers/util/claim_extractor.go
+++ b/pkg/providers/util/claim_extractor.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/bitly/go-simplejson"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests"
+	"github.com/ohler55/ojg/jp"
 	"github.com/spf13/cast"
 )
 
@@ -139,9 +140,13 @@ func parseJWT(p string) ([]byte, error) {
 }
 
 // getClaimFrom gets a claim from a Json object.
-// It can accept either a single claim name or a json path.
+// It can accept either a single claim name or a json path if the path is a valid json path.
 // Paths with indexes are not supported.
 func getClaimFrom(claim string, src *simplejson.Json) interface{} {
+	_, err := jp.ParseString(claim)
+	if err != nil {
+		return src.Get(claim).Interface()
+	}
 	claimParts := strings.Split(claim, ".")
 	return src.GetPath(claimParts...).Interface()
 }


Test patch:
diff --git a/pkg/providers/util/claim_extractor_test.go b/pkg/providers/util/claim_extractor_test.go
index 50275438..e1a416d6 100644
--- a/pkg/providers/util/claim_extractor_test.go
+++ b/pkg/providers/util/claim_extractor_test.go
@@ -25,6 +25,10 @@ const (
       "groups": [
         "idTokenGroup1",
         "idTokenGroup2"
+      ],
+      "https://groups.test": [
+        "fqdnGroup1",
+        "fqdnGroup2"
       ]
     }`
 	basicProfileURLPayload = `{
@@ -224,6 +228,18 @@ var _ = Describe("Claim Extractor Suite", func() {
 				expectedValue: "nestedUser",
 				expectedError: nil,
 			}),
+			Entry("retrieves claim for with FQDN", getClaimTableInput{
+				testClaimExtractorOpts: testClaimExtractorOpts{
+					idTokenPayload:        basicIDTokenPayload,
+					setProfileURL:         true,
+					profileRequestHeaders: newAuthorizedHeader(),
+					profileRequestHandler: shouldNotBeRequestedProfileHandler,
+				},
+				claim:         "https://groups.test",
+				expectExists:  true,
+				expectedValue: []interface{}{"fqdnGroup1", "fqdnGroup2"},
+				expectedError: nil,
+			}),
 		)
 	})