Repeated queries are not handled correctly #157

New Issue

makeworld · 2020-05-24T04:10:29Z

makeworld commented

2020-05-24 04:10:29 +00:00

If a page is loaded with a query, so the URL is now gemini://example.com/page?querytext, and then that page returns a status code 10, for input using another query, bombadillo requests this URL after the user types in the second input: gemini://example.com/page?querytext?secondquery. As far as I can tell, this is incorrect. The query part should be replaced, not appended to.

From my knowledge of Go (but not bomadillo source), the fix should be to parse the current page's URL, replace the query field, then get the new URL string, not just append to it.

This might be worth bringing up on the mailing list, definitely something Sean should add to the client torture tests. I'm too tired now, but anyone who sees this feel free to do it and link back here.

If a page is loaded with a query, so the URL is now `gemini://example.com/page?querytext`, and then that page returns a status code 10, for input using another query, bombadillo requests this URL after the user types in the second input: `gemini://example.com/page?querytext?secondquery`. As far as I can tell, this is incorrect. The query part should be replaced, not appended to. From my knowledge of Go (but not bomadillo source), the fix should be to parse the current page's URL, replace the query field, then get the new URL string, not just append to it. This might be worth bringing up on the mailing list, definitely something Sean should add to the client torture tests. I'm too tired now, but anyone who sees this feel free to do it and link back here.

makeworld changed title from ~~Multiple queries are not parsed correctly~~ to Repeated queries are not handled correctly

2020-05-24 04:11:29 +00:00

makeworld commented

2020-05-24 04:58:00 +00:00

Additionally, query strings are not being escaped when they are sent to the server. This is a quick fix with url.QueryEscape from "net/url", but it's important to stay within RFCs.

Additionally, query strings are not being escaped when they are sent to the server. This is a quick fix with `url.QueryEscape` from `"net/url"`, but it's important to stay within RFCs.

sloum commented

2020-05-24 05:08:27 +00:00

That is weird. That had been an earlier issue a long while back and I thought that got fixed. I guess we either regressed or I never got around to fixing it!

I can add this to the list of things to fix for this weekends release as this is a very easy fix. Thanks for bringing it to my attention!

That is weird. That had been an earlier issue a long while back and I thought that got fixed. I guess we either regressed or I never got around to fixing it! I can add this to the list of things to fix for this weekends release as this is a very easy fix. Thanks for bringing it to my attention!

sloum added the

bug

gemini

labels 2020-05-24 05:08:45 +00:00

sloum referenced this issue

2020-05-24 14:50:31 +00:00

Fixes querystring chaining issue #159

sloum commented

2020-05-24 14:52:08 +00:00

This has now been addressed in #159

The PR does not include querystring encoding as the spec does not directly include it and the gemini search engines do not support it (they do not unencode a querystring before passing it to their search). Since we do not want to break functionality for users I am putting of escaping for now. However, the issue of querystring chaining has been resolved once that PR merges in.

This has now been addressed in #159 The PR does not include querystring encoding as the spec does not directly include it and the gemini search engines do not support it (they do not unencode a querystring before passing it to their search). Since we do not want to break functionality for users I am putting of escaping for now. However, the issue of querystring chaining has been resolved once that PR merges in.

sloum added the

in progress

label 2020-05-24 16:41:08 +00:00

sloum closed this issue

2020-05-24 16:57:33 +00:00

sloum commented

2020-05-24 16:59:12 +00:00

This has been closed per #160

Re: the querystring escaping issue:

I have posted to the mailing list to ask about escaping querystrings and see about getting that specifically added to the spec (as I agree, it is a good call to follow the standards in this case). Since it does not seem to be the widely used current practice on gemini I have left things in that regard as is for now, but will open a new issue when something is landed on.

This has been closed per #160 Re: the querystring escaping issue: I have posted to the mailing list to ask about escaping querystrings and see about getting that specifically added to the spec (as I agree, it is a good call to follow the standards in this case). Since it does not seem to be the widely used current practice on gemini I have left things in that regard as is for now, but will open a new issue when something is landed on.

sloum removed the

in progress

label 2020-05-24 17:01:35 +00:00

makeworld commented

2020-05-24 17:57:33 +00:00

The spec doesn't need to include it, because it's explicitly part of the URL RFC that gemini makes use of. As far as I can tell, any client that doesn't encode the query string is breaking that RFC. I think that Bombadillo should follow the RFC here, and that the rest of gemini space should adjust if necessary.

the gemini search engines do not support it (they do not unencode a querystring before passing it to their search).

I will email Natalie (GUS developer) about this, because this is her problem, or her library's problem.

The spec doesn't need to include it, because it's explicitly part of the URL RFC that gemini makes use of. As far as I can tell, any client that doesn't encode the query string is breaking that RFC. I think that Bombadillo should follow the RFC here, and that the rest of gemini space should adjust if necessary. > the gemini search engines do not support it (they do not unencode a querystring before passing it to their search). I will email Natalie (GUS developer) about this, because this is her problem, or her library's problem.

makeworld commented

