[ISSUE #3621 Fix Passwords containing special characters are not supported (such as character "+") #3656

syapollo · 2020-08-21T01:15:28Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

for #3621

Brief changelog

encode the passowrd and password can not be null

Verifying this change

I simulated some special characters and they can pass normally

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

CLAassistant · 2020-08-21T01:15:32Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ syapollo
❌ 宋裕

宋裕 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.}

KomachiSion · 2020-08-21T02:41:26Z

client/src/main/java/com/alibaba/nacos/client/security/SecurityProxy.java

@@ -41,45 +43,45 @@
 * @since 1.2.0
 */
 public class SecurityProxy {
-    
+


Do not change the indent .
Please use nacos code style to reformat code.

detail see soruce code style/CodeStyle.md

sorry
I have modified and re-uploaded

KomachiSion · 2020-08-21T02:43:21Z

@syapollo Please check your commit author is your github id.

Otherwise the CLA can't pass.

You can search how to change the username and email of git by search engine.

syapollo · 2020-08-21T06:13:45Z

@syapollo Please check your commit author is your github id.

Otherwise the CLA will can't pass.

You can search how to change the username and email of git by search engine.

I send mail to opensource@alibaba-inc.com today

KomachiSion · 2020-08-21T09:20:48Z

@syapollo Please check your commit author is your github id.
Otherwise the CLA will can't pass.
You can search how to change the username and email of git by search engine.

I send mail to opensource@alibaba-inc.com today

No need to send email.

I found you submit commit with user 宋裕 and your github id is syapollo
So github can't link this user rightly.

I suggest that you change the git username and email. Then re-commit your change with syapollo. In this case, you can click CLA link Contributor License Agreement and sign CLA directly.

KomachiSion

the CLA of @syapollo is ok. The next think need to do is to remove the old commit of 宋裕.

Thanks.

KomachiSion · 2020-08-21T09:59:56Z

client/src/main/java/com/alibaba/nacos/client/security/SecurityProxy.java


-        if (StringUtils.isNotBlank(username)) {
+        if (StringUtils.isNotBlank(username) && StringUtils.isNotBlank(password)) {


What does it do when password = ""?

If my password is empty string, the params will be skipped

For security reasons, I think there is a username parameter in the actual scenario, so there should be no empty password
In another case, if there is no username parameter, there is no need to configure a password

···
public boolean login(String server) throws UnsupportedEncodingException {
if (StringUtils.isNotBlank(username) && StringUtils.isNotBlank(password)) {
Map<String, String> params = new HashMap<String, String>(2);
Map<String, String> bodyMap = new HashMap<String, String>(2);
params.put("username", username);
bodyMap.put("password", URLEncoder.encode(password, "utf-8"));
String url = "http://" + server + contextPath + LOGIN_URL;
if (server.contains(Constants.HTTP_PREFIX)) {
url = server + contextPath + LOGIN_URL;
}
try {
HttpRestResult restResult = nacosRestTemplate
.postForm(url, Header.EMPTY, Query.newInstance().initParams(params), bodyMap, String.class);
if (!restResult.ok()) {
SECURITY_LOGGER.error("login failed: {}", JacksonUtils.toJson(restResult));
return false;
}
JsonNode obj = JacksonUtils.toObj(restResult.getData());
if (obj.has(Constants.ACCESS_TOKEN)) {
accessToken = obj.get(Constants.ACCESS_TOKEN).asText();
tokenTtl = obj.get(Constants.TOKEN_TTL).asInt();
tokenRefreshWindow = tokenTtl / 10;
}
} catch (Exception e) {
SECURITY_LOGGER.error("[SecurityProxy] login http request failed"
+ " url: {}, params: {}, bodyMap: {}, errorMsg: {}", url, params, bodyMap, e.getMessage());
return false;
}
}
return true;
}
···

Why does this function return true at the end, is this a bug?

I actually tested it. Without configuring the user password parameter, I skipped the authentication, but succeeded.

syapollo · 2020-08-21T10:05:16Z

@syapollo Please check your commit author is your github id.
Otherwise the CLA will can't pass.
You can search how to change the username and email of git by search engine.

I send mail to opensource@alibaba-inc.com today

No need to send email.

I found you submit commit with user 宋裕 and your github id is syapollo
So github can't link this user rightly.

I suggest that you change the git username and email. Then re-commit your change with syapollo. In this case, you can click CLA link Contributor License Agreement and sign CLA directly.

Thank you for your patience, is it all right now

KomachiSion · 2020-08-21T10:22:26Z

@syapollo The syapollo user's CLA has been ok. And the implementation

@syapollo Please check your commit author is your github id.
Otherwise the CLA will can't pass.
You can search how to change the username and email of git by search engine.

I send mail to opensource@alibaba-inc.com today

No need to send email.
I found you submit commit with user 宋裕 and your github id is syapollo
So github can't link this user rightly.
I suggest that you change the git username and email. Then re-commit your change with syapollo. In this case, you can click CLA link Contributor License Agreement and sign CLA directly.

Thank you for your patience, is it all right now

Not yet.

✅ syapollo
❌ 宋裕

The next thing need to do is to remove the old commit of 宋裕.

And have a look the newest review comment.

KomachiSion · 2020-08-21T10:25:09Z

@syapollo I have other suggestions . You can close this PR and re-checkout one branch from develop branch. And do these change in new branch.

Then commit with syapollo and submit by new PR.

syapollo · 2020-08-24T01:20:11Z

@syapollo I have other suggestions . You can close this PR and re-checkout one branch from develop branch. And do these change in new branch.

Then commit with syapollo and submit by new PR.

for alibaba#3621

bdb159c

KomachiSion requested changes Aug 21, 2020

View reviewed changes

for alibaba#3621

21c5b51

for alibaba#3621

fdbe80c

KomachiSion requested changes Aug 21, 2020

View reviewed changes

syapollo closed this Aug 24, 2020

syapollo deleted the develop-issue#3621 branch August 24, 2020 01:22

snyk-bot mentioned this pull request Aug 19, 2021

[Snyk] Security upgrade webpack-dev-server from 3.11.2 to 4.0.0 snyk123/nacos#27

Open

This was referenced Sep 12, 2021

[Snyk] Security upgrade webpack-dev-server from 3.11.2 to 4.0.0 snyk123/nacos#29

Open

[Snyk] Fix for 1 vulnerabilities snyk123/nacos#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #3621 Fix Passwords containing special characters are not supported (such as character "+") #3656

[ISSUE #3621 Fix Passwords containing special characters are not supported (such as character "+") #3656

syapollo commented Aug 21, 2020

CLAassistant commented Aug 21, 2020 •

edited

Loading

KomachiSion Aug 21, 2020

syapollo Aug 21, 2020

KomachiSion commented Aug 21, 2020 •

edited

Loading

syapollo commented Aug 21, 2020

KomachiSion commented Aug 21, 2020

KomachiSion left a comment •

edited

Loading

KomachiSion Aug 21, 2020

syapollo Aug 21, 2020

syapollo Aug 21, 2020 •

edited

Loading

syapollo commented Aug 21, 2020

KomachiSion commented Aug 21, 2020 •

edited

Loading

KomachiSion commented Aug 21, 2020

syapollo commented Aug 24, 2020


		if (StringUtils.isNotBlank(username)) {
		if (StringUtils.isNotBlank(username) && StringUtils.isNotBlank(password)) {

[ISSUE #3621 Fix Passwords containing special characters are not supported (such as character "+") #3656

[ISSUE #3621 Fix Passwords containing special characters are not supported (such as character "+") #3656

Conversation

syapollo commented Aug 21, 2020

What is the purpose of the change

Brief changelog

Verifying this change

CLAassistant commented Aug 21, 2020 • edited Loading

KomachiSion Aug 21, 2020

Choose a reason for hiding this comment

syapollo Aug 21, 2020

Choose a reason for hiding this comment

KomachiSion commented Aug 21, 2020 • edited Loading

syapollo commented Aug 21, 2020

KomachiSion commented Aug 21, 2020

KomachiSion left a comment • edited Loading

Choose a reason for hiding this comment

KomachiSion Aug 21, 2020

Choose a reason for hiding this comment

syapollo Aug 21, 2020

Choose a reason for hiding this comment

syapollo Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

syapollo commented Aug 21, 2020

KomachiSion commented Aug 21, 2020 • edited Loading

KomachiSion commented Aug 21, 2020

syapollo commented Aug 24, 2020

CLAassistant commented Aug 21, 2020 •

edited

Loading

KomachiSion commented Aug 21, 2020 •

edited

Loading

KomachiSion left a comment •

edited

Loading

syapollo Aug 21, 2020 •

edited

Loading

KomachiSion commented Aug 21, 2020 •

edited

Loading