Live Broadcasting - Warn if Mount is not escaped when sending metadata

[Marco Pracucci]

The "Live Broadcasting" > "Mount" field value is sent as an *unescaped* query string parameter when Mixxx sends metadata to the icecast server.

This lead to the following bug (actual behavior):
- Mount set to "/live?stream_id=1&tracking=2"
- Mixxx builds the metadata URI like "/admin/metadata?mode=updinfo&mount=/live?stream_id=1&tracking=2&song=title"
- As you can see, the mount query string parameter is *not escaped*, so it's not possible to correctly parse the "mount" parameter

How it should be (expected behavior):
- Mount set to "/live?stream_id=1&tracking=2"
- Mixxx builds the metadata URI like "/admin/metadata?mode=updinfo&mount=2Flive%3Fstream_id%3D1%26tracking%3D2&song=title"

Mixxx: 1.11.0
Operating System: OSX 10.8.5

[Kevin Wern (kevin-m-wern)]

https://github.com/mixxxdj/mixxx/pull/771

This is my best attempt at the feature, but I'm not sure if I understand the problem, or if querystring mounts are even supported. I know that by running an Icecast server locally that this seemed to be the previous behavior:

- Mixxx keeps the mount (say, /live?stream_id=1&tracking=2, as in Marco's example), the same. shout uses the mount for the base of the request, which is SOURCE (mount) HTTP/1.0.
- Icecast, using the httpp library, parses this request. httpp splits the URI at the first '?', retaining only the resource and moving the parameters to an AVL tree of variables after parsing. So at this time, a resource /live is generated.
- Mixxx, using the original string, updates the metadata with libshout. The url is parsed by httpp in Icecast, which is split again at the first '?'. The nested query string does mess up the results (because it naively splits at '&' and '='), but the main thing that matters is that the requested mount path to update would look something to the effect of '/live?streamid=1'.

So the real issue is that the requested mount point does not match the one originally generated. With the current fix, the server can generate the correct mount point, but requires listeners to escape the characters in the URI, as well--and even then, the file version of the streams don't work. On the server itself, stream files and admin requests are generated using the unescaped string, which means the links like /live?stream_id=1&tracking=2.m3u do not work. The fix I added really only serves to make mixxx handle querystring items more gracefully (and send metadata correctly in the cases Marco described), while retaining all the flexibility of the original setting,

I'm not sure if I understand how Marco is using the above mount point, because it seems to not be usable in Icecast2. I think that the correct behavior would be either:
- Remove the query string and leave only the resource when the mount point is set in preferences (or do this in a later step and retain it in the dialog).
- Figure out some way to combine the admin querystring and the custom querystring, in the case where the server admin wants to have custom settings. I do not know how useful this would be.

I don't know, what you guys think would be best?

[Marco Pracucci (marco-pracucci)]

Hey there,
please see my suggestions below.

> Remove the query string and leave only the resource when the mount point is set in preferences (or do this in a later step and retain it in the dialog).

Oh god, definitely not.

If you remove the query string, you're actually breaking the protocol specifications and you will probably break some applications out there (including Spreaker, that's one of the biggest broadcasting platforms with 5M users). When you send metadata, the mountpoint must exactly match the original mountpoint sent with the SOURCE request, and so it must include any querystring parameter in the same exact order if the original SOURCE requests did have it.

The issue I reported more than one year ago is the following: when you send metadata, you're actually doing an HTTP request. This HTTP request contains a querystring parameter called "mount" whose value is the original mountpoint (including the original query string). The value of "mount" parameter is not escaped, and it should just be escaped. Basically any language provides a function to escape an URI component (ie. in javascript it's encodeURIComponent) or at least has a library to do so, so I guess it's shouldn't be pretty complex to do.

Thanks for reading,
Marco

[Daniel Schürmann (daschuer)]

Hi Marco, than you for your comment.
Kevin has added this line

QByteArray baMountPoint = QUrl::toPercentEncoding(mountPoint.toLatin1());

is this sufficient?

If you like, you can also look at the entire patch:
https://github.com/mixxxdj/mixxx/pull/771

[Marco Pracucci (marco-pracucci)]

Hey there,

I don't have any experience with QT, but after some googling it looks that `QUrl::toPercentEncoding()` does URL component encoding in the right way.

