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

Webhook notification URL size validation check #3680

Merged

Conversation

@chirathr
Copy link
Contributor

@chirathr chirathr commented Feb 26, 2018

This PR is regarding issue #3669

I have fixed the issue with the form not validating the URL size and also added max_length in the form to prevent input from being larger than 600 characters.
I had to add conditions to check which form is submitted, as there are two forms in the template projects/project_notifications.html. Maybe we could change it to a single form or separate out the views that handle each POST request?

Please let me know if I made some mistakes.

if webhook_form.is_valid():
webhook_form.save()
if 'email' not in request.POST:
email_form = EmailHookForm(data=None, project=project)
Copy link
Member

@stsewd stsewd Feb 26, 2018

Why is data None here? and also the code above is doing the same (lines ~509 - 510)

Copy link
Contributor Author

@chirathr chirathr Feb 27, 2018

I am sorry @stsewd , I should have added a comment there. In the case that the URL is greater than max_length and email_form is empty the page should show only webhook_form.errors. If I don't reinitialize the email_form with None, the page shows errors for the email field along with the errors for the URL field.

image

A better way would be to handle each form separately, should I try to change the code handle both cases separately?

@chirathr
Copy link
Contributor Author

@chirathr chirathr commented Feb 27, 2018

I have updated the code to display validation error for URL length. Can someone review this pull request?

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Mar 2, 2018

Thanks, @chirathr! We'll get to it when we can; Read The Docs is mostly volunteer run. Feel free to ping again in a week if this hasn't been done yet.

@RichardLitt RichardLitt requested a review from humitos Mar 2, 2018

if request.method == 'POST':
if request.method == 'POST' and 'email' in request.POST:
email_form = EmailHookForm(data=request.POST or None, project=project)
Copy link
Member

@stsewd stsewd Mar 3, 2018

We don't need the or None part here, since we already checking if the method is POST

Copy link
Contributor Author

@chirathr chirathr Mar 4, 2018

Thank you, I will remove that :)

return HttpResponseRedirect(project_dashboard)
else:
# Blank email_form if webhook_form is submitted or the request is GET
email_form = EmailHookForm(data=None, project=project)
Copy link
Member

@stsewd stsewd Mar 3, 2018

I think this part isn't needed, would be better to use an elif here to check for request.method == 'POST' and 'url' in request.POST:

Copy link
Member

@stsewd stsewd Mar 3, 2018

looks like this is needed for the below request.

Copy link
Contributor Author

@chirathr chirathr Mar 4, 2018

Yup, it is required. @stsewd should I change it to use and elif like you said?

Copy link
Member

@stsewd stsewd Mar 4, 2018

Would be awesome to do something like that (for readability) but keeping the same logic. Anyway, is fine how it is now, I just didn't read the rest of the code when commenting that 😁.

@chirathr chirathr force-pushed the webhook_notifications_url_size branch from 33e03b3 to d9f4c17 Mar 6, 2018
readthedocs/projects/forms.py Outdated Show resolved Hide resolved
Copy link
Member

@humitos humitos left a comment

Thanks for your contribution.

I suggested a change and I also asked by not use a ModelForm which will handle the validation automatically.

I think that with those two changes we can merge it.

readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
@chirathr chirathr force-pushed the webhook_notifications_url_size branch 4 times, most recently from 92925af to 8550754 Sep 14, 2018
@chirathr
Copy link
Contributor Author

@chirathr chirathr commented Sep 14, 2018

Thank @humitos.

I have made the changes that were requested. Running makemigrations showed multiple changes in the projects app, so I omitted migrations from this commit.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 31, 2018

Testing this locally and it still doesn't properly do validation. It is saving the object in clean_url without checking the length, and allows me to save something larger than 600 in length. It also could use a test, to confirm the validation works (https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_project_forms.py#L1).

@ericholscher ericholscher force-pushed the webhook_notifications_url_size branch from 8550754 to 4b0ddf1 Nov 2, 2018
@codecov
Copy link

@codecov codecov bot commented Nov 2, 2018

Codecov Report

Merging #3680 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3680      +/-   ##
==========================================
+ Coverage   76.46%   76.47%   +<.01%     
==========================================
  Files         158      158              
  Lines        9997     9997              
  Branches     1262     1262              
==========================================
+ Hits         7644     7645       +1     
+ Misses       2011     2010       -1     
  Partials      342      342
Impacted Files Coverage Δ
readthedocs/projects/models.py 85.1% <100%> (ø) ⬆️
readthedocs/projects/forms.py 79.66% <100%> (+0.27%) ⬆️

@ericholscher ericholscher merged commit 3bfec17 into readthedocs:master Nov 6, 2018
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants