cloudstack icon indicating copy to clipboard operation
cloudstack copied to clipboard

Published 20 hours ago • verified publisher icon apache

Remove unnecessary escape method

Open BryanMLima opened this issue 2 years ago • 5 comments

Description

Currently, the API quotaEmailTemplateUpdate escapes JavaScript strings in the email templates for the subject and body fields. However, when using the API quotaEmailTemplateList the data returned were not unescaped, causing inconsistencies in the text. This PR aims to fix this behavior by removing the escape method in the API quotaEmailTemplateUpdate as it was unnecessary.

Types of changes

  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] Enhancement (improves an existing feature and functionality)
  • [ ] Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • [ ] Major
  • [ ] Minor

Bug Severity

  • [ ] BLOCKER
  • [ ] Critical
  • [ ] Major
  • [ ] Minor
  • [x] Trivial

Screenshots (if appropriate):

How Has This Been Tested?

This was tested in a local lab by changing the email templates in the subject and body field with special characters. After the emails arrived, the contents were correctly displayed.

BryanMLima avatar Aug 30 '22 15:08 BryanMLima

Codecov Report

Merging #6692 (00c8941) into main (2ca164a) will decrease coverage by 0.18%. The diff coverage is 91.30%.

:exclamation: Current head 00c8941 differs from pull request most recent head bf54baf. Consider uploading reports for the commit bf54baf to get more accurate results

@@             Coverage Diff              @@
##               main    #6692      +/-   ##
============================================
- Coverage     10.81%   10.62%   -0.19%     
+ Complexity     7083     6874     -209     
============================================
  Files          2485     2466      -19     
  Lines        245346   244570     -776     
  Branches      38313    38277      -36     
============================================
- Hits          26525    25994     -531     
+ Misses       215556   215293     -263     
- Partials       3265     3283      +18
Impacted Files Coverage Δ
...apache/cloudstack/quota/QuotaAlertManagerImpl.java 63.06% <90.47%> (+1.99%) :arrow_up:
...udstack/api/response/QuotaResponseBuilderImpl.java 25.52% <100.00%> (-8.31%) :arrow_down:
...in/java/org/apache/cloudstack/backup/BackupVO.java 7.50% <0.00%> (-32.50%) :arrow_down:
...dstack/network/contrail/model/ModelObjectBase.java 28.84% <0.00%> (-15.39%) :arrow_down:
.../apache/cloudstack/backup/VeeamBackupProvider.java 0.00% <0.00%> (-13.59%) :arrow_down:
...ema/src/main/java/com/cloud/vm/UserVmDetailVO.java 50.00% <0.00%> (-11.12%) :arrow_down:
...a/org/apache/cloudstack/quota/vo/QuotaUsageVO.java 42.37% <0.00%> (-10.17%) :arrow_down:
...g/apache/cloudstack/quota/constant/QuotaTypes.java 91.11% <0.00%> (-8.89%) :arrow_down:
.../org/apache/cloudstack/quota/vo/QuotaTariffVO.java 48.93% <0.00%> (-7.17%) :arrow_down:
... and 44 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Aug 30 '22 16:08 codecov[bot]

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Aug 30 '22 16:08 sonarqubecloud[bot]

@blueorangutan package

GutoVeronezi avatar Sep 09 '22 13:09 GutoVeronezi

@GutoVeronezi a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Sep 09 '22 13:09 blueorangutan

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4157

blueorangutan avatar Sep 09 '22 14:09 blueorangutan

@BryanMLima I haven't seen the code lately, one of the concerns could be somebody could set an browser based XSS code withing the email template @blueorangutan package

rohityadavcloud avatar Oct 08 '22 06:10 rohityadavcloud

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Oct 08 '22 06:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_check_mark: debian :heavy_multiplication_x: suse15. SL-JID 4384

blueorangutan avatar Oct 08 '22 09:10 blueorangutan

@blueorangutan package

DaanHoogland avatar Oct 11 '22 11:10 DaanHoogland

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Oct 11 '22 11:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 4422

blueorangutan avatar Oct 11 '22 13:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_check_mark: debian :heavy_multiplication_x: suse15. SL-JID 4427

blueorangutan avatar Oct 11 '22 15:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 4434

blueorangutan avatar Oct 12 '22 19:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 4446

blueorangutan avatar Oct 13 '22 07:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 4452

blueorangutan avatar Oct 13 '22 09:10 blueorangutan

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4457

blueorangutan avatar Oct 13 '22 12:10 blueorangutan

Found UI changes, kicking a new UI QA build @blueorangutan ui

acs-robot avatar Oct 13 '22 19:10 acs-robot

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan avatar Oct 13 '22 20:10 blueorangutan

UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6692 (SL-JID-2497)

blueorangutan avatar Oct 13 '22 20:10 blueorangutan

clgtm, I would like to know why these were added in the first place and if they got obsoleted for some reason or it was a bug to add them in the first place.

@DaanHoogland, sorry for the delay, the method did not make any sense as it was only for JS strings; it did not prevent any XSS attacks.

@BryanMLima I haven't seen the code lately, one of the concerns could be somebody could set an browser based XSS code withing the email template

As for the XSS attack @rohityadavcloud, I investigated this concern, and tested a lot of scenarios using OWASP Cheatsheet as reference. Displaying the contents of the email template on the UI with malicious code did not result in any problems, as it is inside a component. However, with further investigation, there is a lack of validation upon creating domains and accounts regarding their names. Creating a domain with the name <h1>test</h1> and using the ${domainName} inside the email template would result in the email client rendering that HTML tag. I tested with a simple script tag; however, most email clients already filter these tags, and therefore, it was totally omitted from the email.

In this case, to resolve this problem I escaped the possible HTML in the domain and account names variables to prevent any issues. Finally, it may be interesting to investigate the validation on account and domain names to block users from using any HTML tags and prevent any future problems.

BryanMLima avatar Oct 13 '22 20:10 BryanMLima

@blueorangutan test

DaanHoogland avatar Oct 14 '22 09:10 DaanHoogland

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan avatar Oct 14 '22 09:10 blueorangutan

Trillian Build Failed (tid-5129)

blueorangutan avatar Oct 14 '22 10:10 blueorangutan

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

github-actions[bot] avatar Oct 17 '22 08:10 github-actions[bot]

@BryanMLima can you look at the conflict and at the code smells:

SonarCloud Quality Gate failed. Quality Gate failed

Bug A 0 Bugs Vulnerability A 0 Vulnerabilities Security Hotspot A 0 Security Hotspots Code Smell B 7 Code Smells

73.3% 73.3% Coverage 0.0% 0.0% Duplication

DaanHoogland avatar Oct 17 '22 08:10 DaanHoogland

Trillian test result (tid-5147) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 40195 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6692-t5147-kvm-centos7.zip Smoke tests completed. 103 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_08_upgrade_kubernetes_ha_cluster Failure 569.16 test_kubernetes_clusters.py

blueorangutan avatar Oct 17 '22 19:10 blueorangutan

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

74.5% 74.5% Coverage
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Oct 17 '22 19:10 sonarqubecloud[bot]

@blueorangutan package

rohityadavcloud avatar Oct 19 '22 03:10 rohityadavcloud

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Oct 19 '22 03:10 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4497

blueorangutan avatar Oct 19 '22 05:10 blueorangutan