I checked out the full PR diff and I actually can't understand why you remove the leading '/' from the mountpoint (I read the comment above the code block, but it's a bit unclear to me). May you give any hint, please?

[Florian Kiekhäfer (technik-zwr)]

I just wanted to give my 2 cents...

... and i must say i am confused.

the way i understand the description of the bug is, that streaming works fine but only when sending metadata there is a problem with sending metadata, am i correct?

What i am thinking is:
It may very well be that the problem is because of not escaping the mountpoint in this URL, but the actual URL is constructed, (and requested via HTTP) by libShout and not by Mixxx itself. Also i do think that the icecastserver would expect the mount unescaped for the initial connect. (I would try, but i am not even sure how to configure a mountpoint with queryparameters as a name, i have never seen something like this in an sample config. As a sidenote: i only know mountpoints prefixed with "/")

As there is just one variable in the stucture for the mount there is even no possiblity for mixx to handle it differently.

I am of the impression thats more a bug in libshout than mixxx but i am just guessing here, no tested facts from my side this time.

[Daniel Schürmann (daschuer)]

The libshout example includes the prefix "/"
https://github.com/codders/libshout/blob/master/examples/example.c#L43

[Daniel Schürmann (daschuer)]

libshout adds the prefix "/" if there is none:
https://github.com/codders/libshout/blob/a17fb84671d3732317b0353d7281cc47e2df6cf6/src/shout.c#L493

So it actually does not matter if the mount point in Mixxx has a / or not.

the mounpoint is used unescaped inside libshout:
  rv = sock_write(socket, "GET /admin/metadata?mode=updinfo&mount=%s&%s HTTP/1.0\r\nUser-Agent: %s\r\n%s\r\n",
    self->mount, encvalue, shout_get_agent(self), auth ? auth : "");

This mean that it must at least not contain blanks.

By the way:
Shoutcast (SHOUT_PROTOCOL_ICY) does not support mountpoints.
So we may gray-out the field if Shoutcast is selected.

[Daniel Schürmann (daschuer)]

@Marco: please correct me when I am wrong.

I have looked around in some icecast manuals, but I have found no point of using a mountpoint like that

/live?stream_id=1&tracking=2

It is always used like a normal file path.

If you look to the libshout source, it is obvious that you can set a mount-point that fails the queries. You can do things similar to SQL injection.

Escaping the stream is a workaround for that, but maybe we should also warn the user, since it is unlikely that someone is able to listen to that stream. Right?

[Florian Kiekhäfer (technik-zwr)]

That is exactly like i would expect that it is right now.

At this point libShout is using HTTP, and so he mount should be escaped, i agree.
If Mixx does this by escaping the string before passing it to libShout, that problem would be solved.

But i also means, that libshout will always use the escaped name. The question i ask is, if this is correct wenn you do initialy set the source.

