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

Fix XSS on a search feature #422

Merged
merged 3 commits into from Jul 26, 2019

Conversation

@ikhoon
Copy link
Contributor

commented Jul 23, 2019

Motivation:
XSS issue was reported by Line Security Team

Modifications:

  • Encode html on error notification message.
  • Invalid search query returns Bad Request instead of Internal Server Error.
  • Replace jGitRepository into parent.name() + '/' + name
    to prevent exposure of a full file system path on the error message.

Result:
Enhanced security

@ikhoon ikhoon requested review from minwoox and trustin as code owners Jul 23, 2019

@ikhoon ikhoon added the defect label Jul 23, 2019

@ikhoon ikhoon added this to the 0.40.2 milestone Jul 23, 2019

Fix XSS on a search feature
Motivation:
XSS issue is reported by Line Security Team

Modification:
* Sanitize html on error notification message.
* Invalid search query returns `Bad Request` instead of Internal Server Error.
* Replace `jGitRepository` into `parent.name() + '/' + name`
  to prevent exposure of a full file system path on the error message.

Result:
Enhanced security

@ikhoon ikhoon force-pushed the ikhoon:xss branch from 7846985 to 4b0cebb Jul 23, 2019

@stypr

This comment has been minimized.

Copy link

commented Jul 23, 2019

I think ngSanitize is good enough as a quick fix.
But as I mentioned earlier, It would be better off to encode the text from the notification message itself.

This patch can still load tags like image, etc., but seems to filter out harmful attributes well. Also, it blocks /, therefore not able to load external domain

For example: https://centraldogma/#/projects/a'%22%3E%3Cimg%20src=1%3E

@ikhoon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@stypr The notification UI library we are using is able to embed HTML. I just tried to remove harm tag from HTML on error display to keep the flexibility of HTML.
But I agree that sanitized HTML is still dangerous. I will fix it to encode all the notification text.

@stypr

This comment has been minimized.

Copy link

commented Jul 23, 2019

Code seems good to go. Built and tested it too.
Test Code seems to work fine now. malicious tag is not rendered anymore.

@codecov

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

Merging #422 into master will increase coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #422      +/-   ##
============================================
+ Coverage     69.23%   69.29%   +0.06%     
- Complexity     2975     2978       +3     
============================================
  Files           319      319              
  Lines         12204    12206       +2     
  Branches       1300     1300              
============================================
+ Hits           8449     8458       +9     
+ Misses         2936     2926      -10     
- Partials        819      822       +3
Impacted Files Coverage Δ Complexity Δ
...internal/storage/repository/git/GitRepository.java 80.79% <0%> (+0.31%) 147 <0> (+1) ⬆️
...internal/replication/ZooKeeperCommandExecutor.java 78.23% <0%> (-0.49%) 60% <0%> (ø)
...rp/centraldogma/server/metadata/MigrationUtil.java 78.62% <0%> (+2.29%) 28% <0%> (+1%) ⬆️
...rage/repository/git/CacheableCompareTreesCall.java 51.51% <0%> (+3.03%) 6% <0%> (+1%) ⬆️
...server/internal/thrift/CentralDogmaExceptions.java 63.63% <0%> (+13.63%) 2% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d650417...7d35e01. Read the comment docs.

@ikhoon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@stypr Thank for fast reviewing and feedback.

As we have talked in slack chat, unfortunately, we don’t have test codes and suits for frontend yet. Thanks for the manual test.

@stypr

This comment has been minimized.

Copy link

commented Jul 24, 2019

As we have talked in slack chat, unfortunately, we don’t have test codes and suits for frontend yet. Thanks for the manual test.

For now, I think it should be ok to approach with the current process. we can decide later about making test cases for frontend side.

@ikhoon ikhoon force-pushed the ikhoon:xss branch 2 times, most recently from d35bb79 to db45de1 Jul 25, 2019

@trustin
Copy link
Member

left a comment

Thanks, @ikhoon !

@ikhoon ikhoon force-pushed the ikhoon:xss branch 2 times, most recently from 9f011ca to 2f57250 Jul 25, 2019

@ikhoon ikhoon force-pushed the ikhoon:xss branch from 2f57250 to 7d35e01 Jul 26, 2019

@minwoox
Copy link
Member

left a comment

Thanks, @ikhoon!

@trustin trustin merged commit 25fcbbd into line:master Jul 26, 2019

4 of 5 checks passed

codecov/patch 0% of diff hit (target 69.23%)
Details
Travis CI - Pull Request Build Passed
Details
codecov/project 69.29% (+0.06%) compared to d650417
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.