coleifer / sqlite-web

Web-based SQLite database browser written in Python
MIT License
3.42k stars 339 forks source link

The switch from POST to GET for queries creates problems for queries with new lines inside the Home Assistant Addon #140

Closed pearj closed 10 months ago

pearj commented 10 months ago

I noticed recently that running queries with SQLite Web that have new lines weren't working anymore when embedded inside Home Assistant and found that it's been a problem since 0.6.x when POST was switch to GET for queries which looks like happened in this commit: https://github.com/coleifer/sqlite-web/commit/bae45402470a53e9bf11ec1125f76ea61189b91b

There has been an issue open since October over at the home assistant addon: https://github.com/hassio-addons/addon-sqlite-web/issues/281

Is there anything to be done to resolve this inside sqlite-web, it looks like security measures inside Home Assistant don't like encoded new line characters %0A and %0D which doesn't appear to be easily resolvable on the Home Assistant addon side.

Is it possible to have an option to run SQL queries base64 encoded or maybe encoding new line characters some other way?

I realise it's a bit of a stretch to ask for changes so that it embeds easier in a third party application, but I thought it was worth an ask.

pearj commented 10 months ago

I went looking to find out why Home Assistant doesn't allow newlines in URLs, and it's because they're considered unsafe according to the WhatWG URL parsing standard, apparently cpython strips them out during URL parsing too.

See: https://github.com/home-assistant/core/pull/90348

coleifer commented 10 months ago

Thanks for sharing this, I've made a commit here c776cdc188c4120543b6af25e571782a8ea2b867 that allows this to work with GET or POST, and switches the form method back to POST by default. I had no idea that URL-encoded get parameters could be considered unsafe, that seems strange to me -- what middleware is catching this?

I think this is a bug in the "ha" code, as it's perfectly fine for newlines and carriage returns to be passed in the GET as long as they're quoted (as far as I can tell):

>>> from urllib.parse import *
>>> unquote('q=foo%0D%0Abar')
'q=foo\r\nbar'
>>> parse_qs('q=foo%0D%0Abar')
{'q': ['foo\r\nbar']}

As you can see, Python has no problems with newlines. It's the janky middleware in the "ha" application that is being overly-strict, in my opinion.

The HA clowns locked the discussion on https://github.com/home-assistant/core/pull/90348 so I can't comment there, but I think this is a bug on their side. Passing multiline strings in the querystring should be perfectly fine provided you escape things.

coleifer commented 10 months ago

Pushed new version 0.6.2 with this change.

pearj commented 10 months ago

Hi @coleifer,

I think you are probably right about it being quoted making it ok, but I thank you for walking it back anyway, because I don’t know how we’d convince them otherwise.

You’re the best! You’ll make a lot of people happy.

nkinnan commented 10 months ago

I also just wanted to say thank you! Regardless of the situation, in the end, the reason we write code is so that it can be useful. This change will allow many more of us to benefit from your efforts, and they are appreciated!

frenck commented 10 months ago

Thank you for this change! ❤️

The HA clowns

Not sure if that was needed, adding value, or called for.

../Frenck

PS: I wanted to compare diff between releases, however, it seems like the git tag for this release is missing.

coleifer commented 9 months ago

https://github.com/coleifer/sqlite-web/compare/0.6.1...0.6.2 - here's the diff, I'd forgotten to push the tag I guess.

frenck commented 9 months ago

Thanks for pushing the tag!

pearj commented 9 months ago

Hi @coleifer,

We noticed that the query box on the main page uses GET still. It 404s with a query on the one line and trips up HA’s filtering a multiline. I haven't looked into it super deeply, but all the other query boxes I've tried work fine.

Should this be a separate GitHub issue?

coleifer commented 9 months ago

Oh shoot you're right, I missed that one - apologies.

This is fixed and a new version 0.6.3 is up on pypi now.

pearj commented 9 months ago

Thanks @coleifer that fixed the GET/POST issue, but the 404 we're getting is because the form action is /query/ but running in HA the URL for me is: homeassistant.local/api/hassio_ingress/-Xs0IbVz5d9jlaDhzdtz3u6GKjTGzzDG_1zil4CSbPI/. Using the chrome dev tools I changed the form action to query/, and that fixed it.

If I'm on the query page homeassistant.local/api/hassio_ingress/-Xs0IbVz5d9jlaDhzdtz3u6GKjTGzzDG_1zil4CSbPI/query/ I notice that it uses . as the form action.

Is that something that changed recently or does home assistant need to set an env variable for base URL or something?

coleifer commented 9 months ago

Out of curiosity, do any of the links work, e.g. clicking on a table name in the left-nav or clicking the query button in the top-right? They all use the same format and the form action is not special in any way. Nothing has changed on my end with regards to the URLs. You can use the -u option to specify a URL prefix for the application if you wish.

nkinnan commented 9 months ago

This issue with the main page query not working seems to pre-date the issue with multi-line. I was able to use single-line queries after clicking onto the page for a specific table, but never on the main page. According to my google searches this was a "known problem" with the home assistant integration even before the multi-line thing came up.

coleifer commented 9 months ago

No idea man all the urls are built the same way sounds like some nonsense in the ha integration

pearj commented 9 months ago

Yeah only specific urls work with the HA integration:

https://github.com/hassio-addons/addon-sqlite-web/blob/358481e33b19afd1f03c95f050b8c19ded99c6b5/sqlite-web/rootfs/etc/nginx/templates/ingress.gtpl#L14-L25

I'm going to try and use that -u option, I'm fairly sure it should work, which should resolve random issues like this