----
static int create_http_request(shout_t *self)
{
 char *auth;
 char *ai;
 int ret = SHOUTERR_MALLOC;

 /* this is lazy code that relies on the only error from queue_* being
  * SHOUTERR_MALLOC */
 do {
  if (queue_printf(self, "SOURCE %s HTTP/1.0\r\n", self->mount))
   break;
-----
I think that is not the case.

FIrstly: I'd say encoding would be wrong here, because the resource name is "/live?stream_id=1&tracking=2" and not "live%3Fstream_id%3D1%26tracking%3D2" (i have striped"/" here).
I suspect it would not connect to / create the correct (or better desired) mount on the server.
Kevin already described this already in an way, when he said that this " requires listeners to escape the characters in the URI"

Secondly: Encoded values are decoded somewhere (thats the point isn't it? :) ), and would result in the original string which now (again) does not exist because we've specified another name when connecting..

I'll come back to what i said: i think the expected behavior is achieved when encoding in libShout (for the metadataupdateurl only).

I am saying expected behavior because according to

https://gist.github.com/ePirat/adc3b8ba00d85b7e3870#specifying-mountpoint-information

(Did not find something REALLY official on the icecast website)

"The mountpoint itself is specified as the path part of the URL." - no mention of query parameters. But that doesn't mean that some hoster build something around it, which requires these parameters. I looked around on spreaker (a a mentioned example) a bit, and to me it looks like they use something thats just like close enough to icecast so that most of it works already out of the box.

[Florian Kiekhäfer (technik-zwr)]

Sorry, forgot to add this link:

http://developers.spreaker.com/recording-api/audio-stream

And i was too slow, daschuer wrote something while i was typing.

[Kevin Wern (kevin-m-wern)]

@Dan

https://github.com/codders/libshout/blob/a17fb84671d3732317b0353d7281cc47e2df6cf6/src/shout.c#L493

We do need to get rid of the slash if we escape it, because libshout will do a comparison of '/' to the first character, see there is a '%', and prepend a '/'.

The problem is that we cannot set the metadata mount and actual mount separately, so we are forced to use the escaped one.

As I said before, the problems I experienced with testing my own server are:
* The server expects a resource-only mount, and truncates an unescaped querystring to just the resource.
* Using an escaped mount with any querystring character requires listeners to escape url in order to connect.
* Unless you include the '/' handling code in my pull request, including a leading slash in preferences will result in icecast trying to mount at '//*'

[Florian Kiekhäfer (technik-zwr)]

Theres one more thing i noted:

I tried using a mountpoint with paramters on a server which doesn't expect specific name. The stream worked fine, but mountpoint on the server didn't actually include the parameters in the name. I think the regular icecast server just ignores them (and spreaker uses them for some settings). If this is actually the case, there is no need for escaping, instead one could strip the parameters.
But again: For Metadataupdate only, which would have to be done in libShout too.

[Daniel Schürmann (daschuer)]

For me, the remaining question is: should we merge:

https://github.com/mixxxdj/mixxx/pull/771

What are the real world use cases?

Without the merge, the user has full control, but may hit pitfalls if he tries to use unescaped strings.

according to
https://gist.github.com/ePirat/adc3b8ba00d85b7e3870#specifying-mountpoint-information

samplerate=44100;quality=10.0;channels=2

should become

samplerate=44100;quality=10%2e0;channels=2

but with the patch, it becomes

samplerate%3D44100%3Bquality%3D10.0%3Bchannels%3D2

Is this an issue?

If yes, the user is not able to fix it, after the applying the patch.

[Florian Kiekhäfer (technik-zwr)]

If i had a say, no. The patch is most probably not even a solution to the problem.
I think the rather technical description, was decieving.
Of course the URL will, with the patch, will be correctly encoded but nothing is gained with that.

The bug is about Metadata not working, when the mountpoint contains certain characters. (Queryparameters just includes at least one of them, namely the "?")

Current state: Stream is working, but metadata is not working.
Reason is:
The mount name is "/live?stream_id=1&tracking=2". Thats defiend on Connect via the connection whichs stays open for sending audio data.
When updating the metadata an HTTP GET is send via an second connection. The mountname is one of its paramters.
As such icecast unescapes the paramters as its done by any HTTP server. I do not know what the resullt actually is (hard to say what result out of undefined characters for an encoding) but its clear it doesn't equal the mount name. This leads to the request being ignored, as icecast doesn't recognize / cannot find the mount it should update the data on.

State after patch:
A connect and sending audiodata is generally possible, but the mountpoint (resource) will have a different name than expected.
It is something like: "/live%3Fstream_id%3D1%26tracking%3D2"

(actual encoding varys a bit depending on used library)

The name is a) kryptic for endusers b) may cause errors für automatic generated links on the server c) may even prevent a sucessfull connect when the server expects a very specific mount name containing any characters which needs escaping.
But most importantly the metadata still should not work.
The mountparameter can now be unescaped, and will now resolve to "/live?stream_id=1&tracking=2", but a said above thats not the nameof the mount anymore.

If someone could make a testbuild marco-pracucci should be able to verify that.

[Daniel Schürmann (daschuer)]

Thank you for clarification.

My original goal was it to handle common pitfalls, in the broadcasting setup.

Coming back to that goal, how about just warn the user if he tries to use an problematic mount point.

I have something like that in mind:
* continue to pass the trimmed user entry to libshout
* for the test: remove all "/" from the user mountpoint entry
* decode the user entry (in case it is already encoded)
* encode it again
* compare the original mount point with the decoded
* warn if there are different.
* The warning could be a label below the mountpoint text box:
 "Warning: Suspicious mount point"

Any thoughts?

[Florian Kiekhäfer (technik-zwr)]

Well...

Trimming should be fine.

