654887 - Internal Server Error/HTTP 500 on various /shipping/dashboard? parameters

Status: VERIFIED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

I found the following using PowerFuzzer:

500 HTTP Error code with Vulnerable URL: https://l10n-stage-sj.mozilla.org/shipping/dashboard?av=http%3A%2F%2Fwww.google.com%2F

500 HTTP Error code with Vulnerable URLhttps://l10n-stage-sj.mozilla.org/shipping/dashboard?av=a%3Benv

500 HTTP Error code with Vulnerable URL: https://l10n-stage-sj.mozilla.org/shipping/dashboard?av=%BF%27%22%28

500 HTTP Error code with Vulnerable URL: https://l10n-stage-sj.mozilla.org/shipping/dashboard?av=<script>var+pf_68747470733a2f2f6c31306e2d73746167652d736a2e6d6f7a696c6c612e6f72672f7368697070696e672f64617368626f617264_6176=new+Boolean();</script>

Comment 1

[Axel Hecht [:Pike]]

good news is, we're not vulnerable.

bad news is, we're not showing good errors on malicious input. maybe use a get_or_404 instead of a plain get. Basically, there's no handling of user error in https://github.com/mozilla/elmo/blob/master/shipping/views/__init__.py#L318 and friends at all.

Comment 2

[Peter Bengtsson [:peterbe]]

(In reply to comment #1)
> good news is, we're not vulnerable.
> 
> bad news is, we're not showing good errors on malicious input. maybe use a
> get_or_404 instead of a plain get. Basically, there's no handling of user error
> in https://github.com/mozilla/elmo/blob/master/shipping/views/__init__.py#L318
> and friends at all.

Working on a patch for this.

Comment 3

[Peter Bengtsson [:peterbe]]

Attachment: Raises 404 not found instead of 500 errors on junk parameters

This patch is also a gentle start to client integration tests for shipping.

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build

Improved patch as shipping can and should now also be tested by hudson.

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 530326 [details] [diff] [review]
Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build

Review of attachment 530326 [details] [diff] [review]:

With the nits on the tests, r=me.

::: apps/shipping/tests.py
@@ +14,5 @@
+# The Original Code is l10n django site.
+#
+# The Initial Developer of the Original Code is
+# Mozilla Foundation.
+# Portions created by the Initial Developer are Copyright (C) 2010

Nit, 2011?

@@ +17,5 @@
+# Mozilla Foundation.
+# Portions created by the Initial Developer are Copyright (C) 2010
+# the Initial Developer. All Rights Reserved.
+#
+# Contributor(s):

You should probably add yourself as contributor here. We've not been great at doing this in other parts, but we should get better as long as we still have MPL 1.1 license headers. Something like
#  Peter Bengtsson <[e-mail]>

@@ +52,5 @@
+        )
+        l10n = Forest.objects.create(
+          name='firefox',
+          url='http://firefox.com',
+        )

The way that name and URL tie together in real life is something like

http://hg.mozilla.org/l10n-central/ and l10n-central.

I.e., the name is the path without leading and trailing slashes. I'd rather have the fixture stick to that convention, just to be on the safe side.

@@ +73,5 @@
+
+        return appver, milestone
+
+    def testDashboardBadURLs(self):
+        """test based on https://bugzilla.mozilla.org/show_bug.cgi?id=654887"""

That doc string (and the method name) look like you've done this first, and then aaaaalllll the others?

Make that consistent in naming? And I don't think that referring to the bug is helpful for this test, I'd drop that.

Comment 7

[Peter Bengtsson [:peterbe]]

Attachment: Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build

The reason for the naming convention on the test methods was because I wanted to have the same name (almost) as the view functions. E.g. test_dashboard_bad_urls instead of testDashboardBadURLS for the 'dashboard' function.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 530364 [details] [diff] [review]
Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build

r=me.

Comment 9

[Peter Bengtsson [:peterbe]]

Landed on origin/develop
https://github.com/mozilla/elmo/commit/03781f233cee6e97f48440fc6563b45f0d3d8840

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Can I get a push to test the fixes?

Comment 11

[Axel Hecht [:Pike]]

https://l10n-stage-sj.mozilla.org/test/shipping/dashboard?av=freck is an example.

https://l10n-stage-sj.mozilla.org/test/ in general has the state of develop.

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Thanks, Peter + Axel.

Verified FIXED.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Full list of fuzzed URLs