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

Fixes #1973 - Adds normalization of metadata values. #1974

Merged
merged 3 commits into from
Dec 8, 2017

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Dec 7, 2017

Following discussions in #1971, the goal of this pull request is to sanitize a bit. This is not perfect but should remove some of the issue. It goes on the side of safe play. For example if it detects there is > or < it just sends back an empty string instead of trying to clean up.

@@ -154,6 +159,22 @@ def normalize_url(url):
return url


def normalize_metadata(metadata_value):
"""Normalize the metadata received from the form."""
# Removing closing comments.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@karlcow
Copy link
Member Author

karlcow commented Dec 7, 2017

→ nosetests --nocapture -v tests/test_form.py
Statuses Initialization…
Fetching milestones from Github…
Milestones saved in data/
Milestones in memory
The data body sent to GitHub API. ... ok
Check that domain name is extracted. ... ok
Checks we return the right form with the appropriate data. ... ok
HTML comments need the right values depending on the keys. ... ok
Check that metadata is processed and wrapped. ... ok
Avoid some type of strings. ... ok
Check that URL is normalized. ... ok
Check that appropriate radio button label is returned. ... ok

----------------------------------------------------------------------
Ran 8 tests in 0.007s

OK

and the full test suite is still running.

nosetests
........................................................................
----------------------------------------------------------------------
Ran 72 tests in 2.468s

OK

I didn't run the functional tests though. Maybe to double check before.

Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

Looks good, just one question about normalize_metadata

('blue sky ', 'blue sky'),
('bad_bird <script>', ''),
('bad_bird <script-->>', ''),
('a' * 300, ''),

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

return None
if '-->' in metadata_value:
metadata_value = metadata_value.replace('-->', '')
metadata_value = normalize_metadata(metadata_value)

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

metadata_value = metadata_value.replace('-->', '')
metadata_value = normalize_metadata(metadata_value)
# Let's avoid html tags in
if ('<' or '>') in metadata_value and '-->' not in metadata_value:

This comment was marked as abuse.

This comment was marked as abuse.

# Let's avoid html tags in
if ('<' or '>') in metadata_value and '-->' not in metadata_value:
metadata_value = ''
if len(metadata_value) > 200:

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr miketaylr merged commit 58d7215 into webcompat:master Dec 8, 2017
@miketaylr
Copy link
Member

Looks good, thanks @karlcow.

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.

3 participants