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

Create rule S6781: JWT secret keys should not be disclosed #3838

Merged
merged 5 commits into from
Apr 3, 2024

Conversation

github-actions[bot]
Copy link
Contributor

You can preview this rule here (updated a few minutes after each push).

Review

A dedicated reviewer checked the rule description successfully for:

  • logical errors and incorrect information
  • information gaps and missing content
  • text style and tone
  • PR summary and labels follow the guidelines

@sebastien-andrivet-sonarsource sebastien-andrivet-sonarsource changed the title Create rule S6781 Create rule S6781: JWT secret keys should not be disclosed Mar 28, 2024
Copy link
Contributor

@loris-s-sonarsource loris-s-sonarsource left a comment

Choose a reason for hiding this comment

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

Text LGTM
Some tweak suggestions to the code to improve the diff view

Comment on lines 12 to 13
[Route("login-config")]
public class LoginConfigController : ControllerBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Route("login-config")]
public class LoginConfigController : ControllerBase
[Route("login-example")]
public class LoginExampleController : ControllerBase

public class LoginConfigController : ControllerBase
{
private readonly IConfiguration _config;
public LoginConfigController(IConfiguration config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public LoginConfigController(IConfiguration config)
public LoginExampleController(IConfiguration config)

expires: DateTime.Now.AddMinutes(120),
signingCredentials: credentials);

return Ok(new JwtSecurityTokenHandler().WriteToken(Sectoken));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Ok(new JwtSecurityTokenHandler().WriteToken(Sectoken));
var token = new JwtSecurityTokenHandler().WriteToken(Sectoken);
return Ok(token);


// Code to validate user omitted

var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant (key in appsettings.json)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant (key in appsettings.json)
var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant

No need to explain it twice 👍 It also increases the size of the code and unnecessarily forces to scroll right to read

{
// Code to validate user omitted

var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant: hard-coded key in code
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant: hard-coded key in code
var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)); // Noncompliant

This increases the size of the code and unnecessarily forces to scroll right to read

Comment on lines 74 to 76
var token = new JwtSecurityTokenHandler().WriteToken(Sectoken);

return Ok(token);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var token = new JwtSecurityTokenHandler().WriteToken(Sectoken);
return Ok(token);
var token = new JwtSecurityTokenHandler().WriteToken(Sectoken);
return Ok(token);


=== Code examples

The following noncompliant code contains a hard-coded secret that can be exposed unintentionally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following noncompliant code contains a hard-coded secret that can be exposed unintentionally.
The following noncompliant code uses a secret from the `_config` variable, which means a secret is hardcoded in `appsettings.json` and can be unintentionally exposed along with the code.

I suggest adding the appsettings comment here:

Comment on lines 48 to 49
[Route("login-env")]
public class LoginEnvController : ControllerBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Route("login-env")]
public class LoginEnvController : ControllerBase
[Route("login-example")]
public class LoginExampleController : ControllerBase

public class LoginEnvController : ControllerBase
{
private readonly IConfiguration _config;
public LoginEnvController(IConfiguration config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public LoginEnvController(IConfiguration config)
public LoginExampleController(IConfiguration config)

Copy link
Contributor

@loris-s-sonarsource loris-s-sonarsource left a comment

Choose a reason for hiding this comment

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

Text LGTM, code logic LGTM, it's just the diff appearance that should be fixed

rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-framework.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-framework.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
@loris-s-sonarsource loris-s-sonarsource marked this pull request as ready for review April 3, 2024 12:42
Copy link
Contributor

@loris-s-sonarsource loris-s-sonarsource left a comment

Choose a reason for hiding this comment

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

last review, some mistakes from me

rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-core.adoc Outdated Show resolved Hide resolved
rules/S6781/csharp/how-to-fix/net-framework.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@loris-s-sonarsource loris-s-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarqube-next bot commented Apr 3, 2024

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link

sonarqube-next bot commented Apr 3, 2024

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@jamie-anderson-sonarsource jamie-anderson-sonarsource merged commit 5f72418 into master Apr 3, 2024
11 of 12 checks passed
@jamie-anderson-sonarsource jamie-anderson-sonarsource deleted the rule/S6781-add-csharp branch April 3, 2024 16:33
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.

3 participants