server: Fix patch path escaping

In the Data API, the `path` attribute of `PATCH` operations was
not handling escaped `~` and `/` values (escaped as `~0` and `~1`,
respectively) according to the JSON-Pointer RFC.

The API was using (and continues to support) URL escaping of these
characters (so `%7E` and `%2F`, respectively), but this was not
documented and is not strictly correct according to the documentation.
However, this is being left unchanged to preserve backwards compatibility
for anyone who stumbled across this behaviour.

With this change, `~0` and `~1` in the `path` attribute of `PATCH`
operations will be unescaped into `~` and `/` as defined in the RFC.

Signed-off-by: Geoff Baskwill <me@geoffbaskwill.ca>

server/server.go

@@ -1841,7 +1841,7 @@ func (s *Server) prepareV1PatchSlice(root string, ops []types.PatchV1) (result [
 		}
 
 		var ok bool
-		impl.path, ok = storage.ParsePathEscaped(path)
+		impl.path, ok = parsePatchPathEscaped(path)
 		if !ok {
 			return nil, types.BadPatchPathErr(op.Path)
 		}
@@ -1859,6 +1859,39 @@ func (s *Server) generateDecisionID() string {
 	return ""
 }
 
+// parsePatchPathEscaped returns a new path for the given escaped str.
+// This is based on storage.ParsePathEscaped so will do URL unescaping of
+// the provided str for backwards compatibility, but also handles the
+// specific escape strings defined in RFC 6901 (JSON Pointer) because
+// that's what's mandated by RFC 6902 (JSON Patch).
+func parsePatchPathEscaped(str string) (path storage.Path, ok bool) {
+	path, ok = storage.ParsePathEscaped(str)
+	if !ok {
+		return
+	}
+	for i := range path {
+		// RFC 6902 section 4: "[The "path" member's] value is a string containing
+		// a JSON-Pointer value [RFC6901] that references a location within the
+		// target document (the "target location") where the operation is performed."
+		//
+		// RFC 6901 section 3: "Because the characters '~' (%x7E) and '/' (%x2F)
+		// have special meanings in JSON Pointer, '~' needs to be encoded as '~0'
+		// and '/' needs to be encoded as '~1' when these characters appear in a
+		// reference token."
+
+		// RFC 6901 section 4: "Evaluation of each reference token begins by
+		// decoding any escaped character sequence.  This is performed by first
+		// transforming any occurrence of the sequence '~1' to '/', and then
+		// transforming any occurrence of the sequence '~0' to '~'.  By performing
+		// the substitutions in this order, an implementation avoids the error of
+		// turning '~01' first into '~1' and then into '/', which would be
+		// incorrect (the string '~01' correctly becomes '~1' after transformation)."
+		path[i] = strings.Replace(path[i], "~1", "/", -1)
+		path[i] = strings.Replace(path[i], "~0", "~", -1)
+	}
+	return
+}
+
 func stringPathToDataRef(s string) (r ast.Ref) {
 	result := ast.Ref{ast.DefaultRootDocument}
 	result = append(result, stringPathToRef(s)...)

server/server_test.go

@@ -789,6 +789,85 @@ func TestDataPutV1IfNoneMatch(t *testing.T) {
 	}
 }
 
+func TestParsePatchPathEscaped(t *testing.T) {
+	tests := []struct {
+		note         string
+		path         string
+		expectedPath storage.Path
+		expectedOK   bool
+	}{
+		// success-path tests
+		{
+			note:         "single-level",
+			path:         "/single-level",
+			expectedPath: storage.Path{"single-level"},
+			expectedOK:   true,
+		},
+		{
+			note:         "multi-level",
+			path:         "/a/multi-level/path",
+			expectedPath: storage.Path{"a", "multi-level", "path"},
+			expectedOK:   true,
+		},
+		{
+			note:         "end",
+			path:         "/-",
+			expectedPath: storage.Path{"-"},
+			expectedOK:   true,
+		},
+		{ // not strictly correct but included for backwards compatibility with existing OPA
+			note:         "url-escaped forward slash",
+			path:         "/github.com%2Fopen-policy-agent",
+			expectedPath: storage.Path{"github.com/open-policy-agent"},
+			expectedOK:   true,
+		},
+		{
+			note:         "json-pointer-escaped forward slash",
+			path:         "/github.com~1open-policy-agent",
+			expectedPath: storage.Path{"github.com/open-policy-agent"},
+			expectedOK:   true,
+		},
+		{
+			note:         "json-pointer-escaped tilde",
+			path:         "/~0opa",
+			expectedPath: storage.Path{"~opa"},
+			expectedOK:   true,
+		},
+		{
+			note:         "json-pointer-escape correctness",
+			path:         "/~01",
+			expectedPath: storage.Path{"~1"},
+			expectedOK:   true,
+		},
+
+		// failure-path tests
+		{ // not possible with existing callers but for completeness...
+			note:       "empty string",
+			path:       "",
+			expectedOK: false,
+		},
+		{ // not possible with existing callers but for completeness...
+			note:       "string that doesn't start with /",
+			path:       "foo",
+			expectedOK: false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.note, func(t *testing.T) {
+			actualPath, actualOK := parsePatchPathEscaped(tc.path)
+
+			if tc.expectedOK != actualOK {
+				t.Fatalf("Expected ok to be %v but was %v", tc.expectedOK, actualOK)
+			}
+
+			if !reflect.DeepEqual(tc.expectedPath, actualPath) {
+				t.Fatalf("Expected path to be %v but was %v", tc.expectedPath, actualPath)
+			}
+		})
+	}
+}
+
 func TestBundleScope(t *testing.T) {
 
 	ctx := context.Background()