2020-05-24 18:15:58 +00:00

I just read the mailing list stuff, and solderpunk seems to agree. Could this issue be reopened, and the query stuff added, maybe as v2.3.1?

I just read the mailing list stuff, and solderpunk [seems to agree](https://lists.orbitalfox.eu/archives/gemini/2020/001020.html). Could this issue be reopened, and the query stuff added, maybe as v2.3.1?

sloum referenced this issue

2020-05-24 18:16:00 +00:00

Query strings are not being escaped for gemini status 1x requests #161

sloum commented

2020-05-24 18:19:43 +00:00

@makeworld I have added a new issue (since this issue was really about querystring chaining anyway): #161

I would expect a patch sometime later in the week (as 2.3.1), but possibly as early as tomorrow.

@makeworld I have added a new issue (since this issue was really about querystring chaining anyway): #161 I would expect a patch sometime later in the week (as `2.3.1`), but possibly as early as tomorrow.

❤️ 1

makeworld commented

2020-05-25 22:01:56 +00:00

This is still an issue with the latest Bombadillo, where second and third queries are appended - could you reopen it? I experienced this at gemini://gemini.conman.org/hilo/, it's a good site to test this issue. Try guessing for the number, and there will be an error on your second guess.

This is still an issue with the latest Bombadillo, where second and third queries are appended - could you reopen it? I experienced this at `gemini://gemini.conman.org/hilo/`, it's a good site to test this issue. Try guessing for the number, and there will be an error on your second guess.

sloum commented

2020-05-26 00:13:40 +00:00

Ah. I see. I did not even know what he is doing there was a possible thing. I misunderstood what you meant by repeated queries and thought you meant that if a url already had a query element that a new one would replace it. Which is happening. But because a status 1x response does not trigger a render (or a point in navigation history - an eventual 2x response will do those things though) in bombadillo another one being sent in response to an initial one will create a strange loop where that doesnt happen. There may actually be more wrong than just that. It really had never occurred to me that a status 1 response would then trigger another status 1 response. I dont care for that. :-/

I would prefer that this gets a new issue added making it explicit that this has more to do with a status 1x response triggering another status 1x response than replacing a querystring on an existing URL.

Feel free to add it and reference this issue. If I get a chance later I'll do so if you have not already.

👍

Ah. I see. I did not even know what he is doing there was a possible thing. I misunderstood what you meant by repeated queries and thought you meant that if a url already had a query element that a new one would replace it. Which _is_ happening. But because a status 1x response does not trigger a render (or a point in navigation history - an eventual 2x response _will_ do those things though) in bombadillo another one being sent in response to an initial one will create a strange loop where that doesnt happen. There may actually be more wrong than just that. It really had never occurred to me that a status 1 response would then trigger another status 1 response. I dont care for that. :-/ I would prefer that this gets a new issue added making it explicit that this has more to do with a status 1x response triggering another status 1x response than replacing a querystring on an existing URL. Feel free to add it and reference this issue. If I get a chance later I'll do so if you have not already. :thumbsup:

sloum reopened this issue

2020-05-26 02:19:50 +00:00

sloum commented

2020-05-26 02:19:56 +00:00

On second thought, I am going to reopen this. It turned out to be bad test availability during the previous fix (I didnt have a good reference for this specific situation and no access to a server with cgi support to replicate it on my own) and a reference to the wrong var in the original fix. My bad :-/

Your link to Sean's example was so useful that I think I may want to amend out issue policies, when bug reporting at least, to always include:

A way to replicate the issue
Output of bombadillo -v
OS and architecture

I will add a card to get that to happen.

PR #166 is open to resolve the issue. Feel free to pull the branch it references if you would like to test it locally.

On second thought, I _am_ going to reopen this. It turned out to be bad test availability during the previous fix (I didnt have a good reference for this specific situation and no access to a server with cgi support to replicate it on my own) and a reference to the wrong var in the original fix. My bad :-/ Your link to Sean's example was so useful that I think I may want to amend out issue policies, when bug reporting at least, to always include: - A way to replicate the issue - Output of `bombadillo -v` - OS and architecture I will add a card to get that to happen. PR #166 is open to resolve the issue. Feel free to pull the branch it references if you would like to test it locally.

👍 1

sloum referenced this issue

2020-05-26 02:21:27 +00:00

Adds in the correct variable when checking for existing querystring value #166

sloum self-assigned this 2020-05-26 02:21:51 +00:00

sloum added the

in progress

label 2020-05-26 02:21:59 +00:00

makeworld commented

2020-05-26 03:38:02 +00:00

Thanks a lot!

sloum commented

2020-06-30 16:42:23 +00:00

I am closing this as resolved. I verified both items are currently fixed in the develop branch. Not sure why this did not get closed at the time. If anything further with this issue is notices, comment here and I'll re-open.

I am closing this as resolved. I verified both items are currently fixed in the `develop` branch. Not sure why this did not get closed at the time. If anything further with this issue is notices, comment here and I'll re-open.

sloum closed this issue

2020-06-30 16:42:24 +00:00

Sign in to join this conversation.