Scope in refresh_token grant type is ignored. about node-oauth2-server HOT 7 CLOSED

@jorenvandeweyer great you found this!

from node-oauth2-server.

I thought about this issue yesterday.

We have to be careful how we implement this. With the current ignored scope parameter there is no potential security issue. You always get the scope of the original requested scope (?).

But Imagine following case:

Authorization Code Grant: User can select three scopes: "read write delete" but selects only read scope. Now if refresh token would contains scope value, it could contain "read write delete" scope. What you would expect is that you have again only "read"-scope as the user specified in the authorization code flow specified that he only wants to grant "read" permission to the resource server.

If we would just check if the user has the scope allowed by using the usual validateScope-method, an attacker could get with a refresh token more privileges than actually allowed by the user in the first password or authorization- grant. So we actually have to persist the original requested scope from the user then always check against that original scope when a scope is specified in the refresh token flow.

from node-oauth2-server.

@jorenvandeweyer is this still an issue?

from node-oauth2-server.

Yes this is still an issue and it is not that easy to fix.

1. We should check our scope input better

scope       = scope-token *( SP scope-token )
scope-token = 1*( %x21 / %x23-5B / %x5D-7E )

regex: ^[\x21\x23-\x5B\x5D-\x7E]+(?:\s+[\x21\x23-\x5B\x5D-\x7E]+)*$

2. We should always parse the scope into an array of strings, this would be breaking.

3. Then we can validate the scope input against the original scope with something like this

const previousScopes = ['user:read', 'user:write']
const newScopes = ['user:read']

const valid = newScopes.every(newScope => previousScopes.includes(newScope))

from node-oauth2-server.

We should always parse the scope into an array of strings, this would be breaking

If it's entirely internal (transforming any incoming scope to an array before furhter) I think it's at least non-breaking for "the outside", right?

I think this should be implemented in the formats package, what do you think?

from node-oauth2-server.

I think we should also change it for the outside.

would become:

validateScope?(user: User, client: Client, scope: string[]): Promise<string[] | Falsey>;

which means that all implementations of validateScope functions should return a string instead of an array of strings.

from node-oauth2-server.

Then let's target this for 5.x and mark it breaking

from node-oauth2-server.