关于v2接口中间件强行要求验证sign字段问题
dxkrs opened this issue · 8 comments
运行环境
php:7.4
wechatpay-php:1.4.2
描述你的问题现象
在使用此组件对接了大部分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
添加这个 need_vefify
其实等于留了个后门
,可能会被滥用到所有v2请求接口都被添加上,建议通过请求的 requestTarget
做内置白名单,符合白名单的URL自动忽略验签。
例如:
<?php
$securityUrls = [
'mmpaymkttransfers/promotion/transfers',
];
if (in_array($request->getRequestTarget(), $securityUrls)) {
return $response;
}
另外,对于响应明确无须验签的,测试用例其实是给了例子,是可以通过请求时,传递特殊 handler
自动忽略验签,参考代码如下:
wechatpay-php/tests/OpenAPI/V2/Mmpaymkttransfers/Promotion/TransfersTest.php
Lines 135 to 140 in 3a64295
“设置的白名单”应该是对同一个Cilent实例的全局管理,如果给使用组件者管理,在使用中维护不善依然有“后门”风险。
目前从最新的官网文档公示来看,依然有不少接口请求、查询的接口是不需要验证sign,何况可能存在一些旧项目依然使用已经无公示的v2接口(并且能正常调用),这么看的话把白名单写进组件里也不太现实。
通过传递handler
来忽略验签确实是个好方法,也符合guzzlehttp的使用,但对组件使用者来说未免有点复杂了,而且传递特殊handler
也是可以在工厂方法构造的时候传递,与“设置的白名单”有类似的“后门”风险。
既要安全作为首要考虑,又要简便、灵活使用,我新增了一个提交:把这个不验证标识改为只能在request
的时候传递,才有效,在初始化Cilent
实例(即工厂方法构造)中传递无效。
试试这个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));
同意 @TheNorthMemory 的观点,不应该提供给 SDK 使用方主动选择不验签的能力,从根本上避免安全隐患。例如,SDK 使用方因为验签不通过,干脆关闭了之。
目标应该是让 SDK 使用方不关注某个接口是否要验签,如何验签这些“琐碎”的事情,而集中精力放在业务逻辑上。所以 SDK 内置不验签接口清单的模式更合适。
我也赞同 @xy-peng 的说法,应该是要以安全为主,但如果真的要“从根本上避免安全隐患”,可能还需要禁止SDK使用方定制验签方式
,例如在post()
,postAsync()
传递handler
参数,或者是readme上面的定制节点介绍的方法,这些方法无疑是稍微复杂地“提供给 SDK 使用方主动选择不验签的能力”。
@TheNorthMemory 这个diff,由官方来维护v2接口不验签的接口,当然是最好,也很安全,期待能发布更新,但记得在工作备忘录中标记一下,以后有相关业务接口更新,也要及时发布此组件的一个新版本。
无sign返回值,总感觉是不负责行为,即使是走https安全通道,也存在中间人攻击等行为,对v2接口做些维护工作,把这些“历史遗留缺sign问题”给补上,才是最好的方案。
无sign返回值,总感觉是不负责行为,即使是走https安全通道,也存在中间人攻击等行为,对v2接口做些维护工作,把这些“历史遗留缺sign问题”给补上,才是最好的方案。
最好的方案实际上可能困难重重。毕竟API是难以变更的,特别对于v2,增加参数对于一些实现比较极端的商户也是 BREAKING CHANGE。
比较现实的方案是:以白名单的方式不验签,并且不提供外部关闭验签的能力。