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

Review host matching validation #25

Closed
Tratcher opened this issue Mar 24, 2020 · 4 comments · Fixed by #882
Closed

Review host matching validation #25

Tratcher opened this issue Mar 24, 2020 · 4 comments · Fixed by #882
Assignees
Labels
help wanted We will welcome a contribution Type: Task A task that needs to be completed.
Milestone

Comments

@Tratcher
Copy link
Member

The RouteValidator uses a strict regex to check the host matching pattern. I have my doubts that this level of validation is even needed. The source is developer input, not external input, and it's only used in the route matcher (HostAttribute). We should be careful about second guessing what the HostMatcherPolicy can handle. For example the regex doesn't allow IDNs (Unicode hosts).

Preferably we'd use a validation helper provided directly by the framework so they'd be in sync.

@mifopen
Copy link

mifopen commented Nov 28, 2020

Is there any workaround for IDN support? I need to route requests based on IDN host

@Tratcher
Copy link
Member Author

@Tratcher
Copy link
Member Author

Tratcher commented Mar 8, 2021

#817 removed the offending regex because it was preventing hosts with ports. We still need to add some tests for IDNs, host:port, etc..

@Tratcher Tratcher added the help wanted We will welcome a contribution label Mar 8, 2021
@Tratcher Tratcher added the Type: Task A task that needs to be completed. label Mar 19, 2021
@karelz
Copy link
Member

karelz commented Mar 24, 2021

Triage: We need the IDN part which is still missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We will welcome a contribution Type: Task A task that needs to be completed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants