Copilot commented on code in PR #91:
URL:
https://github.com/apache/dubbo-go-pixiu-samples/pull/91#discussion_r2288369087
##########
shutdown/http2/test/pixiu_test.go:
##########
@@ -30,16 +30,14 @@ import (
_ "github.com/apache/dubbo-go-pixiu/pkg/pluginregistry"
"github.com/apache/dubbo-go-pixiu/pkg/server"
+ gproto "github.com/dubbo-go-pixiu/samples/grpc/deprecated/proto"
+
"github.com/stretchr/testify/assert"
Review Comment:
[nitpick] The import consolidation creates an empty line after the import
statement. Consider removing the extra blank line after the import for cleaner
formatting.
```suggestion
gproto "github.com/dubbo-go-pixiu/samples/grpc/deprecated/proto"
"github.com/stretchr/testify/assert"
```
##########
mcp/oauth/authserver/handlers_test.go:
##########
@@ -0,0 +1,282 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package main
+
+import (
+ "crypto/sha256"
+ "encoding/base64"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+ "time"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestValidatePKCE(t *testing.T) {
+ testCases := []struct {
+ name string
+ verifier string
+ challenge string
+ expected bool
+ }{
+ {
+ name: "Valid PKCE",
+ verifier: "test_verifier",
+ challenge: calculateS256Challenge("test_verifier"),
+ expected: true,
+ },
+ {
+ name: "Invalid PKCE",
+ verifier: "wrong_verifier",
+ challenge: calculateS256Challenge("test_verifier"),
+ expected: false,
+ },
+ {
+ name: "Empty Verifier",
+ verifier: "",
+ challenge: calculateS256Challenge("test_verifier"),
+ expected: false,
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ assert.Equal(t, tc.expected, validatePKCE(tc.challenge,
tc.verifier))
+ })
+ }
+}
+
+func TestGenerateRandomString(t *testing.T) {
+ // Test length
+ s32 := generateRandomString(32)
+ assert.Len(t, s32, 64) // 32 bytes = 64 hex characters
+
+ s16 := generateRandomString(16)
+ assert.Len(t, s16, 32) // 16 bytes = 32 hex characters
+
+ // Test for randomness (not a perfect test, but checks for non-empty
and different results)
+ s32_another := generateRandomString(32)
+ assert.NotEmpty(t, s32)
+ assert.NotEqual(t, s32, s32_another, "Two generated strings should not
be the same")
+}
+
+func TestHandleMetadata(t *testing.T) {
+ req := httptest.NewRequest(http.MethodGet,
"/.well-known/oauth-authorization-server", nil)
+ w := httptest.NewRecorder()
+
+ handleMetadata(w, req)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusOK, resp.StatusCode)
+ assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
+
+ var meta map[string]interface{}
+ err := json.NewDecoder(resp.Body).Decode(&meta)
+ require.NoError(t, err)
+
+ issuer := "http://localhost"; + listenAddr
Review Comment:
The issuer URL construction should be consistent with the hardcoded issuer
in jwt.go. Consider using a shared constant or configuration approach.
```suggestion
```
##########
mcp/oauth/authserver/jwt.go:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package main
+
+import (
+ "crypto"
+ "crypto/rand"
+ "crypto/rsa"
+ "crypto/sha256"
+ "encoding/base64"
+ "encoding/json"
+ "fmt"
+ "log"
+ "time"
+)
+
+const (
+ keyID = "demo-key-1"
+ tokenTTL = time.Hour
+)
+
+var (
+ // privKey is the RSA private key generated at startup.
+ privKey *rsa.PrivateKey
+)
+
+// initJWT generates an ephemeral RSA key for signing tokens.
+// In a production environment, keys should be loaded from a secure vault.
+func initJWT() {
+ var err error
+ privKey, err = rsa.GenerateKey(rand.Reader, 2048)
+ if err != nil {
+ log.Fatalf("failed to generate RSA key: %v", err)
+ }
+}
+
+// issueJWT creates a new JWT with the given audience and scope.
+func issueJWT(audience, scope string) (string, error) {
+ header := map[string]string{
+ "alg": "RS256",
+ "typ": "JWT",
+ "kid": keyID,
+ }
+ headerBytes, _ := json.Marshal(header)
+ headerEnc := base64.RawURLEncoding.EncodeToString(headerBytes)
+
+ claims := map[string]interface{}{
+ "iss": "http://localhost:9000";, // Hardcoded issuer
Review Comment:
The hardcoded issuer URL should be configurable or derived from the server's
actual address to make the authorization server more flexible.
```suggestion
"iss": issuerURL, // Configurable issuer
```
##########
mcp/oauth/authserver/util.go:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package main
+
+import (
+ "crypto/rand"
+ "encoding/hex"
+ "encoding/json"
+ "net/http"
+)
+
+// writeJSON is a helper to write JSON responses.
+func writeJSON(w http.ResponseWriter, status int, v any) {
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(status)
+ _ = json.NewEncoder(w).Encode(v)
+}
+
+// generateRandomString creates a secure random string of a given length.
+func generateRandomString(length int) string {
+ bytes := make([]byte, length)
+ if _, err := rand.Read(bytes); err != nil {
+ // In a real application, this should be handled more
gracefully.
+ panic(err)
+ }
+ return hex.EncodeToString(bytes)
Review Comment:
The panic on random generation failure should be replaced with proper error
handling and return an error to the caller instead of panicking.
```suggestion
func generateRandomString(length int) (string, error) {
bytes := make([]byte, length)
if _, err := rand.Read(bytes); err != nil {
// Return error to caller instead of panicking.
return "", err
}
return hex.EncodeToString(bytes), nil
```
##########
mcp/oauth/pixiu/conf.yaml:
##########
@@ -0,0 +1,204 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# MCP OAuth (Authorization) Sample Configuration
+# - Only protects the /mcp endpoint via MCP Authorization filter
+# - Uses a local authorization server at http://localhost:9000
+# - Uses remote JWKS served by the local authorization server
+
+static_resources:
+ listeners:
+ - name: "net/http"
+ protocol_type: "HTTP"
+ address:
+ socket_address:
+ address: "0.0.0.0"
+ port: 8888
+ filter_chains:
+ filters:
+ - name: "dgp.filter.httpconnectionmanager"
+ config:
+ route_config:
+ routes:
+ # Protected MCP endpoint
+ - match:
+ prefix: "/"
+ route:
+ cluster: "mcp-protected"
+ cluster_not_found_response_code: 505
+ http_filters:
+ - name: "dgp.filter.http.cors"
+ config:
+ allow_origin:
+ - "*"
+ allow_methods: "GET, POST, PUT, DELETE, OPTIONS"
+ allow_headers: "Content-Type, Authorization,
X-Requested-With"
+ max_age: "3600"
+ # MCP Authorization Filter (protect /mcp)
+ - name: "dgp.filter.http.auth.mcp"
+ config:
+ resource_metadata:
Review Comment:
[nitpick] Consider adding a comment explaining the purpose of the
resource_metadata path and how it relates to OAuth2 resource server discovery.
```suggestion
resource_metadata:
# The path below exposes resource metadata for OAuth2
resource server discovery.
# Authorization servers and clients use this endpoint
to obtain information about the protected resource.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
