Real CORS handling #6

jucrouzet · 2017-01-06T23:21:53Z

Introduces a real handling of CORS requests as a replacement of just setting a

Control allowed/forbidden origins (Instead of one origin or "*")
Allow the Icy-MetaData header to be sent and expose icy-* headers in response
Control CORS headers by request path

Configuration model :

    <!-- CORS settings
         Defines the CORS configuration to allow or forbid browsers Javascript requests.

         Each uri path can be configured in a <path> node, Icecast will test the a requested
         path against each base path defined here, from the longest to the shortest, the first
         match will be used.
         Ex: If defined base path are "/abcd", "/a/", "/a" and "/"
           "/abdef" will uses the config from "/abcd"
           "/a/b" will uses the config from "/a/"
           "/af" will uses the config from "/a"
           "/foo" will uses the config from "/"
    -->
    <cors>
        <path base="/admin/">
            <!--
                <no-cors> Disallow javascript requests to this path.
                It has precedence over any other tags.
            -->
            <no-cors/>
        </path>
        <path base="/streams">
            <!-- 
                <allowed-origin> allow any origin that starts by the given value.
                    In this example, http://www.site1.com, http://www.site1.com/page,
                    http://www.site3.com/page, http://www.site1.com/long/path.html
                    will be allowed, but NOT http://site1.com or http://site2.com
            -->
            <allowed-origin>http://www.site1.com</allowed-origin>
            <!-- 
                <forbidden-origin> forbids any origin that starts by the given value.
                    In this example, http://www.site2.com, http://www.site2.com/page,
                    http://www.site2.com/long/path.html will be forbidden, but NOT
                    http://site1.com or http://site2.com

                <forbidden-origin> has precedence over <allowed-origin>, that means
                    that an origin allowed by <allowed-origin> and forbidden by
                    <forbidden-origin> will be forbidden.
            -->
            <forbidden-origin>http://www.site2.com</forbidden-origin>
            <!-- 
                <exposed-header> Allow to manually defines exposed headers.
                    By default, all icy-* headers are exposed, you can use these
                    tags to only expose some manually chosen ones.
            -->
            <exposed-header>icy-br</exposed-header>
            <exposed-header>icy-metaint</exposed-header>
        </path>
        <path base="/status-json.xsl">
            <!-- To allow any origin, use '*' : -->
            <allowed-origin>*</allowed-origin>
        </path>
    </cors>

This feature will allow a full-JS HTML5 player with MetaData synchronization for example.

jucrouzet · 2017-01-06T23:24:09Z

PS :
That's my first PR on this project and didn't see any unit tests, please let me know if there is somes, i tested on my side a lot of config and request scenari.
I also tested with valgrind if any leaks were introduced, found none.

jucrouzet · 2017-01-10T21:43:06Z

Just added OPTIONS handling for preflighted requests as we need to send the Icy-MetaData header to get metadatas.

…wed origin

ePirat · 2017-02-01T00:50:59Z

That's my first PR on this project and didn't see any unit tests

We do not have any tests at the moment.

ePirat

First of all, thanks for this contribution! I've done a first review of the code, see the comments below.

One general thing I noticed:
If would be great if you could indent using 4 spaces, to be consistent with the rest of the Icecast codebase.
Additionally please try to avoid whitespace changes that are unrelated to your changes, it seems you stripped all trailing whitespace from lines, causing changes that are unrelated to this feature.

ePirat · 2017-02-01T01:11:06Z

src/cfgfile.c

