Baoyuantop commented on PR #13165: URL: https://github.com/apache/apisix/pull/13165#issuecomment-4560404893
Thanks for continuing to improve this DPoP plugin. The current revision does address the two earlier review points: `ssl_verify` is now configurable and propagated to outbound discovery/JWKS calls, and proof signature verification now covers the ES/RS/PS algorithm families with additional tests. However, I do not think this is mergeable yet. There are a few blocking issues that should be fixed first: 1. A malformed proof without `alg` can hit a Lua runtime error. `verify_dpop_proof_signature` eventually concatenates the nil `alg` value when no algorithm branch matches, and `validate_proof` also calls `h.alg:sub(...)` directly. This kind of input should consistently return `invalid_dpop_proof`, not fall into a 500/error path. Please add explicit header structure validation and negative tests for missing/non-string `alg`. 2. `require_nonce` is accepted by the schema, while the documentation says it is “not yet implemented”, and the implementation does not validate a DPoP nonce. A security option should not be accepted as a no-op. Please either implement the nonce semantics with tests, or remove the option for now. 3. With the default `strict_htu=false`, the plugin only compares the path and ignores scheme/host/port. This compatibility mode can weaken the URI binding semantics from RFC 9449. Please re-evaluate the default behavior, or at least document it clearly and add tests for strict/full-URL and path-only boundaries. Please address these points and request another review. -- 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]
