Scope in refresh_token grant type is ignored.

[jorenvandeweyer]

While checking the compliance of the refresh_token grant. I discovered that the optional scope parameter in the body is ignored.

node-oauth2-server/lib/grant-types/refresh-token-grant-type.js

Lines 64 to 73 in 1b91ddc

	return Promise.bind(this)
	.then(function() {
	return this.getRefreshToken(request, client);
	})
	.tap(function(token) {
	return this.revokeToken(token);
	})
	.then(function(token) {
	return this.saveToken(token.user, client, token.scope);
	});

https://datatracker.ietf.org/doc/html/rfc6749#section-6

[jankapunkt]

@jorenvandeweyer great you found this!

[Uzlopak]

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.

[jankapunkt]

@jorenvandeweyer is this still an issue?

[jorenvandeweyer]

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))

[jankapunkt]

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?

[jorenvandeweyer]

I think we should also change it for the outside.

node-oauth2-server/index.d.ts

Line 385 in 48baa8b

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

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.

[jankapunkt]

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