Entering an already encoded mount would likely contain characters which are a problem. ;) (Keep in mind that libShout isn't sending the wrong name, but merely the wrong HTTP representation where needed so to speak. )

For a warning I would only check if the entered mountpoint name (as is) would need escaping to be interpreted correctly as a query parameter. Meaning I would test if the mountname equals the percentencoding of it and display a warning if they do not match.

But I also think I would exclude the "/" from the encoding ( there seems to be a parameter for that in QUrl::toPercentEncoding).
Although it is under normal circumstances a character which needs encoding, it does seem to work fine with icecast and according to the documentation linked above, the source is a path, which would make "/genre/stream.ogg" a legit mountname although the encoded version "/genre/stream.ogg" would not match, and stripping the prefix is not enough here.

"Suspicious mount point" is not that much of information. At least you should have a tooltip or be able to click on it and a message appears with something along the lines: "The name of your mountpoint seems to include characters which propably prevent the update of metadata on the server"

----

Although the idea for the warning is nice, is just a hint to a underlying problem mixxx is not responsible for. ( namely libShout building non HTTP-compliant URLs which lead to failing metadata updates). So I'd suggest raising a bug/enhancement for libShout, but perhaps that should be done by someone who is affected.

[Florian Kiekhäfer (technik-zwr)]

Oh, somethere along my writing something got lost. Of course the encoded Version is "%2Fgenre%2Fstream.ogg"

[Daniel Schürmann (daschuer)]

The an encoded mount point like "%2Fgenre%2Fstream.ogg" will issue the warning if we try to encode it again and compare it with the original mountpoint, since % will be escaped again.
That's why it is required to decode the mountpoint first.
I think, you can even decode an unencoded mountpoint without changing it. So it should be possible to decode any user entered mountpoint before encoding it.

[Daniel Schürmann (daschuer)]

Thank you all for the input. I think we have a plan :-)

@Kevin: Do you agree? Do you still have fun to fix this bug again? (Sorry for pointing you such a fuzzy bug in the first place)

I think I will rephrase the bug title.

[Florian Kiekhäfer (technik-zwr)]

>The an encoded mount point like "%2Fgenre%2Fstream.ogg" will issue the warning if we try to encode it again and compare it with >the original mountpoint, since % will be escaped again.

Yes, and that would be correct because metadata update will NOT work properly in this case...

(I get the feeling that my explanations are - especially in english - not as clear as in my head ;) )

[Florian Kiekhäfer (technik-zwr)]

Just did some testing again with mixx 1.11:

Configured mount Mount on server Metadata OK
--------------------------- ----------------------- ----------------
test.ogg /test.ogg yes
/test.ogg /test.ogg yes
/test/2.ogg /test/2.ogg yes
/test%2F2.ogg /test/2.ogg yes, (for me very unexpected)
%2Ftest.ogg can't connect no
/test.ogg?x=abc /test.ogg no
/test.ogg%3Fx%3Dabc /test.ogg?x=abc no
%2Ftest.ogg%3Fx%3Dabc can't connect no
/test.ogg%3Fx%3Dabc%26y%3D8 /test.ogg?x=abc&y=8 no

I'm confused that it doesn't matter weather the / is encoded or not, but everything else seems fitting to my thinking

[Florian Kiekhäfer (technik-zwr)]

Also i did not find a player with which i could listen to the streams, for which i configured an encoded mount, no matter what kind of url i used.

Although we do not try to fix the problem anymore, based on the 6th test, i tried to set the metadata just as if the queryparamter were not not part of the name

/admin/metadata.xsl?song=song4711&mount=/test.ogg&mode=updinfo

it worked.

But (against our thinking) both of the following did not work:

/admin/metadata.xsl?song=song4712&mount=/test.ogg%3Fx%3Dabc&mode=updinfo
http://admin/metadata.xsl?song=song4712&mount=/test.ogg?x=abc&mode=updinfo

But that leads nowhere. Firstly i do not know, if that just the original icecast or on spreaker too and even if it were lib shou would still do the wrong thing, not mixxx.

There. I think that is really the last thing i can contribute to this topic.

[Daniel Schürmann (daschuer)]

Now I think I got it. Thank you for explaining it again.

[Kevin Wern (kevin-m-wern)]

@Dan, makes sense to me. I can work on this.

[Marco Pracucci (marco-pracucci)]

