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

fixed 渗透测试问题:用户名密码枚举和SSRF #3251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qinde025
Copy link

Hi,
功能内部使用fastgpt做大模型平台,请渗透测试供应商做了渗透测试,发现几个问题,详情见附近。修复其中了2个问题,申请合并导致主分枝。

Fastgpt-渗透测试报告_20241121.docx

渗透测试结果:
本次测试的目的是从技术上度量被测试系统的安全性,监督检查系统是否存在安全漏洞,发现并修复系统的漏洞从而提高系统安全性。
测试结果如下:
高危问题:0
中危问题:3
未授权访问
用户名密码枚举
TOKEN无注销机制
低危问题:1
SSRF

Copy link

cla-assistant bot commented Nov 27, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


qinde seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
Copy link

cla-assistant bot commented Nov 27, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


qinde seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -90,3 +92,58 @@ export const addLog = {
});
}
};

/* add login logger */
export const addLoginLog = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有专门的 log 函数,不需要单独写

};

/* check15分钟内失败的登录记录,5条以上就锁定账号 */
export default async function isLoginLocked(userName: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不属于 log 的模块,这个应该属于,login 模块的代码,并且不会复用。只需要放在 login api 里即可

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们有专门的 TmpDataSchema 表用于存放临时数据,不需要单独建表

]
};
// 分页查询
const loginLogList = await getLoginLog()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以直接用 TmpDataSchema 表,设置 ttl 为 15 分钟过期。data 里记录登录次数即可。不需要写这么复杂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

登录错误,属于用户层错误,错误信息统一在 UserErrEnum 枚举中,且报错也在枚举中声明


//校验头像路径格式
if (avatar) {
var regex = /\.(png|jpe?g|gif|svg)(\?.*)?$/;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

此处应该封装一个通用的系统图片链接校验方法,通过校验后缀以及以及 baseurl 是否为系统预设前缀。
此外,非特殊情况,不要使用 var

var regex = /\.(png|jpe?g|gif|svg)(\?.*)?$/;
var result = regex.test(avatar);
if (!result) {
throw new Error('头像路径格式不正确');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

所有文案都需要经过 i18nT 加工,进行国际化翻译。

@c121914yu
Copy link
Collaborator

非常感谢你提出的问题,我们留意到这些问题,但是如果要合入主分支,我们对代码风格和质量有一定要求,需要进行一些优化。

@@ -73,6 +74,7 @@ export const useUserStore = create<State>()(
};
});
try {
user.avatar = user.avatar?.replace(imageBaseUrl, '');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

前端是不需要做任何操作,后端校验链接来源即可

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

Successfully merging this pull request may close these issues.

2 participants