Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature HistoryController add @Secured #5884

Closed
CherishCai opened this issue May 28, 2021 · 17 comments · Fixed by #6173
Closed

Feature HistoryController add @Secured #5884

CherishCai opened this issue May 28, 2021 · 17 comments · Fixed by #6173

Comments

@CherishCai
Copy link
Contributor

now, HistoryController has not @secured check permission, We can bypass the permission check by querying the historical configuration.
目前,历史配置接口没有@secured,我们可以通过查询历史配置绕过权限检验。

故需加上改作权限校验,由于管控台会传输三个关键参数,所以可添加上。

@KomachiSion
Copy link
Collaborator

Add secured I think ok. But why add parameter?

If you want to change them. please has more discuss in issue before you PR.

@CherishCai
Copy link
Contributor Author

CherishCai commented Jun 1, 2021

add parameter for compatible with ConfigResourceParser

public class ConfigResourceParser implements ResourceParser {
    
    private static final String AUTH_CONFIG_PREFIX = "config/";
    
    @Override
    public String parseName(Object request) {
        HttpServletRequest req = (HttpServletRequest) request;
        String namespaceId = NamespaceUtil.processNamespaceParameter(req.getParameter("tenant"));
        String groupName = req.getParameter("group");
        String dataId = req.getParameter("dataId");
        
        StringBuilder sb = new StringBuilder();
        
        if (StringUtils.isNotBlank(namespaceId)) {
            sb.append(namespaceId);
        }
        
        if (StringUtils.isBlank(groupName)) {
            sb.append(Resource.SPLITTER).append("*");
        } else {
            sb.append(Resource.SPLITTER).append(groupName);
        }
        
        if (StringUtils.isBlank(dataId)) {
            sb.append(Resource.SPLITTER).append(AUTH_CONFIG_PREFIX).append("*");
        } else {
            sb.append(Resource.SPLITTER).append(AUTH_CONFIG_PREFIX).append(dataId);
        }
        
        return sb.toString();
    }
}

@CherishCai
Copy link
Contributor Author

CherishCai commented Jun 1, 2021

目前管控台历史版本的查询会带上这三个关键参数,故可添加上。且该强依赖 参数校验,不知各位认为如何
The current of the HistoryController of the control console will bring these three key parameters, so they can be added.

@CherishCai
Copy link
Contributor Author

但是还有另一个问题,nid 为用户碰撞输入的,目前也没判断其与 dataId group 的关系。


But there is another problem. The nid is input by the user collision, and the relationship with the dataId group has not been judged at present.

@KomachiSion
Copy link
Collaborator

@TsingLiang Will add parameters have some effect in config module?

@KomachiSion
Copy link
Collaborator

但是还有另一个问题,nid 为用户碰撞输入的,目前也没判断其与 dataId group 的关系。

But there is another problem. The nid is input by the user collision, and the relationship with the dataId group has not been judged at present.

But I have a confusion, The method has no need dataId and groupId, if you add parameters but no used. It will make other confused why add these parameters.

The ConfigResourceParser is get dataId and group from request not method definition.

@CherishCai
Copy link
Contributor Author

但是还有另一个问题,nid 为用户碰撞输入的,目前也没判断其与 dataId group 的关系。
But there is another problem. The nid is input by the user collision, and the relationship with the dataId group has not been judged at present.

But I have a confusion, The method has no need dataId and groupId, if you add parameters but no used. It will make other confused why add these parameters.

The ConfigResourceParser is get dataId and group from request not method definition.

Yeah, Perhaps the following methods can increase parameters or judgments to ensure Secured.

persistService.detailConfigHistory(nid);
persistService.detailPreviousConfigHistory(id);
// to
persistService.detailConfigHistory(nid,dataId,group,namespace);
persistService.detailPreviousConfigHistory(id,dataId,group,namespace);

@KomachiSion
Copy link
Collaborator

Why.I can't get the changes reason, the controller parameters has no used. And for use them, add usage in persistService? I am more confused.

@CherishCai
Copy link
Contributor Author

Why.I can't get the changes reason, the controller parameters has no used. And for use them, add usage in persistService? I am more confused.

嗯,其实权限校验的漏洞是存在的,得看社区的同学们讨论是否有更好的方案修修;目前多余参数我是为了兼容 ConfigResourceParser 加上的,暂时定制的能解决,更好更优方案看大家讨论啦。

@KomachiSion
Copy link
Collaborator

I think add this parameter and use it to check whether history config is belonged to inputed dataId will be better. Which will make resource parser can get dataId and group, and also can avoid user input invalid dataId and group.

@CherishCai
Copy link
Contributor Author

I think add this parameter and use it to check whether history config is belonged to inputed dataId will be better. Which will make resource parser can get dataId and group, and also can avoid user input invalid dataId and group.

So, this is what I suggested before. like this

persistService.detailConfigHistory(nid);
persistService.detailPreviousConfigHistory(id);
// to
persistService.detailConfigHistory(nid,dataId,group,namespace);
persistService.detailPreviousConfigHistory(id,dataId,group,namespace);

CherishCai added a commit to CherishCai/nacos that referenced this issue Jun 15, 2021
@KomachiSion
Copy link
Collaborator

I think has no need change in SQL level. Only in add compare in memory is enough.

@CherishCai
Copy link
Contributor Author

compare in memory , return null if not equals. Doesn't look elegant.

@KomachiSion
Copy link
Collaborator

KomachiSion commented Jun 21, 2021

not return null, throw an exception and return an error response to describe the nid is not belong this dataid and group.

If you add in SQL, you only get 0 rows, and do not know whether has this nid.

@CherishCai
Copy link
Contributor Author

Emm... 改SQL确实不恰当,我暂时不作提交哈,发挥了提个鉴权遗漏的作用就好,bingo ~~~

@KomachiSion
Copy link
Collaborator

Those who are interested can try to add this judgment logic, that uses the dataid and group of the input parameters to verify the historical changes that are queried.

@brotherlu-xcq
Copy link
Collaborator

I will continue the left work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants