Don't treat numbers larger than the JS max number size as number values in environment variables #6119

skizer · 2021-06-07T22:31:22Z

-Fixes incorrect parsing of the env. file with numeric values that are outside of Number.MAX_SAFE_INTEGER resulting in unwanted behaviour. (e.g -> oauth -> discord_client_id)
-Removed unnecessary multiple "IF" statements since value can only be ether one of the listed values.

P.S This is my first ever pull request on github, please bare with me when I've missed something out.

* Fixes incorrect parsing of the env. file with numeric values that are outside of Number.MAX_SAFE_INTEGER resulting in unwanted behaviour. - Like wrong client_ids for oauth. (tested with discord oauth) * Removed unnecessary multiple "IF" statements since value can only be ether one of the listed values.

Incorrect parsing of numeric values in env.

paescuj · 2021-06-08T06:45:05Z

@skizer Good catch, thanks! Would it be okay for you if I'm going to include this change in my pull request #6101 as I'm working on the exact same lines? Props to you of course 😃

skizer · 2021-06-08T15:34:40Z

@paescuj Ya sure, I'm fine with that :)
I've also recently noticed that you can't use custom_params inside the .env file which grant supports to pass additionally parameters to the oauth2 provider.

*e.g discord allows to set the "prompt" parameter to skip the next authorization without the users need of approving it first.

So prolly the parser will need anyway some rework on that part.

According to grants documentation you can provide additionally custom parameters to supported OAUTH provider with ```custom_params```. This change allows to add them in JSON format and thus adding multiple parameters.

paescuj · 2021-06-09T08:55:25Z

@skizer Great, thanks! I've included the check for Number.MAX_SAFE_INTEGER in pull request #6101.

I'm not sure about your other change regarding JSON values (@rijkvanzanten):

It doesn't seem like custom params for OAUTH is implemented at the moment (at least not mentioned in the docs), right?
Do we even want JSON support in env variables?
Do we need a better check, e.g. try to convert to JSON beforehand? (Error handling)

Depending on @rijkvanzanten opinion here, it might be the best to open up a new pull request or issue for this.

skizer · 2021-06-09T10:14:42Z

@skizer Great, thanks! I've included the check for Number.MAX_SAFE_INTEGER in pull request #6101.

@paescuj You're welcome!

I'm not sure about your other change regarding JSON values (@rijkvanzanten):

It doesn't seem like custom params for OAUTH is implemented at the moment (at least not mentioned in the docs), right?

This is fine, since directus soly relies on grant for the OAUTH flow, thus making this implementation working right away.
That's also why the docs reference grant if you're interested into the available options. The doc needs an update on that anyway if this gets merged but probably only because of many people getting confused over what they can do in directus with OAUTH. (People asking in discord how oauth in directus works, what they can do and why it doesn't work)

If you're going straight according to the docs this would even be the correct way, it would say you should look up the grant doc and see what options are available and see that the custom_params actually expects this datatype.

Do we even want JSON support in env variables?

Exactly, IMO usually such stuff shouldn't even be configured in the env file due the nature of different types as parameters. Then again there's the pro for me that all your config stuff lives in one location, which speaks for it again :'D

Do we need a better check, e.g. try to convert to JSON beforehand? (Error handling)

No, since the console will scream at you right when trying to start up directus - misconfigured stuff should always fail, otherwise it leads to unwanted behaviour and in worst case even not noticing when going for production.

Depending on @rijkvanzanten opinion here, it might be the best to open up a new pull request or issue for this.

Can do that, let's wait for @rijkvanzanten :)

paescuj · 2021-06-09T10:38:33Z

Thanks for your feedback!

This is fine, since directus soly relies on grant for the OAUTH flow, thus making this implementation working right away.
That's also why the docs reference grant if you're interested into the available options. The doc needs an update on that anyway if this gets merged but probably only because of many people getting confused over what they can do in directus with OAUTH. (People asking in discord how oauth in directus works, what they can do and why it doesn't work)

If you're going straight according to the docs this would even be the correct way, it would say you should look up the grant doc and see what options are available and see that the custom_params actually expects this datatype.

So all OAUTH environment variables are (already) getting passed to grant?

Exactly, IMO usually such stuff shouldn't even be configured in the env file due the nature of different types as parameters. Then again there's the pro for me that all your config stuff lives in one location, which speaks for it again :'D

Yeah, there are pros and downs... That's why I've raised this question 😄

No, since the console will scream at you right when trying to start up directus - misconfigured stuff should always fail, otherwise it leads to unwanted behaviour and in worst case even not noticing when going for production.

I agree!

rijkvanzanten · 2021-06-09T15:36:26Z

P.S This is my first ever pull request on github, please bare with me when I've missed something out.

Could've fooled me! You're a natural at this

I've also recently noticed that you can't use custom_params inside the .env file which grant supports to pass additionally parameters to the oauth2 provider.

Any parameter you set is passed on to Grants configuration object lowercased:

directus/api/src/grant.ts

Lines 27 to 35 in 792281c

    
           // OAUTH <PROVIDER> SETTING = VALUE 
        
           parts.splice(0, 2); 
        
           const configKey = parts.join('_').toLowerCase(); 
        
           config[provider] = { 
        
           	...(config[provider] || {}), 
        
           	[configKey]: value, 
        
           };

That being said, this should be updated to work the same as DB_ and various other prefixes to allow for nested fields (https://docs.directus.io/reference/environment-variables/#type-casting-and-nesting) using getConfigFromEnv:

directus/api/src/database/index.ts

Line 17 in 792281c

const connectionConfig: Record<string, any> = getConfigFromEnv('DB_', [

Do we even want JSON support in env variables?

I don't think we need it at this point! The getConfigFromEnv setup we use in other places already supports nested values, so the custom params for Grant should be configurable with the same setup.

No, since the console will scream at you right when trying to start up directus - misconfigured stuff should always fail, otherwise it leads to unwanted behaviour and in worst case even not noticing when going for production.

100% agree

rijkvanzanten · 2021-06-09T15:38:25Z

Lets consider the nested value support for Grant config a separate PR, makes it a little cleaner in the archiving / release logs etc 🙂

rijkvanzanten · 2021-06-09T15:39:33Z

@skizer The only tip I have for next PR on GitHub is to use a new branch on your fork for the changes. Right now, your main branch contains all the changes you did for this feature, which means that any other PR you might open at the same time will also include those changes, as you start those from the same main branch

…port

@skizer

* Add support for _FILE environment variables * Enhance processing of _FILE env vars * Same processing as with other env vars (do not simply treat as string) - tested successfully * Warn if both variables are set (EXAMPLE and EXAMPLE_FILE) * Add comments to make it easier to understand the code * Set newKey only after file read was successful * Don't convert value > MAX_SAFE_INTEGER to number Thanks to @skizer! As stated by @skizer (from #6119): Altho it seems that we do have a numerical value it can happen that its outside of Number.MAX_SAFE_INTEGER thus resulting in a change of the original intended value e.g oauth -> discord -> client_id * Fix recursive logger-env import Co-authored-by: rijkvanzanten <rijkvanzanten@me.com>

skizer added 2 commits June 8, 2021 00:23

Merge pull request #1 from skizer/env-oauth-parsing-fix

71a08d2

Incorrect parsing of numeric values in env.

skizer requested a review from rijkvanzanten as a code owner June 7, 2021 22:31

Implements custom_param for OAUTH via grant

73b2163

According to grants documentation you can provide additionally custom parameters to supported OAUTH provider with ```custom_params```. This change allows to add them in JSON format and thus adding multiple parameters.

rijkvanzanten added API Improvement labels Jun 9, 2021

rijkvanzanten added this to the v9.0.0-rc.75 milestone Jun 9, 2021

Fix linter warnings, remove JSON support in favor of nested Grant sup…

d0c1e19

…port

rijkvanzanten changed the title ~~Incorrect parsing of numeric values in env~~ Don't treat numbers larger than the JS max number size as number values in environment variables Jun 9, 2021

rijkvanzanten assigned skizer and rijkvanzanten Jun 9, 2021

rijkvanzanten merged commit ad8bd3e into directus:main Jun 9, 2021

This was referenced Jun 9, 2021

Allow any of grant's (nested) configuration parameters (oAuth) #6155

Merged

Add support for _FILE environment variables #6101

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't treat numbers larger than the JS max number size as number values in environment variables #6119

Don't treat numbers larger than the JS max number size as number values in environment variables #6119

skizer commented Jun 7, 2021

paescuj commented Jun 8, 2021

skizer commented Jun 8, 2021 •

edited

Loading

paescuj commented Jun 9, 2021

skizer commented Jun 9, 2021 •

edited

Loading

paescuj commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

Don't treat numbers larger than the JS max number size as number values in environment variables #6119

Don't treat numbers larger than the JS max number size as number values in environment variables #6119

Conversation

skizer commented Jun 7, 2021

paescuj commented Jun 8, 2021

skizer commented Jun 8, 2021 • edited Loading

paescuj commented Jun 9, 2021

skizer commented Jun 9, 2021 • edited Loading

paescuj commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

rijkvanzanten commented Jun 9, 2021

skizer commented Jun 8, 2021 •

edited

Loading

skizer commented Jun 9, 2021 •

edited

Loading