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 bug 889491 - Improve error messages for invalid queries #3359

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented May 27, 2016

Don't ask why I dedicated an hour of my Friday afternoon to this. The short answer was that I was just curious. I wanted to play with the handler400 but that's not at all what that's for. :)

Before:
screen shot 2016-05-27 at 3 55 55 pm
screen shot 2016-05-27 at 3 56 15 pm
screen shot 2016-05-27 at 3 56 32 pm

After:
screen shot 2016-05-27 at 3 54 18 pm
screen shot 2016-05-27 at 3 54 45 pm
screen shot 2016-05-27 at 3 55 13 pm

</div>
<div class="body">
{% if 'ul class="errorlist"' in error %}
{{ error | safe }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is really safe? Could you add tests that verify it can't lead to an XSS attack for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't think of a way to test that. So I hijacked one of the form validation functions and put in raise forms.ValidationError('<script> ...
This is what it looked like;
http://jmp.sh/CRDYEwV

@adngdb
Copy link
Contributor

adngdb commented Jun 2, 2016

I see you applied this new behavior to a handful of views. Is there not a way to apply to every view we have? Any 400 page should behave like that imo, and having to add another decorator to every view sounds painful.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 6, 2016

Is there not a way to apply to every view we have?

Yes, mayhaps if we write a middleware. If we just make sure to not meddle if the request is an AJAX one it might just work.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 9, 2016

@adngdb See the new update. I managed to do it with a piece of middleware. Now, if any view does something like...

def myview(request):
    if condition:
        return http.HttpResponseBadRequest('Bad!')

it will be rendered as the 400.html template.

I've tested this...

  • on the report index page, by replacing one of the characters in the crash ID with an x
  • on the topcrashers page, by setting &days=xxx
  • on the signature report, removing the &signature=...
  • on the home page, by setting &days=xxx
  • on the supersearch page, by entering a date that isn't a date (Note! Here it should NOT use 400.html)

@adngdb
Copy link
Contributor

adngdb commented Jun 21, 2016

This looks good, but it needs to be rebased!

@peterbe peterbe force-pushed the bug-889491-improve-error-messages-for-invalid-queries branch from 01f3971 to 49987a2 Compare June 21, 2016 14:05
@adngdb adngdb merged commit 2b32f0b into mozilla-services:master Jun 21, 2016
@peterbe peterbe deleted the bug-889491-improve-error-messages-for-invalid-queries branch June 21, 2016 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants