Security: explicit string requirement for request parameters (client id, tokens etc.)

[jankapunkt]

Summary

This updates @node-oauth/formats to 1.1.0 which adds an explicit assertion for string types on every format check.
Before this, it was possible that the following values were passing format checks, such as .vschar:

null, undefined, true, {}, () => {}

Therefore, some tests were passing, although they used client ids or token as number values.
With this update this is not possible anymore, every value that needs to be a string is now explicitly required to be a string.

Please note security implications of this.

The fix is non-breaking in terms of the standard specifications or intended behavior.

Linked issue(s)

node-oauth/formats#3

Involved parts of the project

tests, isFormat (dependency)

Added tests?

yes, all upgraded

OAuth2 standard

relates to all involved RFCs that define an allowed character range for request parameters

[Copilot]

Pull request overview

This PR updates the OAuth2 server to use @node-oauth/formats v1.1.0, which enforces string types for format validation, and aligns all tests and redirect URI validation logic with the stricter type requirements.

Changes:

• Bumped @node-oauth/formats to ^1.1.0 and relaxed the mocha devDependency to a caret range, updating the lockfile accordingly.
• Updated unit and integration tests so client IDs, authorization codes, access tokens, and refresh tokens are passed and asserted as strings instead of numbers.
• Hardened AuthorizationCodeGrantType.validateRedirectUri to explicitly require a non-empty redirect_uri string before running URI format validation, preventing runtime errors with the new formats library while keeping RFC-compliant behavior.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
lib/grant-types/authorization-code-grant-type.js	Tightens redirect URI validation to guard against missing values before applying string-only URI checks from the updated formats library.
test/unit/handlers/token-handler_test.js	Adjusts unit tests for TokenHandler to use string client IDs and expectations consistent with string-only format checks.
test/unit/handlers/authorize-handler_test.js	Updates AuthorizeHandler unit tests to treat client_id as a string in request bodies and assertions.
test/unit/grant-types/authorization-code-grant-type_test.js	Ensures authorization codes and related values are strings in unit tests and updates PKCE-related tests accordingly.
test/integration/server_test.js	Aligns integration tests around the Server class with string client IDs and authorization codes.
test/integration/handlers/token-handler_test.js	Converts numeric client IDs, codes, and refresh tokens in token handler integration tests to strings to match new validation behavior.
test/integration/handlers/authorize-handler_test.js	Uses string client_id values consistently in authorize handler integration tests and keeps redirect URI error-path coverage in place.
test/integration/grant-types/refresh-token-grant-type_test.js	Switches client IDs and refresh tokens in refresh token grant integration tests from numbers to strings, preserving all behavioral expectations.
test/integration/grant-types/authorization-code-grant-type_test.js	Updates authorization-code grant integration tests for string codes/IDs and extends coverage around redirect URI validation under the new rules.
package.json	Bumps @node-oauth/formats to ^1.1.0 and changes mocha to ^11.7.5 to allow compatible patch/minor updates.
package-lock.json	Regenerates the lockfile to reflect the new @node-oauth/formats version, updated tooling (Rollup, Vue, Shiki, etc.), and removal of unused dev-time AI/docsearch-related dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dhensby]

The change in the tests from numbers to strings makes me a little nervous as that can be indicative of some breaking behaviour change. I get that most of these tests are just mock values and perhaps were invalid to begin with, but are we confident this doesn't break existing behaviours in the wild?

Request bodies should be application/x-www-form-urlencoded format, right? So everything is a string - so assuming there's not funky middleware happening, the request bodies should validate without a problem; but there is also some change to function return values (eg: getAuthorizationCode() return values have changed).

Are these requirements reflected in the typings? edit: I see the typings already have these down as strings

[jankapunkt]

@dhensby I definitely would not rule out possible breaks. However, from what I saw the tests were not covering the real world scenario behavior and this is on most cases strings being used for client IDs and tokens.

In order to validate non breaking behavior we can publish a release candidate and communicate that changes are not likely breaking but maybe. RC are not installed by default and need explicit version being added to npm install.
In my experience there only few adopters who really make use of it, though.

but there is also some change to function return values (eg: getAuthorizationCode() return values have changed)

Can you please comment the lines that changed the return values? The PR was not intended to change anything beyond tests. The only change in the code is to verify redirectURL the same way it was done before.

[dhensby]

I definitely would not rule out possible breaks. However, from what I saw the tests were not covering the real world scenario behavior and this is on most cases strings being used for client IDs and tokens.

Yeah - that's what I was thinking - the previous tests just happened to work but weren't really true to life

Can you please comment the lines that changed the return values? The PR was not intended to change anything beyond tests. The only change in the code is to verify redirectURL the same way it was done before

Sorry, I meant it was just in tests that the stubbed functions had their returns changed (and my point being this was not incoming requests that would be string, but had the potential to be loaded from a database where the types could be numeric).

---

I think we have two options; accept this as-is and release it with a warning that it may cause breaking changes for those implementing outside the expected types... or we could make the validator coerce some values to strings where appropriate (ie: if it's a number, can we coerce it to a string and check for loose equality to provide some attempt at backward compatibility where the distinction between "1234" (string) and 1234 (number) is of no great importance. The latter adds some complexity and we need to be careful of undesirable behaviour, but would make us more permissive when some in use with some other typing system.

This is what a request could look like:

client_id=123&client_secret=abc&grant_type=client_credentials

The client_id URL encoding means all of these are strings, but there could be another layer that is loading this from the database and returning the client_id as a number and I think it's overly onerous to expect people to be coercing that to a string for us... The same applies if the request body has also been coerced before being sent in to the library.

[jankapunkt]

@dhensby I think backwards compatible coercion is doable but I would strictly restric it to numbers, what do you think?

[jankapunkt]

@dhensby checked all code and none of the values, returned from the model are validated against the formats. This also aligns with the standard, that defines these validations only for incoming requests.

I know we don't want to burden users with coercion but the request arguments must be string according to the standard. I found a non-backwards-breaking way and also found even more non-standard behavior which will make this breaking anyway:

currently we validate parameters as "Missing" when they are falsy, however, false, '', 0 are obviously not a missing, but "Invalid" parameters.

For this I currently extend this PR with the followng:

• throw "Missing" error only if params are undefined or null
• explicitly check, if params, that might be represented by numbers (ids, tokens) and convert to string
• validate these string params using the formats library
• throw on all passed params that are of other types, such as {}, [], () => {}, Symbol
• remaining question: is empty string a missing param, an invalid param or should the model decide?