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

Switch to URI.parse to parse httprequest host line. #84

Closed
wants to merge 1 commit into from

Conversation

wishdev
Copy link
Contributor

@wishdev wishdev commented Jan 15, 2022

This solves the issue of the use of underscores in a host name. Using URI.parse makes us consistent with its functionality.

This solves the issue of the use of underscores in a host name. Using
URI.parse makes us consistent with its functionality.
@jeremyevans
Copy link
Contributor

Underscores are not valid in domain names. Is there a reason we should have webrick support underscores in in host names? Is it to support non-DNS host names (e.g. WINS)?

@wishdev
Copy link
Contributor Author

wishdev commented Jan 15, 2022

I do not have a good answer here - underscores work "for me" in any context I use them. I respect that an RFC somewhere may or may not allow them in a domain name. RFC3986 which I believe controls the parsing of a uri and which URI.parse uses as the default parser is fine with the underscore.

Webrick currently appears to be using the regexps from the RFC2396 parser within URI. That appears to have issues with the underscore.

If there is no desire to go down the URI.parse road (and I grasp/respect/appreciate why there would not be) - would the alternative of moving those 2 lines (494-495) out to a method so I can override the method with the URI.parse concept be at all acceptable?

@jeremyevans
Copy link
Contributor

I am fine with extracting a method to make it easier to extend the behavior. Could you submit a pull request for that?

@wishdev
Copy link
Contributor Author

wishdev commented Jan 16, 2022

Moved to #85

@wishdev wishdev closed this Jan 16, 2022
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.

2 participants