+      }
+      if (!cors_path->exposed_headers) {
+        ICECAST_LOG_ERROR("Out of memory while parsing config file");
+        break;


Shouldn't you return an error to the caller and not 1 for success, when this happens?
It might be a good idea to use 0 as success and everything non-zero as error for consistency, or add a comment documenting what the possible returned values are supposed to indicate.

ePirat · 2017-02-01T01:19:06Z

src/cors.c

+ * This program is distributed under the GNU General Public License, version 2.
+ * A copy of this license is included with this source.
+ *
+ * Copyright 2014,      Philipp "ph3-der-loewe" Schafft <lion@lion.leolix.org>,


Wrong copyright and author name.

ePirat · 2017-02-01T01:29:21Z

src/cors.c

+  strcat(new_out, header_name);
+  strcat(new_out, ": ");
+  strcat(new_out, header_value);
+  strcat(new_out, "\r\n");


Might be better to use snprintf here, as we do in util_build_http_header here.

ePirat · 2017-02-01T01:35:29Z

src/cors.c

+    *len -= header_end[i] - header_start[i];
+  }
+  return;
+}


Sorry, I do not really understand what this function is supposed to do. Could you maybe add a few comments to the code in this function?

It removes an header by its name in the current headers str.
It's used in _remove_cors which removes all known cors headers (probably added in by <header> in config) is the path was denied in CORS config.

ePirat · 2017-02-01T01:39:16Z

src/cors.h

+ *                      oddsock <oddsock@xiph.org>,
+ *                      Karl Heyes <karl@xiph.org>
+ *                      and others (see AUTHORS for details).
+ * Copyright 2014,      Philipp "ph3-der-loewe" Schafft <lion@lion.leolix.org>,


Same as above, wrong copyright and author name.
Should be just:

Copyright 2017, Your Name <your-email@example.com>

jucrouzet · 2017-02-01T12:38:49Z

I'll make requested changes tomorrow.
Sorry for the indent / padding spaces trim ... the IDE did it automatically. I'll revert it and apply the correct indent.
(BTW, in case of, Github has a ignore whitespaces mode by adding ?w=1 to the url : https://github.com/xiph/Icecast-Server/pull/6/files?w=1)

Should I submit a patch to the ML or just updating the PR is OK ?

ePirat · 2017-02-01T12:43:50Z

I think just updating the pull request is enough.
Thanks a lot for letting me know about the ?w=1 option, had no idea it exists.

jucrouzet · 2017-02-01T12:47:35Z

https://github.com/tiimgreen/github-cheat-sheet 🌮 ;) They have a lot of useful pro tips

jucrouzet · 2017-02-04T17:51:07Z

@ePirat : Done changes as requested.

marcelgwerder · 2017-03-08T09:41:20Z

Cool to see a PR for this, any update on the status? we're currently trying to use XHR and the WebAudio API for streaming. This limitation of icecast is currently a blocker.

ePirat · 2017-03-09T21:40:01Z

Sorry, right now there are very few people working on Icecast and most are busy right now with other things. Big features like this require some coordination, regarding the config changes and such, so this is definitely not something I could do on my own.

jucrouzet · 2017-05-09T22:28:26Z

@ePirat ? Any chance ?

dm8tbr · 2017-05-10T06:37:59Z

Config example up top LGTM. +1
Regarding code review I trust ePirat and PH3, as I don't have time at the moment to delve in.

jucrouzet · 2017-06-28T04:07:08Z

Desperately hoping for any news 😿

jucrouzet · 2017-08-24T14:25:47Z

(Resolved conflicts, in case of ...)

ePirat · 2017-09-04T19:57:42Z

Thanks a lot for the conflict resolution! I will have another look and do some more testing this week.

ePirat · 2017-10-08T21:39:15Z

src/cfgfile.c

+        return;
+    }
+    for (length = 0; origins[length]; length++);
+    for(int step = 0; step <length; step++)


Please use { } here, makes it clearer that those are nested loops.

(Nitpick: space after for is missing, same for the one below, and the if)

ePirat · 2017-10-08T21:39:55Z

src/cfgfile.c

