772605 - Consider allowing cross-origin requests for certain API calls

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: API, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

When we remake the Calendar website, I would like to include a widget with a download button like on AMO that displays the latest version.

To do so, I would like to make an XMLHttpRequest to https://services.addons.mozilla.org/en-US/thunderbird/api/1.5/addon/lightning and get the data from the XML.

Obviously, this will fail since our page is on www.mozilla.org so its cross-origin. Therefore I propose one of two things:

A) Allow cross origin calls for the API fully, i.e set Access-Control-Allow-Origin: * in the response headers.

B) Allow cross origin calls for all mozilla.org sites, i.e set Access-Control-Allow-Origin: $_REQUEST["HTTP_ORIGIN"] if the origin is part of mozilla.org.

What do you think?

Comment 1

[Wil Clouser [:clouserw]]

We support cross-origin requests on our statistics data I don't think adding this to our other views would be a problem.  I don't think we should add it universally though - the API continues to expand and adds new functionality (including functions other than just retrieving data) so I'd like to keep us on a whitelist system.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Wil Clouser [:clouserw] from comment #1)
> We support cross-origin requests on our statistics data I don't think adding
> this to our other views would be a problem.  I don't think we should add it
> universally though - the API continues to expand and adds new functionality
> (including functions other than just retrieving data) so I'd like to keep us
> on a whitelist system.

Definitely, I was thinking the same when I wrote the bug but I see I forgot a few words to make that clear.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

So, what are the next steps on this bug? I guess it won't make it into the 27th update, or is it an easy fix?

Comment 4

[Wil Clouser [:clouserw]]

need a=rforbes and then, yeah, it's a low priority.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This should do it for the addon details, you just have to say what other api calls should have it allowed.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 644683 [details] [diff] [review]
Fix - v1

Tests coming up shortly.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

This patch includes tests, choosing cvan for the review since he talked me through it on IRC (thanks!)

Comment 8

[Christopher Van Wiemeersch [:cvan]]

Comment on attachment 644686 [details] [diff] [review]
Fix - v2

Looks very good. I've made the small changes myself and merged in your patch: https://github.com/mozilla/zamboni/commit/dbfa18a

I wanted to make sure that your patch lands for our push this Thursday (7/26).

Thanks!

Comment 9

[Christopher Van Wiemeersch [:cvan]]

https://github.com/mozilla/zamboni/commit/dbfa18a

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Looks like you changed the line with the ^L control character, was that on purpose?


@@ -133,7 +133,7 @@ class ControlCharacterTest(TestCase):
-        a.name = "I ove You"
+        a.name = "Iove You"

Comment 11

[Christopher Van Wiemeersch [:cvan]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Looks like you changed the line with the ^L control character, was that on
> purpose?
> 
> 
> @@ -133,7 +133,7 @@ class ControlCharacterTest(TestCase):
> -        a.name = "I ove You"
> +        a.name = "Iove You"

Yeah, I saw that. I'll fix it with a different editor. Test still passes (not surprisingly), but I'll amend it. Thanks.

Comment 12

[Raymond Forbes[:rforbes]]

just saw this.  i am ok with it.