Fix bad matching against subdomain regex #7

2014-04-19T01:07:03Z

romansky commented

2014-04-19 01:07:03 +00:00

(Migrated from github.com)

This pattern will match against any (domain).com, instead it should extract (subdomain).domain.com, the change fixes this.

defunctzombie commented

2014-04-19 02:01:00 +00:00

(Migrated from github.com)

Please add tests. Also, if we are going this route, then we might as well change the .* to a proper regexp group for the characters that would be supported for domains and tld.

Please add tests. Also, if we are going this route, then we might as well change the `.*` to a proper regexp group for the characters that would be supported for domains and tld.

defunctzombie commented

2014-04-19 02:02:49 +00:00

(Migrated from github.com)

Also, tests fail because it no longer works on "localhost" domain. I am not sure this fix is actually needed. The only thing I can see it preventing is matching on (domain).com and thus trying to see if the domain client exists. Maybe we can address that.

Also, tests fail because it no longer works on "localhost" domain. I am not sure this fix is actually needed. The only thing I can see it preventing is matching on `(domain).com` and thus trying to see if the `domain` client exists. Maybe we can address that.

romansky commented

2014-04-19 12:54:10 +00:00

(Migrated from github.com)

Sorry for breaking the build.. (didn't run local tests obviously)
From what I gathered in the code, the bottom part where you decide if "maybe_bounce" is the way to go is to route requests coming from localtunnel vs requests who want to use the resource on the other end..
Inside maybe bounce it assumes that if you have a regex match (client_id, client), you must be in the "clients" map, but this would obviously not be the case for a localtunnel client registration request.

Considering this, we the two goals:

allow localtunnel registration requests
allow web clients connect to the target resource

Because you may want to support these schemes for end points urls:
ddd.ccc.bbb.aaa (dev.beta.mysite.com)
ccc.bbb.aaa (beta.mysite.com)

You need to pass the base domain as part of the options, then a simple indexOf will assert if this is a registration request or an end user trying to connect to a resource..

In anyway, this is currently broken for me because the current regex will match against mysite.com and will try to look up the "clients" list and fail (instead of returning false and allowing the localtunnel client to register)

Your thoughts? (happy to add tests when we have an agreed approach)

Sorry for breaking the build.. (didn't run local tests obviously) From what I gathered in the code, the bottom part where you decide if "maybe_bounce" is the way to go is to route requests coming from localtunnel vs requests who want to use the resource on the other end.. Inside maybe bounce it assumes that if you have a regex match (client_id, client), you must be in the "clients" map, but this would obviously not be the case for a localtunnel client registration request. Considering this, we the two goals: - allow localtunnel registration requests - allow web clients connect to the target resource Because you may want to support these schemes for end points urls: ddd.ccc.bbb.aaa (dev.beta.mysite.com) ccc.bbb.aaa (beta.mysite.com) You need to pass the base domain as part of the options, then a simple indexOf will assert if this is a registration request or an end user trying to connect to a resource.. In anyway, this is currently broken for me because the current regex will match against mysite.com and will try to look up the "clients" list and fail (instead of returning false and allowing the localtunnel client to register) Your thoughts? (happy to add tests when we have an agreed approach)

defunctzombie commented

2014-04-19 14:55:16 +00:00

(Migrated from github.com)

I actually broke this with a recent commit that allows for capturing arbitrary length subdomains! I am inclined to special case localhost and require a tld for other capturing. We could event append a "fake" tld if we detect the hostname ends in localhost so the regexp for capture with tld can still be used and tested.

I think this approach will allow for both goals. The landing page is important and there is no reason why a subdomain name couldn't match the site name even tho that may not be used in practice.

I actually broke this with a recent commit that allows for capturing arbitrary length subdomains! I am inclined to special case `localhost` and require a tld for other capturing. We could event append a "fake" `tld` if we detect the hostname ends in `localhost` so the regexp for capture with tld can still be used and tested. I think this approach will allow for both goals. The landing page is important and there is no reason why a subdomain name couldn't match the site name even tho that may not be used in practice.

romansky commented

2014-04-19 15:02:28 +00:00

(Migrated from github.com)

What is the scenario for localhost? does it also apply for 127.0.0.1?
Is it only used for testing / local deployment?
Not sure what changes to make..

What is the scenario for `localhost`? does it also apply for `127.0.0.1`? Is it only used for testing / local deployment? Not sure what changes to make..

defunctzombie commented

2014-04-19 15:07:01 +00:00

(Migrated from github.com)

The scenario for localhost is when you are testing locally and use localhost in the url bar. Just makes testing easier, one could certainly just edit /etc/hosts. I usually add a few custom subdomains to /etc/hosts of the form blahblah.localhost so I can test localtunnel manually in addition to the automated tests.

Does not apply to 127.0.0.1 and that can just be ignored.

I am on the fence about just requiring a TLD in the match and adding docs to the readme about modifying /etc/hosts.

The scenario for localhost is when you are testing locally and use `localhost` in the url bar. Just makes testing easier, one could certainly just edit `/etc/hosts`. I usually add a few custom subdomains to `/etc/hosts` of the form `blahblah.localhost` so I can test localtunnel manually in addition to the automated tests. Does not apply to `127.0.0.1` and that can just be ignored. I am on the fence about just requiring a TLD in the match and adding docs to the readme about modifying `/etc/hosts`.

romansky commented

2014-04-19 15:20:47 +00:00

(Migrated from github.com)

Cool.
Ok, how about adding a command line option for specifying a TLD?
Something like --domain mydomain.com should work

Cool. Ok, how about adding a command line option for specifying a TLD? Something like `--domain mydomain.com` should work

defunctzombie commented

2014-04-19 15:21:29 +00:00

(Migrated from github.com)

Don't want to add a command line option for this. I don't think we need it and it just adds another thing that could be messed up during deployment.

romansky commented

2014-04-19 15:31:48 +00:00

(Migrated from github.com)

I thought I was following :)
Did you mean read a TLD from the DNS of the local machine?

I thought I was following :) Did you mean read a TLD from the DNS of the local machine?

defunctzombie commented

2014-04-19 15:33:14 +00:00

(Migrated from github.com)

Nope, I meant basically the fix your PR provides :) By "require TLD" I meant require the correct number of "dots" basically!

I will merge a variation of your PR and fix the tests.

Nope, I meant basically the fix your PR provides :) By "require TLD" I meant require the correct number of "dots" basically! I will merge a variation of your PR and fix the tests.

romansky commented

2014-04-19 15:35:17 +00:00

(Migrated from github.com)

Gotchya! :)
Great, I will test the fix locally when its ready..

Gotchya! :) Great, I will test the fix locally when its ready..

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hoelee/server#7