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

#23900 fixing ajax vulnerabilities reported by sonar #23911

Merged
merged 4 commits into from Jan 27, 2023

Conversation

fabrizzio-dotCMS
Copy link
Contributor

Proposed Changes

  • Ajax controllers need to check the command name against a white-list

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Unit Tests Report

1 418 tests  +6   1 407 ✔️ +6   3m 49s ⏱️ -5s
   140 suites +2        10 💤 ±0 
   140 files   +2          1 ±0 

For more details on these failures, see this check.

Results for commit d07c834. ± Comparison against base commit aa04d26.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Postman Tests Report

     54 files  ±0     737 suites  ±0   1h 30m 14s ⏱️ - 2m 5s
   363 tests ±0     361 ✔️ ±0  0 💤 ±0  2 ±0 
1 122 runs  ±0  1 113 ✔️ ±0  0 💤 ±0  9 ±0 

For more details on these failures, see this check.

Results for commit d07c834. ± Comparison against base commit aa04d26.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Integration Tests [postgres] Report

   425 files  ±0     425 suites  ±0   1h 10m 50s ⏱️ + 12m 18s
4 569 tests ±0  4 544 ✔️ +2  23 💤 ±0  2  - 2 
4 608 runs  ±0  4 583 ✔️ +2  23 💤 ±0  2  - 2 

For more details on these failures, see this check.

Results for commit d07c834. ± Comparison against base commit aa04d26.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Integration Tests [mssql] Report

   424 files  ±0     424 suites  ±0   2h 31m 48s ⏱️ - 5m 30s
4 564 tests ±0  4 523 ✔️  - 6  23 💤 ±0  18 +6 
4 603 runs  ±0  4 562 ✔️  - 6  23 💤 ±0  18 +6 

For more details on these failures, see this check.

Results for commit d07c834. ± Comparison against base commit aa04d26.

♻️ This comment has been updated with latest results.

@fabrizzio-dotCMS fabrizzio-dotCMS marked this pull request as draft January 26, 2023 18:06
@fabrizzio-dotCMS fabrizzio-dotCMS changed the base branch from release-23.02 to master January 26, 2023 18:08
@fabrizzio-dotCMS fabrizzio-dotCMS marked this pull request as ready for review January 26, 2023 18:09
@dotcms-sonarqube
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 5.6% 5.58% Duplicated Lines (%) on New Code (is greater than 3%)

See analysis details on SonarQube

@@ -266,4 +267,19 @@ private void remove () {

}

@Override
protected Set<String> getAllowedCommands() {
return Set.of(
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be a constant static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it as a class field. non-static but Mockito wouldnt see the results when calling the real method so ended up inlining the Set instantiation. It isn't a big penalti performance wise @jdotcms

* @throws NoSuchMethodException
*/
@VisibleForTesting
Method getMethod(String method, Class<?>... parameterTypes ) throws NoSuchMethodException {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a ReflectionUtils, may be better there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First this is a deprecated class Im not spending too much effort on it.. and the only reason I extracted that logic into a separate method was to verify that the method gets called using mokito @jdotcms

*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of( "action", "reorder", "delete", "add", "save", "deleteActionForStep" );
Copy link
Contributor

Choose a reason for hiding this comment

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

same constant

*/
@VisibleForTesting
Method getMethod(String method, Class<?>... parameterTypes ) throws NoSuchMethodException {
return this.getClass().getMethod(method, parameterTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems the method is repeat

@fmontes fmontes merged commit fb67760 into master Jan 27, 2023
@fmontes fmontes deleted the issue-23900-major-security-fixes branch January 27, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants