Merge development into master to recover lost #390 fix#441
Conversation
…e-revoke fix(refresh-token): validate scope before revoking token node-oauth#390
The refresh-token "validate scope before revoking" fix (PR node-oauth#389, issue node-oauth#390) was merged into `development` and released as 5.2.2-rc.0, but `development` was never merged back into `master`. As a result 5.3.0 shipped from `master` without the fix, and a refresh request with an invalid scope still revokes the token before scope validation, leaving the client unable to refresh. `development` had since fallen 47 commits behind `master`. This merge brings `development`'s history back into `master`, resolving all conflicts in favour of `master` (keeping lib/model.js, the VitePress `.md` docs, refactors and the 5.3.0 version/lockfile). The only net change versus master is the node-oauth#390 fix and its accompanying test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR restores the fix for issue #390 on master by adjusting the refresh-token grant flow so scope validation happens before refresh token revocation, preventing refresh tokens from being destroyed when an invalid/extra scope is requested during a refresh.
Changes:
- Reordered refresh-token handling to validate requested scope before calling
revokeToken(). - Added an integration test that asserts an
InvalidScopeErroris thrown for extra scopes and that revocation/saving do not occur in that failure case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/grant-types/refresh-token-grant-type.js | Validates scope prior to token revocation during refresh-token grant handling to avoid destroying tokens on scope errors. |
| test/integration/grant-types/refresh-token-grant-type_test.js | Adds coverage ensuring extra/invalid refresh scope fails with InvalidScopeError without revoking or saving a new token. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jankapunkt
left a comment
jankapunkt
left a comment
There was a problem hiding this comment.
Thank you very much for pointing this out! We should discuss how to continue after this merge with getting master and development on par again.
|
If we move to semantic releases, they define some sane defaults for where pre-release/next development goes. |
What
Merges the
developmentbranch back intomasterto recover the refresh-token "validate scope before revoking" fix that was lost between releases.Closes #390.
Why
The fix for #390 was made in #389, merged into
development, and released as5.2.2-rc.0. Howeverdevelopmentwas never merged back intomaster, so when5.3.0was cut frommasterit shipped without the fix. The bug is therefore live in the current5.3.0release: arefresh_tokenrequest that asks for an invalid/extra scope revokes the token before scope validation runs, so validation fails after the token has already been destroyed — permanently breaking refresh for that client. (Reported again on #390 on 2026-06-11.)How
developmenthad fallen 47 commits behindmaster(which since addedlib/model.js, migrated the docs from Sphinx.rstto VitePress.md, and refactored broadly). A straight merge would have tried to revert all of that, so the merge was resolved entirely in favour ofmaster, letting only the #390 change through:package.json/package-lock.json→ keptmaster(stays at5.3.0; the stray5.2.2-rc.0bump is discarded).master's state (git only conflicts where both sides changed a file;developmentnever touched master's new work).Net change versus
masteris exactly the fix + its test:The merge is a real merge commit (history preserved —
development's commits, including the original fix authored by @runeb, become ancestors ofmaster).Verification
lib/model.jspresent, 37.mddocs present, 0.rstfiles — no regression from the stale branch.npm run lintclean.should throw an error if extra \scope` is requested` test.Follow-up
After this lands,
developmentis fully contained inmasterand is otherwise stale. Recommend deleting it (or resetting it tomaster) to prevent the same drift/lost-fix situation recurring.