[BB-6041] Convert password compliance warning to html - #30130
Conversation
openedx-webhooks
added
needs triage
open-source-contribution
|
Thanks for the pull request, @pkulkark! I've created OSPR-6546 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
DubeySandeep
left a comment
DubeySandeep
left a comment
There was a problem hiding this comment.
@pkulkark Thanks for the PR, I took a quick pass on the code changes and left a comment/question, PTAL!
| except password_policy_compliance.NonCompliantPasswordException as e: | ||
| # Prevent the login attempt. | ||
| raise ValidationError(str(e)) # lint-amnesty, pylint: disable=raise-missing-from | ||
| raise ValidationError(HTML(str(e))) # lint-amnesty, pylint: disable=raise-missing-from |
There was a problem hiding this comment.
(not sure!) Instead of converting the string to HTML should just use the e which is already an HTML object? (ref)
Also, it makes sense for the NonCompliantPasswordWarning exception to handle the format/structure of the message.
There was a problem hiding this comment.
@DubeySandeep The str was added as part of a XSS security fix (as far as I can remember), so I'm hesitant to remove it. Keeping it would add an extra layer of security and doesn't really affect the performance much.
There was a problem hiding this comment.
I see, but earlier it was str(HTML(str)) and now it's HTML(str(HTML(str))), I think this will again raise the XSS issue (in case it existed before)?
There was a problem hiding this comment.
The existing str(HTML) should take care of escaping problematic HTML tags. So it won't be converted back next time. The same has been done at https://github.com/openedx/edx-platform/blob/557f58c341710d2d03c2a491e6141276d15823b1/openedx/core/djangoapps/user_authn/views/login.py#L200.
|
Hi there @natabene, can you assign this to me? I can't modify the OSPR Jira dashboard. |
DubeySandeep
left a comment
DubeySandeep
left a comment
There was a problem hiding this comment.
The code changes in the PR LGTM!
|
@pkulkark Thank you for your contribution. @mariajgrimaldi Thanks for volunteering! @wajeeha-khalid Do you mind if @mariajgrimaldi reviews and merges this? |
|
sorry for delay; I had been out vacationing and saw this only now. @waheedahmed can you please review this to confirm this aligns with our understanding of compliance and security for interjection of tags. |
…onvert-warning-back-to-html
|
@mariajgrimaldi We got green light from the owning team, so this is all yours now. |
…onvert-warning-back-to-html
|
Thanks @mariajgrimaldi. I've rebased to latest master. |
|
@pkulkark 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
8 participants
Description
This PR converts the password compliance warning message back to HTML. It was converted to string as part of additional security but that resulted in HTML attributes not being applied while displaying the warning message.
Jira tickets: BB-6041
Screenshots:
Before fix:
After fix:
Testing instructions
Reviewers:
TBD