Thanks for all the effort you're putting in this issue. Florian perfectly
described the issue we got on our side (Spreaker's icecast custom server):

> When updating the metadata an HTTP GET is send via an second connection.
The mountname is one of its paramters. As such icecast unescapes the
paramters as its done by any HTTP server. I do not know what the resullt
actually is (hard to say what result out of undefined characters for an
encoding) but its clear it doesn't equal the mount name.

To clarify our use case: at Spreaker we currently use query string to
configure per-connection options to the server (it's not the opensource
icecast server, but a server completely written by us that follows the
Icecast2 specifications). For example, we support auto-sharing of your
stream on social networks: if you wanna enable it, just add "?shares=FB" to
share it on Facebook, or "?hidden=true" to keep it privately, ad so on.
Since an user could have multiple streams with the same mount path but
different options at the same time, when we receive the HTTP metadata
request we have to compare the full mountpoint (including query string) and
not just the path, in order to correctly route metadata to the right stream.

Once your changes will be ready, I will be more than happy to test a
patched build against Spreaker.

2015-11-11 22:55 GMT+01:00 Daniel Schürmann <email address hidden>:

> Thank you all for the input. I think we have a plan :-)
>
> @Kevin: Do you agree? Do you still have fun to fix this bug again? (Sorry
> for pointing you such a fuzzy bug in the first place)
>
> I think I will rephrase the bug title.
>
> ** Summary changed:
>
> - Live Broadcasting - Mount is not escaped when sending metadata
> + Live Broadcasting - Warn if Mount is not escaped when sending metadata
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1329202
>
> Title:
> Live Broadcasting - Warn if Mount is not escaped when sending metadata
>
> Status in Mixxx:
> Confirmed
>
> Bug description:
> The "Live Broadcasting" > "Mount" field value is sent as an
> *unescaped* query string parameter when Mixxx sends metadata to the
> icecast server.
>
> This lead to the following bug (actual behavior):
> - Mount set to "/live?stream_id=1&tracking=2"
> - Mixxx builds the metadata URI like
> "/admin/metadata?mode=updinfo&mount=/live?stream_id=1&tracking=2&song=title"
> - As you can see, the mount query string parameter is *not escaped*, so
> it's not possible to correctly parse the "mount" parameter
>
> How it should be (expected behavior):
> - Mount set to "/live?stream_id=1&tracking=2"
> - Mixxx builds the metadata URI like
> "/admin/metadata?mode=updinfo&mount=2Flive%3Fstream_id%3D1%26tracking%3D2&song=title"
>
>
> Mixxx: 1.11.0
> Operating System: OSX 10.8.5
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1329202/+subscriptions
>

[Daniel Schürmann (daschuer)]

Does this mean a Spreaker user will likely try to set the mount:

"/test.mp3?shares=FB"

Accoriding to our cuurent planes this will issue a warning, because it is not equal its encoded string
"/test.mp3%3Fshares%3DFB"

If the user entries this encoded mount instead, it will fail to update metadata on the open source server as Florian pointed out.
Does this not happen with Spreaker?

@Marco: Please test with the current version of Mixxx and the Spreaker server, if it is able to update the metadata with such a string.

What is the mount-point for the listener in this case.

Maybe we need an additional checkbox: "encode mount"

[Marco Pracucci (marco-pracucci)]

Uhm, looks the proposed plan is actually worse the current bug, at least to Spreaker.

Is there any option to ignore this issue I reported more than a year ago and move on with other issues? :)

[Daniel Schürmann (daschuer)]

We do not want to make things worse.
What hare your test results. Why is it worse?
Does the current PR 771 work with speaker?
Maybe it is already a good solution. since it is unlikely that a user adds a query string to his mount using the open source server.

[Kevin Wern (kevin-m-wern)]

I think Florian made a lot of good points, most importantly that libshout cannot use separate strings for the metadata and SOURCE call. In Marco's case, it seems he wants the mount string to be escaped for the metadata, but not the SOURCE call.

If this isn't an urgent fix anymore, it might be worth raising an issue with libshout instead of warning the user about unescaped strings, which seems unapplicable to most other cases. However, warning about unescaped strings might be okay in the interim while we look at better solutions.

[Daniel Schürmann (daschuer)]

You might be right. But I am curious to hear about the test results with the Spreaker server. Maybe there is some magic going on, that it already works without changing libshout

