白盒测试 一个简单的 if 判断可能导致潜在的业务不安全

iqianxing · 2018年08月09日 · 最后由 sindynasty 回复于 2018年08月11日 · 681 次阅读

下面是一个简单的if构成的程序片段:

obj = rpc.getObj();
if(obj != null && obj.isSuccess()==true){
    doTask1();
}

doOtherTasks();

上述代码存在下述问题:

  1. obj==null时,没有任何代码处理; 2.obj!=null && obj.isSuccess()==false时,没有代码处理;

如果在代码中看到类似的代码,请停下来思考一下代码逻辑是否影响到业务的安全性。举例说明潜在的风险:
rpc.getObj()是判断一个用户是否是黑名单用户,由于rpc方法在外部接口异常时返回null,可能导致在黑名单里面的用户做了其他事情(即调用了doOtherTasks()方法)。

如果觉得我的文章对您有用,请随意打赏。您的支持将鼓励我继续创作!
共收到 17 条回复 时间 点赞

那也是doothertask的问题吧 和if没什么关系吧

为啥要判断:

obj.isSuccess()==true

obj.isSuccess() 这个方法,本身返回的就是布尔类型吧

白盒分析的topic很少,楼主加油,继续分享

对java 代码不太熟悉, 不一定对:

按我的理解上面的代码等效于:
obj = rpc.getObj();
if(obj != null && obj.isSuccess()==true){
doTask1();
return;
}
else{
doOtherTasks();
}

如果这种情况, null 和 success 是 false 的情况不是都在 doOtherTasks 里处理了吗?

simonpatrick 回复

doothertask与if语句没有关系,但是由于没有考虑if语句没有对应的else处理其他异常,导致doothertask成功执行了。

部分接口从业务上来说应该是强依赖关系,一旦返回非预期结果应该中断流程或者抛出异常。

autotester1 回复

是这样的,写成这样是为了强调仅判断了返回结果success==true的情况

jerrylizilong 回复

描述有点问题,if语句中的return去掉。doTask1()与doothertask()是顺序执行的。

iqianxing 回复

坦白讲我觉得还是else里面要处理什么响应的验证也是在doothertask这个方法里面做 本质应该我觉得是doothertask这个业务校验不严格 以后如果其他地方调用可能会出现一样的问题

simonpatrick 回复

看到的代码类似这样:

obj1 = rpc1.getObj();
if(obj != null && obj.isSuccess()==true){
    doTask1();
}

obj2= rpc2.getObj();
if(obj2 != null && obj2.isSuccess()==true){
    doTask2();
}

obj3= rp3.getObj();
if(obj3!= null && obj3.isSuccess()==true){
    doTask3();
}

这样就了了……这是习惯不好,跟IF没啥关系
另外,非空判断尽量把null写左边吧,少一点npe的报错~这是写java的经验,其他语言就不清楚了~

obj = rpc.getObj();
if(null != obj && obj.isSuccess()){
    doTask1();
    return;
}

doOtherTasks();

我合作过的coder,最常犯的、最愚蠢的而且能反复错的问题:
1、循环外定义变量,内部不做初始化
2、不做NPE预处理,出了事就一脸懵逼反问测试会不会操作,换个数据给他立刻吃惊:还有这种骚操作?
3、catch exception之后不在finally里面做事务完整性保护习惯性在catch里面做
……

fudax 回复

赞,代码气味很好

iqianxing 回复

原来是被你的 return 给带歪了

jerrylizilong 回复

抱歉

一定要加return...

RealLau 回复

是常见的任务串行处理流程中常见的问题,if语句内部可能处理的正常逻辑

抛开业务讲代码安不安全?你们连需求都不知道,连doTask1和doOtherTasks的内容都不知道,谈代码安不安全有啥意义

需要 登录 后方可回复, 如果你还没有账号请点击这里 注册