Closed dxkrs closed 2 years ago
TheNorthMemory
commented
2 years ago 添加这个 need_vefify 其实等于留了个后门，可能会被滥用到所有v2请求接口都被添加上，建议通过请求的 requestTarget 做内置白名单，符合白名单的URL自动忽略验签。
例如:
<?php
$securityUrls = [
'mmpaymkttransfers/promotion/transfers',
];
if (in_array($request->getRequestTarget(), $securityUrls)) {
return $response;
}另外，对于响应明确无须验签的，测试用例其实是给了例子，是可以通过请求时，传递特殊 handler 自动忽略验签，参考代码如下：
dxkrs
commented
2 years ago “设置的白名单”应该是对同一个Cilent实例的全局管理，如果给使用组件者管理，在使用中维护不善依然有“后门”风险。
目前从最新的官网文档公示来看，依然有不少接口请求、查询的接口是不需要验证sign，何况可能存在一些旧项目依然使用已经无公示的v2接口（并且能正常调用），这么看的话把白名单写进组件里也不太现实。
通过传递handler来忽略验签确实是个好方法，也符合guzzlehttp的使用，但对组件使用者来说未免有点复杂了，而且传递特殊handler也是可以在工厂方法构造的时候传递，与“设置的白名单”有类似的“后门”风险。
既要安全作为首要考虑，又要简便、灵活使用，我新增了一个提交：把这个不验证标识改为只能在request的时候传递，才有效，在初始化Cilent实例（即工厂方法构造）中传递无效。
TheNorthMemory
commented
2 years ago 试试这个diff，已知不需要验签的，无须请求(request)增加参数。
diff --git a/src/ClientXmlTrait.php b/src/ClientXmlTrait.php
index de02ef6..ed7c4c5 100644
--- a/src/ClientXmlTrait.php
+++ b/src/ClientXmlTrait.php
@@ -5,6 +5,7 @@ namespace WeChatPay;
use function strlen;
use function trigger_error;
use function sprintf;
+use function in_array;
use const E_USER_DEPRECATED;
@@ -30,6 +31,32 @@ trait ClientXmlTrait
'Content-Type' => 'text/xml; charset=utf-8',
];
+ /**
+ * @var string[] - Special URLs whose were designed that none signature respond.
+ */
+ protected static $noneSignatureRespond = [
+ '/mchrisk/querymchrisk',
+ '/mchrisk/setmchriskcallback',
+ '/mchrisk/syncmchriskresult',
+ '/mmpaymkttransfers/gethbinfo',
+ '/mmpaymkttransfers/gettransferinfo',
+ '/mmpaymkttransfers/promotion/paywwsptrans2pocket',
+ '/mmpaymkttransfers/promotion/querywwsptrans2pocket',
+ '/mmpaymkttransfers/promotion/transfers',
+ '/mmpaymkttransfers/query_bank',
+ '/mmpaymkttransfers/sendgroupredpack',
+ '/mmpaymkttransfers/sendminiprogramhb',
+ '/mmpaymkttransfers/sendredpack',
+ '/pay/downloadbill',
+ '/pay/downloadfundflow',
+ '/payitil/report',
+ '/risk/getpublickey',
+ '/risk/getviolation',
+ '/sandboxnew/pay/getsignkey',
+ '/secapi/mch/submchmanage',
+ '/xdc/apiv2getsignkey/sign/getsignkey',
+ ];
+
abstract protected static function body(MessageInterface $message): string;
abstract protected static function withDefaults(array ...$config): array;
@@ -88,6 +115,10 @@ trait ClientXmlTrait
{
return static function (callable $handler) use ($secret): callable {
return static function (RequestInterface $request, array $options = []) use ($secret, $handler): PromiseInterface {
+ if (in_array($request->getRequestTarget(), static::$noneSignatureRespond)) {
+ return $handler($request, $options);
+ }
+
return $handler($request, $options)->then(static function(ResponseInterface $response) use ($secret) {
$result = Transformer::toArray(static::body($response));
xy-peng
commented
2 years ago 同意 @TheNorthMemory 的观点，不应该提供给 SDK 使用方主动选择不验签的能力，从根本上避免安全隐患。例如，SDK 使用方因为验签不通过，干脆关闭了之。
目标应该是让 SDK 使用方不关注某个接口是否要验签，如何验签这些“琐碎”的事情，而集中精力放在业务逻辑上。所以 SDK 内置不验签接口清单的模式更合适。
dxkrs
commented
2 years ago 我也赞同 @xy-peng 的说法，应该是要以安全为主，但如果真的要“从根本上避免安全隐患”，可能还需要禁止SDK使用方定制验签方式，例如在post(),postAsync()传递handler参数，或者是readme上面的定制节点介绍的方法，这些方法无疑是稍微复杂地“提供给 SDK 使用方主动选择不验签的能力”。
@TheNorthMemory 这个diff，由官方来维护v2接口不验签的接口，当然是最好，也很安全，期待能发布更新，但记得在工作备忘录中标记一下，以后有相关业务接口更新，也要及时发布此组件的一个新版本。
TheNorthMemory
commented
2 years ago 无sign返回值，总感觉是不负责行为，即使是走https安全通道，也存在中间人攻击等行为，对v2接口做些维护工作，把这些“历史遗留缺sign问题”给补上，才是最好的方案。
xy-peng
commented
2 years ago 无sign返回值，总感觉是不负责行为，即使是走https安全通道，也存在中间人攻击等行为，对v2接口做些维护工作，把这些“历史遗留缺sign问题”给补上，才是最好的方案。
最好的方案实际上可能困难重重。毕竟API是难以变更的，特别对于v2，增加参数对于一些实现比较极端的商户也是 BREAKING CHANGE。
比较现实的方案是：以白名单的方式不验签，并且不提供外部关闭验签的能力。
运行环境
描述你的问题现象
在使用此组件对接了大部分v3接口后，有些业务依然需要对接v2接口，比如“付款到零钱”接口。（https://pay.weixin.qq.com/wiki/doc/api/tools/mch_pay.php?chapter=14_2）
这类接口的响应数据中，并没有sign字段，但ClientXmlTrait.php中设置了一个guzzlehttp的中间件方法static::transformResponse()，该方法必须验证body中的sign字段，当验证不通过或者字段不存在，此时会抛出一个RejectionException的异常。这导致了，即使接口返回业务正常响应（例如已经真实付款到用户零钱），依然抛这个错。目前唯一可以解决的方法，是在RejectionException的抛错中通过getReason()方法来找到返回业务正常的响应。
提交了一个修改建议
91