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

[14.0][FIX] document_page: Stored XSS #426

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

SergiCForgeFlow
Copy link

Currently Knowledge pages are vulnerable to stored XSS attacks. This is because the 'content' field is type Text, so it doesn't receive the default sanitize provided by Html fields.

This PR solves this, changing 'content' field to type Html.

@SergiCForgeFlow SergiCForgeFlow changed the title [FIX] document_page: Stored XSS [14.0][FIX] document_page: Stored XSS Aug 24, 2023
@SergiCForgeFlow SergiCForgeFlow force-pushed the 14.0-fix-document_page-xss_stored branch 5 times, most recently from b700fb0 to 90c66bc Compare August 24, 2023 08:17
@max3903 max3903 added this to the 14.0 milestone Aug 24, 2023
@max3903 max3903 self-assigned this Aug 24, 2023
@SergiCForgeFlow SergiCForgeFlow force-pushed the 14.0-fix-document_page-xss_stored branch from e567de9 to e3757f5 Compare August 24, 2023 15:06
Copy link

@OriolMForgeFlow OriolMForgeFlow 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
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

All existing contents that are not HTML will show incorrectly after this change. You need to incorporate a migration script that converts them to HTML.

@PabloEForgeFlow
Copy link

It might be worth adding a test which verifies that the content is indeed sanitized after writing the record.

@SergiCForgeFlow SergiCForgeFlow force-pushed the 14.0-fix-document_page-xss_stored branch from e3757f5 to 4073b53 Compare August 28, 2023 09:46
@SergiCForgeFlow
Copy link
Author

All existing contents that are not HTML will show incorrectly after this change. You need to incorporate a migration script that converts them to HTML.

All existing contents currently are stored in a HTML format. Regardless of whether the Field is type Text, the editor takes the input, generates and stores the data in a HTML format.

unitary tests added

post-migration script added
@SergiCForgeFlow SergiCForgeFlow force-pushed the 14.0-fix-document_page-xss_stored branch from 4073b53 to eda3c2a Compare August 28, 2023 11:04
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-426-by-pedrobaeza-bump-nobump, awaiting test results.

@SergiCForgeFlow
Copy link
Author

All existing contents that are not HTML will show incorrectly after this change. You need to incorporate a migration script that converts them to HTML.

All existing contents currently are stored in a HTML format. Regardless of whether the Field is type Text, the editor takes the input, generates and stores the data in a HTML format.

Field 'content' on 'document.page' model is compute. So when Field Type is changed to HTML this receives default sanitize. This data is computed from 'content' field on model 'document.page.history', which is stored. If some 'content' data was malicious the malicious script is still stored on the database on model 'document.page.history'. That's why I have added post-migration script on the recent commit. Just by reassigning the content data, now this field also receives default HTML sanitize and the script is completely erased from database.

@OCA-git-bot OCA-git-bot merged commit 32b84c4 into OCA:14.0 Aug 28, 2023
4 checks passed
@OCA-git-bot
Copy link
Contributor

@pedrobaeza The merge process could not be finalized, because command git push origin 14.0-ocabot-merge-pr-426-by-pedrobaeza-bump-nobump:14.0 failed with output:

To https://github.com/OCA/knowledge
 ! [rejected]          14.0-ocabot-merge-pr-426-by-pedrobaeza-bump-nobump -> 14.0 (fetch first)
error: failed to push some refs to 'https://***@github.com/OCA/knowledge'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 002e1b8. Thanks a lot for contributing to OCA. ❤️

@LoisRForgeFlow LoisRForgeFlow deleted the 14.0-fix-document_page-xss_stored branch August 29, 2023 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants