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

Add more validation to destructive actions (CSRF + additional password checking) #796

Merged
merged 10 commits into from
Jul 17, 2021

Conversation

Glandos
Copy link
Member

@Glandos Glandos commented Jul 12, 2021

This also switches all such actions to POST requests.

Remaining topics:

  • Use form.errors instead of translated strings
  • Project deletion
  • Project history deletion

@Glandos
Copy link
Member Author

Glandos commented Jul 12, 2021

I was surprised by the fact that on authentication form, the CSRF error message is displayed directly from Flask_WTF, but this is always in English. In fact, there is an issue on that: pallets-eco/flask-wtf#401
The main issue with providing our own string is that we lose some information, since Flask_WTF is using different strings for different cases. And this error should never happen in a normal environment, so displaying it in English is not a real issue.
On the other hand, it seems better to have a translated application.

What do you think?

@zorun zorun added this to the v5 milestone Jul 14, 2021
@zorun
Copy link
Collaborator

zorun commented Jul 14, 2021

I have reworked project deletion to have its own form and to ask for the private code:

2021-07-14-161729_1245x817_scrot

@zorun
Copy link
Collaborator

zorun commented Jul 14, 2021

Same thing for the "Clear project history" modal, but I need help from someone who knows how to talk to bootstrap:

2021-07-14-161829_520x343_scrot

@zorun
Copy link
Collaborator

zorun commented Jul 14, 2021

I was surprised by the fact that on authentication form, the CSRF error message is displayed directly from Flask_WTF, but this is always in English. In fact, there is an issue on that: wtforms/flask-wtf#401
The main issue with providing our own string is that we lose some information, since Flask_WTF is using different strings for different cases. And this error should never happen in a normal environment, so displaying it in English is not a real issue.
On the other hand, it seems better to have a translated application.

What do you think?

Yes, I was also surprised about the built-in CSRF error messages.

I think it's fine to handle errors through flash messages, it shouldn't happen in normal cases (and practically speaking it's hard to handle this as form errors...)

Let me know what you think of the new code.

@zorun zorun changed the title Add CSRF validation to disruptive actions Add more validation to destructive actions (CSRF + additional password checking) Jul 14, 2021
@zorun zorun marked this pull request as ready for review July 14, 2021 22:11
Copy link
Member Author

@Glandos Glandos left a comment

Choose a reason for hiding this comment

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

For the edit project form, I changed the interaction:

  • Button is normal at first, and changed to danger when clicked. Otherwise, it is hard to notice a change with the mouse over it.
  • If the button lose focus, confirmation state is lost.
  • Attach the event at document load, and not on each form click.

ihatemoney/templates/edit_project.html Outdated Show resolved Hide resolved
ihatemoney/templates/forms.html Outdated Show resolved Hide resolved
Copy link
Member Author

@Glandos Glandos left a comment

Choose a reason for hiding this comment

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

Forms are already translating interesting strings, so I'd like to use them instead of flashing a new message.
For this, we need a function (in utils.py?) like this:

def flash_form_errors(form, title):
    if len(form.errors) == 0:
        return None
    elif len(form.errors) == 1:
        # I18N: Form error with only one error
        return _("{title}: {description}").format(title=title, error=form.errors.popitem()[1][0])
    else:
        error_list = "</li><li>".join(str(error) for (field, errors) in form.errors.items() for error in errors)
        errors = f"<ul><li>{error_list}</li></ul>"
        # I18N: Form error with a list of errors
        return Markup(_("{title}<br />{errors}").format(title=title, errors=errors))

And it can be used:

        flash(
            flash_form_errors(form, _("Error deleting project history")),
            category="danger",
        )

Then all strings from validators are displayed as-is, potentially in a list, if there more than one of them. I voluntarily removed the field name, otherwise the flashing message would have been too long.

ihatemoney/web.py Outdated Show resolved Hide resolved
# Used for CSRF validation
form = EmptyForm()
if not form.validate():
flash(_("CSRF Token: The CSRF token is invalid."), category="danger")
Copy link
Member Author

Choose a reason for hiding this comment

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

Use flash_form_errors

ihatemoney/web.py Outdated Show resolved Hide resolved
# Used for CSRF validation
form = EmptyForm()
if not form.validate():
flash(_("CSRF Token: The CSRF token is invalid."), category="danger")
Copy link
Member Author

Choose a reason for hiding this comment

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

Use flash_form_errors

ihatemoney/web.py Show resolved Hide resolved
ihatemoney/web.py Show resolved Hide resolved
@Glandos
Copy link
Member Author

Glandos commented Jul 15, 2021

I'll gladly provide my help for the form, but this will for another evening.

Baptiste Jonglez and others added 10 commits July 17, 2021 13:44
This also switches all such actions to POST requests.

Deleting the project is handled in another commit because it requires more
changes.
It requires reworking the user interface, but it's probably for the best.
This is the same idea as deleting a project: deleting history is also a
major destructive action.  We reuse the same form as for project deletion
to ask for the private code and provide CSRF validation.
* add the event listener only once, instead of every time the form is clicked

* use a standard button by default, so that the second state with a
  "danger" button is more visible

* reset confirmation button to original state when losing focus

Co-authored-by: Glandos <[email protected]>
This is nice because we can reuse the translated strings of form error
messages in another context.

Suggested by Glandos.
@zorun
Copy link
Collaborator

zorun commented Jul 17, 2021

Many thanks for the review! I have integrated all your suggestions, and I figured out what was wrong with the form layout.

@zorun zorun merged commit 82f3f06 into spiral-project:master Jul 17, 2021
@zorun zorun mentioned this pull request Sep 7, 2021
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.

2 participants