[RJ Skerry-Ryan (rryan)]

Chiming into this bug quite late :)

This looks like a bug in libshout -- and not something that is fixable (in a principled manner) in Mixxx. My suggestion would be to submit a patch to libshout making this change. I don't think we should put a hack in Mixxx since it won't be able to fix this due to Florian's comment in #9.

As a workaround for Spreaker -- I would suggest putting hacks in place on the Spreaker server to detect and patch the issue (e.g. by applying heuristics to pre-parse the mount= argument of the URI before running it through your URI parsing library. Should be possible since you control the server-side completely.

[Marco Pracucci (marco-pracucci)]

Thanks for all the effort you're putting here. Let me reply to each open point.

> What hare your test results. Why is it worse? [...]
> As a workaround for Spreaker [...]

We reported this issue more than 1 year ago, and we immediately fixed it on the server side, otherwise Mixxx would still be not-compatible with our servers. We re-implemented HTTP request query string parsing, doing some hacks to correctly parse non-escaped query string parameters.

> In Marco's case, it seems he wants the mount string to be escaped for the metadata, but not the SOURCE call.

My original issue was: Icecast2 protocol is an HTTP-based protocol, thus it follows HTTP specs for the request/response.

Given the mountpoint "/live?stream_id=1&tracking=2" then the SOURCE's request first line is something like:
SOURCE /live?stream_id=1&tracking=2 HTTP/1.1

When Mixxx sends metadata, what it actually does is an HTTP GET request, that should follow HTTP specs. The request is something like:
GET /admin/metadata?mode=updinfo&mount=<mount>&song=title HTTP/1.1

where <mount> is the query string value that should be replaced with the original mountpoint. According to HTTP specs, the query string parameters MUST be escaped.

Right know, when Mixxx (well, libshout) does the request it's something like:
GET /admin/metadata?mode=updinfo&mount=/live?stream_id=1&tracking=2&song=title HTTP/1.1

but it's actually wrong, because the query string params MUST be escaped. It should be something like:
GET /admin/metadata?mode=updinfo&mount=2Flive%3Fstream_id%3D1%26tracking%3D2&song=title HTTP/1.1

> However, warning about unescaped strings might be okay in the interim while we look at better solutions.

If I correctly understand how the warning works, it conducts to a worse UX, IMO. When you insert a mountpoint in Mixxx it shouldn't be escaped. It should just be a standard URL, with no double-escaping applied to it. If you display a warning when the user insert the mountpoint "/live?stream_id=1&tracking=2", he will be disoriented, because he actually inserted a good URL and there's no reason to warn him that the URL is invalid (or unescaped).

> This looks like a bug in libshout [...]

I agree that should be fixed in libshout.

[RJ Skerry-Ryan (rryan)]

Good to hear you have a workaround in place server-side.

I filed a ticket with libshout:
https://trac.xiph.org/ticket/2234#ticket

[Florian Kiekhäfer (technik-zwr)]

It looks like, there are 2 now:

https://trac.xiph.org/ticket/2233

[RJ Skerry-Ryan (rryan)]

Doh -- I skimmed their ticket list but didn't see it. I'll dupe mine
against yours.

On Tue, Nov 17, 2015 at 9:51 AM, Florian Kiekhäfer <
<email address hidden>> wrote:

> It looks like, there are 2 now:
>
> https://trac.xiph.org/ticket/2233
>
> ** Bug watch added: Xiph.org Trac #2233
> http://trac.xiph.org/ticket/2233
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1329202
>
> Title:
> Live Broadcasting - Warn if Mount is not escaped when sending metadata
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1329202/+subscriptions
>

[Kevin Wern (kevin-m-wern)]

Sorry, that was my bad. I filed the ticket and it completely slipped my mind to post it here (got sleepy...).

[Daniel Schürmann (daschuer)]

I can reproduce this on Windows XP

[RJ Skerry-Ryan (rryan)]

@Daniel -- I think you got the wrong bug :). Bug #1514335 ?

[Kevin Wern (kevin-m-wern)]

Libshout issue has been fixed here:

https://trac.xiph.org/ticket/2233#comment:4

[Florian Kiekhäfer (technik-zwr)]

I am not so sure.
As my C -Skills are not very high someone should double check, but i would say it is not fixed.

