adonisjs / env

Framework agnostic environment variables parser and validator
https://docs.adonisjs.com/guides/environment-variables
MIT License
35 stars 10 forks source link

feat: allow to configure strictness of url validator #24

Closed targos closed 3 years ago

targos commented 3 years ago

Similar to https://github.com/adonisjs/env/pull/18

BREAKING CHANGE: the validator now throws by default for URLs without a protocol.

Proposed changes

This aims to support URLs without a top-level domain. Those are common in private networks such as corporate or Docker networks.

Note that the test with URL "foo" now doesn't throw anymore because by default isURL(str) doesn't require a protocol (such as http://) to be present.

thetutlage commented 3 years ago

Note that the test with URL "foo" now doesn't throw anymore because by default isURL(str) doesn't require a protocol (such as http://) to be present.

That means any string will pass the URL validation?

targos commented 3 years ago

Not exactly. I think any string which is a valid URL if you prefix it with https:// will pass. I can also add the require_protocol option if you want?

thetutlage commented 3 years ago

Can we accept tld and protocol as options when using the format: 'url'. That way, the default validation is strict and one can configure it differently (if required).

schema.string({ format: 'url', tld: false, protocol: true })
targos commented 3 years ago

In my opinion, it would be more correctly strict if the protocol was mandatory by default. I'm okay with adding options, but I don't really understand why the host should be validated to be a tld by default. My reasoning is that it is env validation, so data is provided by the developer, not by a random user. It is not unsafe to use a hostname without dots in a URL, and it is common on the server side (as written above with internal networks, and also just to put "localhost")

thetutlage commented 3 years ago

Then shouldn't the validation format be host over url?

targos commented 3 years ago

No. Take http://mailgun-service:1234/v1. This is a valid URL. We cannot use the host validator for it. I don't see why it should be rejected by default. The tld validator essentially checks that the hostname contains at least one dot. What is the benefit of this check at the level of an environment variable? It doesn't verify that the hostname can be resolved or anything. If I change it to http://mailgun.service:1234/v1, it would be accepted. But there's nothing really more valid with that version of the URL.

thetutlage commented 3 years ago

I get your point here. In fact the format based validation is never 100% accurate.

The main question is should the validation be strict or loose. You want it to be loose, I am saying it to be strict and all this comes down to subjective preference.

Even if we go with the loose validation, I would still want it to be configurable for someone who wants it to be strict.

targos commented 3 years ago

I mean, I'm okay with it being strict, but in that case I think we should also enable require_protocol by default, because technically, a URL without a protocol is not a valid URL (per spec).

thetutlage commented 3 years ago

Okay. I wasn't aware that require_protocol is false by default. Cool, lets go with the strict option if you are good with it. Also shorten the option names to protocol and tld

targos commented 3 years ago

Updated. PTAL

thetutlage commented 3 years ago

Thanks 🙂