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

VERIFIED FIXED in 1.2

Status

--
major
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: stephend, Assigned: peterbe)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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.
(Assignee)

Comment 2

8 years ago
(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.
(Assignee)

Updated

8 years ago
Assignee: nobody → peterbe
(Assignee)

Comment 3

8 years ago
Created attachment 530270 [details] [diff] [review]
Raises 404 not found instead of 500 errors on junk parameters

This patch is also a gentle start to client integration tests for shipping.
Attachment #530270 - Flags: review?(l10n)
(Assignee)

Comment 4

8 years ago
Created attachment 530272 [details] [diff] [review]
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.
Attachment #530270 - Attachment is obsolete: true
Attachment #530270 - Flags: review?(l10n)
(Assignee)

Comment 5

8 years ago
Created attachment 530326 [details] [diff] [review]
Raises 404 not found instead of 500 errors on junk parameters and includes shipping in hudson build
Attachment #530272 - Attachment is obsolete: true
Attachment #530326 - Flags: review?(l10n)

Comment 6

8 years ago
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 <peterbe@mozilla.com>

@@ +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.
Attachment #530326 - Flags: review?(l10n) → review+
(Assignee)

Comment 7

8 years ago
Created attachment 530364 [details] [diff] [review]
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.
Attachment #530326 - Attachment is obsolete: true
Attachment #530364 - Flags: review?(l10n)

Comment 8

8 years ago
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.
Attachment #530364 - Flags: review?(l10n) → review+
(Assignee)

Comment 9

8 years ago
Landed on origin/develop
https://github.com/mozilla/elmo/commit/03781f233cee6e97f48440fc6563b45f0d3d8840
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

8 years ago
Can I get a push to test the fixes?
(Reporter)

Comment 12

8 years ago
Thanks, Peter + Axel.

Verified FIXED.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 13

8 years ago
Created attachment 531445 [details]
Full list of fuzzed URLs

Updated

7 years ago
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.