[Kimchi-devel] [PATCH] [Kimchi 0/1] Fix VNC 1006: urlencode path

Tue Sep 12 16:32:25 UTC 2017

Aline,

I don't really have a patch I'm working on, I just was looking into the
issue.  The commit I sent earlier was an initial attempt to make the port
number for vnc relative so that I could use haproxy in front of kimchi.

To be honest, I don't know enough about url encodings to make a more
general fix.  Patrick, if you have any ideas, go for it!

Galen

On Mon, Sep 11, 2017 at 1:41 PM, aline.manera at gmail.com <
aline.manera at gmail.com> wrote:

> Hi Galen,
>
> I agree about relative URLs. I could try that and check it fixes the VNC
> problem but I can not reproduce the bug with upstream code.
>
> About your patch, please, go ahead and send it here. I'd be glad to merge
> it upstream.
>
> On Mon, Sep 11, 2017 at 12:09 PM, Galen Pospisil <
> galen.m.pospisil at gmail.com> wrote:
>
>> AFAIK, relative would be a better way to do things all around.  I tried
>> to address this earlier, but I think the way I went about it was less than
>> ideal.  I don't know much about encoding. In any case, relative urls and
>> port numbers would work best, as even with a reverse proxy in front of
>> kimchi, the browser will get the correct port.
>>
>> See: https://github.com/kimchi-project/kimchi/commit/b14fb611d3d7
>> 9904694a14e67f28db8f9c8acf40
>>
>>
>>
>>
>> On Mon, Sep 11, 2017 at 8:51 AM, Patrick Barrett <patrick at psbarrett.com>
>> wrote:
>>
>>> Yes, that would also fix it in this case, but as far as I can tell it
>>> would still be technically broken, there should not be a second '?' in
>>> the first page URL. I don't have access to test it while I'm at work,
>>> but AFAIK if you get rid of the slash and change the '?' to '&' it
>>> should also work and be properly formatted, I'm just not totally sure if
>>> it's semantically correct.
>>>
>>> --
>>>   Patrick Barrett
>>>   patrick at psbarrett.com
>>>
>>> On Mon, Sep 11, 2017, at 09:42, Aline Manera wrote:
>>> > Ops... I haven't tested serial console before and with this patch it is
>>> > broken.
>>> >
>>> > Analyzing the code and your detailed explanation, don't we just need to
>>> > make the path relative instead of absolute?
>>> > So instead referring to '/websockify' it would be 'websockify'
>>> >
>>> > On 09/11/2017 11:16 AM, Aline Manera wrote:
>>> > > Hi Patrick,
>>> > >
>>> > > Thanks for this patch!
>>> > >
>>> > > I could not reproduce the problem but I tested this patch and it does
>>> > > not break anything, ie, VNC continue working for me.
>>> > >
>>> > > Could you check it solves the issue?
>>> > >
>>> > > Regards,
>>> > > Aline Manera
>>> > >
>>> > > On 09/03/2017 11:44 AM, patrick at psbarrett.com wrote:
>>> > >> From: Patrick Barrett <patrick at psbarrett.com>
>>> > >>
>>> > >> Currently kimchi directs you to
>>> > >> `https://urldefense.proofpoint.com/v2/url?u=https-3A__hera.l
>>> an-3A8001_plugins_kimchi_novnc_vnc-5Fauto.html-3Fport-3D8001
>>> -26path-3D_websockify-3Ftoken-3D-5B...-5D-26encrypt-3D1&d=
>>> DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=IH2IbOioVtBemYyjWtO2FV71Y6
>>> c8-CWLHAPDI5dc2gs&m=55perbMCcacetij4ysJl2fw3_svtUDOuvY1ooDsE
>>> u94&s=VTJ3PxuzW_QFrrxibrpU87drdUNAlQrSbpwXNlXMsZ4&e=
>>> > >> `
>>> > >> when you click on 'View Console`, however this is improperly encoded
>>> > >> and Firefox guesses that the second `?` was meant to be an `&`. This
>>> > >> means that `token` and `encrypt` are sent to the current page rather
>>> > >> than being used for the websocket path.
>>> > >>
>>> > >> It seems that noVNC should "work" with this, but there's another bug
>>> > >> (or kimchi is using path improperly, not sure, could be fixed in
>>> > >> either AFAIK) in noVNC where it assumes path is relative and
>>> prepends
>>> > >> a `/` to it, which then when it uses and anchor tag to reformat the
>>> > >> URL (I think) in `WebUtil.injectParamIfMissing`. However when path
>>> > >> already has a leading `/` it interprets "websockify" as the host
>>> > >> (that is it becomes `//websockify/`) and the path becomes just
>>> > >> `/?token=[...]`.
>>> > >>
>>> > >> That code path is only executed when token is part of the original
>>> > >> URL, so this commit urlencodes the path to prevent the
>>> > >> interpretation of the token in the original URL and use it only in
>>> > >> path. This could also be fixed by changing the `?` to an `&` and
>>> > >> removing the leading `/`. However, `encodeURIcomponent` should
>>> > >> probably be used regardless since parts of path can come from a
>>> > >> config file.
>>> > >>
>>> > >> **Caveats**
>>> > >>
>>> > >> This is **UNTESTED** because I don't know how to build it all and
>>> > >> wanted to just get this since it's simple, but I have manually
>>> > >> tested it with the URL I believe this should produce.
>>> > >>
>>> > >> I wasn't totally sure about the encrypt param because it was just
>>> > >> another & param in the URL and there's nothing to indicate where if
>>> > >> it's part of path or part of the page url. However, it doesn't seem
>>> > >> to make a visible difference where it's put, or even if it is
>>> > >> removed. (This would also make a difference to the alternate
>>> > >> solution I suggested above.)
>>> > >>
>>> > >> Patrick Barrett (1):
>>> > >>    urlencode path
>>> > >>
>>> > >>   ui/js/src/kimchi.api.js | 24 ++++++++++++++----------
>>> > >>   1 file changed, 14 insertions(+), 10 deletions(-)
>>> > >>
>>> > >
>>> > > _______________________________________________
>>> > > Kimchi-devel mailing list
>>> > > Kimchi-devel at ovirt.org
>>> > > http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>> > >
>>> >
>>> _______________________________________________
>>> Kimchi-devel mailing list
>>> Kimchi-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>
>>
>>
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>>
>
>
> --
> Aline Manera
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20170912/849bd40d/attachment.html>