I had a look at the changeset and to me it looks like they are encoding the mount for the SOURCE-Call and the metadata call now.

And i am still of the opinion that it is not correct to encode for the source-call, but perhaps someone should try before saying anything...

[Kevin Wern (kevin-m-wern)]

I can confirm that's what is happening--I guess fix wasn't the right word. The commit message says:

"...This commit fixes this by always doing the escaping step before sending this to the server. This may only be a problem in case GET parameters were included in the mountpoint URI. *However, this was never part of the concept and was supported by the main line Icecast2 server*..."

I don't know what that should mean for this bug. I know from the experience of using the escape fix on mixxx's end that this DOES fix the metadata issue, but it also destroys the meaning of the get parameters and forces users to escape reserved characters.

[Florian Kiekhäfer (technik-zwr)]

I doubt that is, what marco wanted...

Especially as i did not find a player with which i could listen to the streams, when i entered the already encoded mount-name in mixxx...

I guess it could even break what Spreaker tries to do completely...

[Marco Pracucci (marco-pracucci)]

Yes, this completely break what we do.

Sorry to be rude, but this issue is becoming a fucking nightmare for us.
After weeks, each of the proposed solution and patch doesn't fix anything
and just introduce more issues. Rolling back and keeping the current
behaviour looks the best solution to me.

Marco

2015-11-22 13:17 GMT+01:00 Florian Kiekhäfer <email address hidden>:

> I doubt that is, what marco wanted...
>
> Especially as i did not find a player with which i could listen to the
> streams, when i entered the already encoded mount-name in mixxx...
>
> I guess it could even break what Spreaker tries to do completely...
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1329202
>
> Title:
> Live Broadcasting - Warn if Mount is not escaped when sending metadata
>
> Status in Mixxx:
> Confirmed
>
> Bug description:
> The "Live Broadcasting" > "Mount" field value is sent as an
> *unescaped* query string parameter when Mixxx sends metadata to the
> icecast server.
>
> This lead to the following bug (actual behavior):
> - Mount set to "/live?stream_id=1&tracking=2"
> - Mixxx builds the metadata URI like
> "/admin/metadata?mode=updinfo&mount=/live?stream_id=1&tracking=2&song=title"
> - As you can see, the mount query string parameter is *not escaped*, so
> it's not possible to correctly parse the "mount" parameter
>
> How it should be (expected behavior):
> - Mount set to "/live?stream_id=1&tracking=2"
> - Mixxx builds the metadata URI like
> "/admin/metadata?mode=updinfo&mount=2Flive%3Fstream_id%3D1%26tracking%3D2&song=title"
>
>
> Mixxx: 1.11.0
> Operating System: OSX 10.8.5
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1329202/+subscriptions
>

[Daniel Schürmann (daschuer)]

Hi Marco,

It looks like it is urgent that you or one of you colleges propose a patch against libshout.

It looks like the solution proposed by Florian will work, but be honest, I am not sure since there are a lot of degrees of freedom.
This thread with its already #43 entries indicates that the topic is not trivial. It is much easier, to discuss the along a patch than in prosa like we did here.

It would be really frustrating, if our effort to make Spreaker support better, finally lead to break Spreaker support completely.

[Marco Pracucci (marco-pracucci)]

Hi Daniel,

I totally agree with you. It was frustrating for you too and I really *appreciate a lot* all your effort. What I would like to get is a solution that works for all (Spreaker, Mixxx users, other companies, ...) or no change at all, otherwise we risk to get a change that break backward compatibility to someone and will cause troubles to some businesses (Spreaker or others).

I'm going to reply to libshout bug tracking too.

[RJ Skerry-Ryan (rryan)]

I've already replied on the libshout bug and ph3-der-loewe said he would look into it.

[RJ Skerry-Ryan (rryan)]

So, what's the latest on this bug? Any action needed on the Mixxx side?

[Kevin Wern (kevin-m-wern)]

Last I remember we were waiting on this (it seems closed now):

https://trac.xiph.org/ticket/2233

At the very least I should take myself off this bug.

[Marco Pracucci (marco-pracucci)]

I posted the issue 4 years ago and we actually fixed it on the server side few days later, since it was impacting our customers (Spreaker podcasting platform). The hack we introduced to parse non-escaped metadata URIs is still alive and working, so - from my perspective - we can close the bug (and we'll keep the hack).

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7507