Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix empty path parsing #418

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Conversation

alerque
Copy link
Member

@alerque alerque commented Nov 8, 2023

Fixes #417.

How does this fix look? Simplistic but more valid than the current parsing I think.

@catwell
Copy link
Member

catwell commented Nov 8, 2023

It does look more correct but I am unsure what to do with the path. If I am not mistaken here the end result is that the path will be nil because of this. Is this what we want? Alternatives may be either "/" or an empty string I guess.

Also, don't we have the same issue with the fragment (#)?

Finally I see this code attempts to support HTTP path parameters (;). This is an obscure, deprecated feature and unless other parts of LuaSocket require it I suggest we remove that code (and consider path parameters part of the path, like they are in modern HTTP).

@alerque
Copy link
Member Author

alerque commented Nov 8, 2023

Yes, the path will be nil, but at least the query string will have the right value now and socket will be valid too. Given that path already returns nil in other cases if not set (i.e. empty per spec) that didn't seem like something I wanted to mess with. It might be more valid per spec to return an empty string, but that also might be a breaking change here! Ouch.

I didn't check if the fragment has the same issue, but I can do that.

@alerque
Copy link
Member Author

alerque commented Nov 8, 2023

Yes, the same issue applied to fragments and I just fixed it the same way. I'm still hesitant to make path an empty string since I don't know what downstream usage might look like right now. Perhaps somebody is testing if a path exists by evaluating the path attribute for truthyness.

@catwell
Copy link
Member

catwell commented Nov 9, 2023

@alerque I agree re. keeping the path nil.

I still think we should delete the code parsing path parameters. This style of path parameters comes from long deprecated RFC 1808 and is not used in modern HTTP, semicolons are a normal character (and here it may also cause issues with path parsing).

@Tieske
Copy link
Member

Tieske commented Nov 9, 2023

imho for all elements, path, fragment, etc, this test must hold;

assert(uri == url.build(url.parse(uri)))

Exceptions;

  • path normalization
  • escape/unescape differences

So for path being nil or "", both could work. But for fragment it should return a string, even an empty one, if # is present, as I don't think there is any other way to convey the fragment identifier # was present in the first place.

@alerque
Copy link
Member Author

alerque commented Nov 9, 2023

@Tieske That's the way it works right now and we're not changing that, an empty string "" is returned if the fragment marker is included even if no fragment is present.

@catwell I agree, but I think that should be in a separate PR as it is a much more involved fix & possibly breaking change. I'll get the party started with one after this merge and we can talk about the details there. It's part of the builder and other logic too.

@alerque alerque merged commit 8a5368b into lunarmodules:master Nov 9, 2023
19 checks passed
@alerque alerque deleted the url-empty-path branch November 9, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

url.parse() doesn't correctly handle URL with no path component.
4 participants