+    int length;
+    char *temp;
+
+    if (!origins || !origins[1]) {


Why is this checking for origins[1] instead of origins[0]?

Means if there are no origins or only one origin (just 0 and not 1), there is not need to sort`

Is it guaranteed origins always has at least 2 entries (a string and NULL ptr)? Else you could read out of the array bounds with origins[1] when not checking origins[0] first.

Yep, possible values are :

origins = null

origins = &{string, null}

origins = &{string, string, null}

etc ...

But you're right, this part is not explicit as it should be, I will refactor it

ePirat · 2017-10-10T08:38:27Z

In general this looks ok and seems to work as I would expect. Thank you very much for the contribution! We still need a second reviewer (ph3) before we can merge. For this purpose I would like to push your code to a branch on the git.xiph.org Icecast repo, as Github is just a mirror.

jucrouzet · 2017-10-10T08:46:29Z

I will make the two fixes you requested above tonight, I'll mention you when pushed here for the branch creation.
Thanks !

jucrouzet · 2017-10-10T17:20:54Z

@ePirat : Tried to make it more readable. Hope it's ok for you.

ePirat · 2017-11-11T23:46:38Z

Create the branch with your PR: jucrouzet-CORS

Brahmasmi · 2017-11-16T14:35:53Z

Thanks @jucrouzet and @ePirat. I am excited to be finally be able to use @jucrouzet's excellent https://github.com/jucrouzet/icecast.js. That would also answer my question #15.

Would it be possible for this feature to be cut into a new point release?

ePirat · 2017-11-16T14:39:33Z

It's not even merged yet, as I already explained previously, the purpose of the branch is to make review easier.

Would it be possible for this feature to be cut into a new point release?

No, thats not how our versioning works.

Brahmasmi · 2017-11-16T14:49:25Z

I apologize for not correctly interpreting your message. I am sorry.

I look forward to if and when this gets released in icecast server. As I said, I am excited to be able to use this + icecast.js. Thanks.

ePirat · 2017-11-16T16:58:34Z

I apologize for not correctly interpreting your message. I am sorry.

No problem, I am looking forward to get this into Icecast as soon as we can, too.

jucrouzet · 2018-01-08T12:46:45Z

Happy birthday to this PR 🎂 ;)

Brahmasmi · 2018-02-17T14:23:20Z

Hi @ph3-der-loewe ,

Would it be possible for you to please review the following branch - http://git.xiph.org/?p=icecast-server.git;a=shortlog;h=refs/heads/jucrouzet-CORS ?

This would help add a functionality that would be very useful for @jucrouzet and me, and I presume others as well.

Hi @ePirat ,

I am not sure if ph3 is active on github. In case he prefers a different channel for communication, could you please help share that? The aim is to connect with him to request him to review the above PR. Hopefully, the above PR is merged and then released so that the functionality can be used.

I would really appreciate all your help.

Danke.

ePirat · 2018-02-17T14:32:25Z

I already asked him to review it a while ago.

Brahmasmi · 2018-03-19T21:17:51Z

Hi @dm8tbr,

Would it be possible for you to please review the following branch - http://git.xiph.org/?p=icecast-server.git;a=shortlog;h=refs/heads/jucrouzet-CORS ?

This would help add a functionality that would be very useful for @jucrouzet and me, and I presume others as well.

Danke.

mydnic · 2018-04-07T16:35:53Z

Hello all

As everyone else, I'm really looking forward to see this. In fact I need this a lot because I'm building a website that helps all webradio owners to create cool web interfaces.
As from now I forced all my javascript features to use old version of jQuery because it didn't required the HTTP "OPTIONS" pre-flight request, but now it seems that it's really a requirement that cannot be avoided anymore..

My last solution to counter the lack of OPTIONS method support in Icecast, is to perform a request to my own backend and then perform a curl request on Icecast.. which begin to seems like an overkill 😛

Anyway, it would be great that Icecast PR reviewers could check this asap and merge !
If they're not active anymore, what can we do ??

Thanks a lot

Brahmasmi · 2018-04-18T12:02:58Z

@mydnic I think it would be very unfair to suggest that the Icecast reviewers are not active.

Just looking at the commits in the repository should be enough to indicate that @ph3-der-loewe is active and busy patching Icecast. More importantly, @ePirat has been very helpful and supportive.

In terms of what can be done - I am not in a position to comment. Unfortunately, current efforts in getting this patch reviewed have largely been unsuccessful.

@ePirat It may be entirely possible that this patch may have fallen off the radar of @ph3-der-loewe - it happens all the time with all of us volks. Would it be possible for you to please remind him again?

ePirat · 2018-04-18T12:21:41Z

He already had a quick look a few days ago. We are currently preparing a new beta release and a maintenance release for 2.4.x, I am sure once he has time, that he will have a detailed look at it.

Brahmasmi · 2018-06-08T15:19:55Z

Polite reminder.

@ePirat @dm8tbr @ph3-der-loewe.

ph3-der-loewe · 2018-06-09T12:07:11Z

Good morning,

On Fri, 2018-06-08 at 15:20 +0000, Brahmasmi wrote: Polite reminder. @ePirat @dm8tbr @ph3-der-loewe.

Here is a little status update: * In mean time I rebased the patch several times to master (as master moved since the path was supplied). You can find it in the branch ph3-jucrouzet-CORS-orig. * We currently work on a new stable release as well as there is some work on the build system for current development. This also requires time from the team. * tbr expressed his interest in including the patch in the next beta. * The patch still has some major problems, such as: * Fails to build for me. * Does not use correct types (int vs. size_t, ...). * Does implement it's own sorting function. * Does bypass the ACL layer, may allow harmful remote access. * Does not use correct coding style. * I think that those problems are best handled with me re-writing parts of it. Including fundamental support for OPTIONS-method. The actual patch would be applied ontop of that. * We already reorganized how requests are handled internally, allowing more simple OPTIONS support. The patch is still on my radar. Yet there are some tasks that need to be done before. Generally speaking there is some process, however I fear it can hardly been seen from the outside. :( Hope that updates helps. With best regards, PS: Is there a ticket for us on our side? Github is like the worst place in the two universes to discuss software related stuff.

…

-- Philipp. (Rah of PH2)

ph3-der-loewe · 2018-06-15T19:09:11Z

Good evening, I just added some experimental support for OPTIONS. You can find it in branch ph3-options. With best regards,

…

-- Philipp. (Rah of PH2)

Brahmasmi · 2018-07-29T20:39:51Z

Thank you @ph3-der-loewe for your response. I apologize for the delay in my reply.

I see that you have merged the ph3-options branch in the master. Thanks for that.

With respect to the problems with the patch, I am afraid I cannot comment on it or provide help with it. I am just a user who hopes to see a simple browser play an Icecast stream.

@jucrouzet - If it is possible, could I request you to please collaborate with @ph3-der-loewe on this patch. I am not sure what channel or mechanism of collaboration would be suitable, but I am sure @ph3-der-loewe would be able to share it with you. I am requesting you, since you are the original author of this patch, and might be able to collaborate with @ph3-der-loewe on it.

Thanks once again to @jucrouzet for the patch and @ePirat @dm8tbr and @ph3-der-loewe for their help and support.

dm8tbr · 2018-07-31T12:54:50Z

A simple browser can play an Icecast stream already just fine. It's the advanced use cases that require a more complete CORS implementation. This is what this is about.

Our stated goal is to have this included in the 2.5 release, this has not changed. Ideally, it will be already part of the next beta to give it wider test exposure.

I would kindly ask to not "poke" or "bump" this issue as there is nothing it will accomplish or accelerate, aside from eating into our time that could be spent actually working on this.

From this point on, everyone please keep it to technical discussion on the implementation of CORS.

dm8tbr · 2018-11-13T05:07:55Z

Philipp now finished the manual merge, rework and feature work on CORS. Everything is on the master branch and will ship as part of beta 3.

I'm thus closing this.

Brahmasmi · 2018-11-20T13:10:16Z

Merci beaucoup @jucrouzet.

Danke @ePirat @dm8tbr and @ph3-der-loewe.

If I may ask, would we happen to know the timeline for the 2.5 release (full, not beta)?

Dhanyavaad.

dm8tbr · 2018-11-20T20:41:43Z

It will ship once it is considered done.

There will be at the very least one more Beta, but we might decide to release a Beta 4 or Beta 5 if it is deemed necessary for whatever reason.
There will be one or more Release Candidates
There will be a final 2.5.0 release

There is no specific timeline except for the above order of events.

If there are commercial entities interested in accelerating this process, please contact me directly to discuss options of contracting core developers, enabling them to spend more than their spare time on Icecast.

Brahmasmi · 2018-11-20T21:31:09Z

Danke @dm8tbr for sharing the order of events. I understand and appreciate the inability of anyone to forecast future. By timeline, I meant the order of events. Apologies for my incorrect choice of words.

jucrouzet added 3 commits January 6, 2017 16:43

Config file parsing for <cors> option

5909d15

Sort paths and origins to optimize lookups

052c330

Handle cors in headers response

a8debba

Handle pre-flighted requests

d272760

ePirat added the enhancement label Jan 10, 2017

Access-Control-Allow-Headers should be lowercase & handle "*" as allo…

74153ca

…wed origin

ePirat reviewed Feb 1, 2017

View reviewed changes

ePirat review changes

74e3399

4 spaces indent

2d35fa4

Merge branch 'master' into CORS

7a0179e

Brahmasmi mentioned this pull request Sep 8, 2017

Request for suggestions for pure JS HTML5 MSE based player for Icecast streams #15

Open

ePirat self-assigned this Sep 24, 2017

ePirat reviewed Oct 8, 2017

View reviewed changes

Rearrange/comment origins sorting. Linting

a0bb255

jucrouzet mentioned this pull request Jul 6, 2018

Documentation greatly appreciated... jucrouzet/icecast.js#2

Open

dm8tbr closed this Nov 13, 2018

jucrouzet deleted the CORS branch November 13, 2018 07:57

Real CORS handling #6

Real CORS handling #6

Conversation

jucrouzet commented Jan 6, 2017

jucrouzet commented Jan 6, 2017

jucrouzet commented Jan 10, 2017

ePirat commented Feb 1, 2017 • edited Loading

ePirat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ePirat Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jucrouzet commented Feb 1, 2017

ePirat commented Feb 1, 2017

jucrouzet commented Feb 1, 2017

jucrouzet commented Feb 4, 2017

marcelgwerder commented Mar 8, 2017

ePirat commented Mar 9, 2017

jucrouzet commented May 9, 2017

dm8tbr commented May 10, 2017

jucrouzet commented Jun 28, 2017

jucrouzet commented Aug 24, 2017

ePirat commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ePirat commented Oct 10, 2017

jucrouzet commented Oct 10, 2017

jucrouzet commented Oct 10, 2017

ePirat commented Nov 11, 2017

Brahmasmi commented Nov 16, 2017

ePirat commented Nov 16, 2017 • edited Loading

Brahmasmi commented Nov 16, 2017

ePirat commented Nov 16, 2017

jucrouzet commented Jan 8, 2018

Brahmasmi commented Feb 17, 2018

ePirat commented Feb 17, 2018

Brahmasmi commented Mar 19, 2018

mydnic commented Apr 7, 2018

Brahmasmi commented Apr 18, 2018

ePirat commented Apr 18, 2018

Brahmasmi commented Jun 8, 2018

ph3-der-loewe commented Jun 9, 2018 via email

ph3-der-loewe commented Jun 15, 2018 via email

Brahmasmi commented Jul 29, 2018

dm8tbr commented Jul 31, 2018

dm8tbr commented Nov 13, 2018

Brahmasmi commented Nov 20, 2018

dm8tbr commented Nov 20, 2018

Brahmasmi commented Nov 20, 2018

ePirat commented Feb 1, 2017 •

edited

Loading

ePirat left a comment •

edited

Loading

ePirat Feb 1, 2017 •

edited

Loading

ePirat commented Nov 16, 2017 •

edited

Loading