Resolve #144 & avoid misusing Refresh-token as Auth-token for Authorisation

[ModulesSoft]

144 #136 ...

Bug:

If we use refresh token instead of auth/access token in request headers (for the ones which need authorization), it works! And it doesn't matter how long we use it.

This is also prone to MITM attacks when using http protocol.

Bug fix:

This update prevents (mutation) execution for requests which have refreshToken set in their header, instead of accessToken .

Feel free to comment

[hatsumatsu]

Chiming in here to add that this is a breaking change for existing projects. As mentioned in #136 the Wordpress example in the offical next.js repo uses refreshTokens to preview draft posts so I guess a number of projects adopted this approach. Would be a bummer if an update breaks these sites.

[ModulesSoft]

I thank you for your comment in first place. I agree many projects may have been adopted to old approach but this doesn't mean they don't have vulnerabilities. I can suggest creating another branch for my update to consider both approaches, then anyone can choose which one to use.

[hatsumatsu]

@majhoolsoft thx for your feedback on this.
Regarding your PR: What are the new permissions of the refreshToken? Can it only request a new authToken? Or does it allow querying published and/or draft posts? (Since querying draft posts is not a mutation...)

[ModulesSoft]

Yes. refresh token will no longer be accepted for mutation requests. Refresh token should be handled by client side to get new auth token, every time it expires. Then use the new auth token for further mutation requests.

[ModulesSoft]

This is the article I wrote to instruct developers who may need to know more about my solution.

[montchr]

The longer the Next.js example promotes the bad practice allowed by this bug, I suspect it will become more difficult for maintainers to take action on it later. And, well, considering that 2023 is quite a while "later" already – if the PR had been merged in 2021 or early 2022, would things have been different?

As an upstream dependency of the official Next.js cms-wordpress example, I would suggest that that example's reliance on the ~behavior~ ~bug~ vulnerability this PR aims to fix should be clearly flagged and deprecated as soon as possible within the plugin itself despite the breaking changes. Such a change should, of course, be communicated to users very prominently (perhaps in bold text at the top of this plugin's Readme file and an error message propagated through Composer if Composer is capable of doing so). A semver minor version increment would probably also be in order since the plugin still is not 1.0.0. And, considering that too – if the project follows semver – users should not have any expectation that minor version increments will be free of breaking changes.

I almost started following the cms-wordpress example, but I then became extremely wary of its reliability after reading the following issue and its resulting (automated?) dismissal. I'm not sure if the original author was referring to the same bug, but it does seem related:

examples/cms-wordpress preview role-based security vulnerability · Issue #29877 · vercel/next.js

It looks like the example setup for WordPress allows lower privileged WordPress users (like authors) to view unpublished content from all users (as an administrator).

I believe WordPress has a role-based approach to drafts. I.e. authors should only see their drafts. If user's use the link recommended to see drafts by appending secret= <secret> in the URI query, they'll have the privileges of an administrator because the recommended refreshToken is from an Administrator.

And the complete response from Vercel before locking the issue comments:

This issue has been automatically closed because it wasn't verified against next@canary. If you think it was closed by accident, please leave a comment. If you are running into a similar issue, please open a new issue with a reproduction. Thank you.

So I'm not sure that Vercel cares much about investigating or fixing the example without a "verifiable reproduction". Somewhat understandable considering the scope of their project, but the opaque dismissal of https://github.com/vercel/next.js/issues/29877 concerns me nonetheless.

Introducing a breaking change in this plugin might probably result in a "verifiable reproduction" of a failing example downstream, and thus hopefully prevent further proliferation of unquestioned reliance on #144 as a feature instead of a bug.

[hatsumatsu]

The more I think about this the more I conclude that whether this is a bug / security issue depends on how the tokens are generated and used.

I agree that using a long-lived token with admin privileges is not safe. However, there a many real world use cases where the frontend does not need to be able to do mutations but instead needs read-only access to unpublished posts for live previewing. In this scenario, using a long-lived token with limited permissions (such as WP's editor role) would be acceptable, much easier to implement in the frontend, and in line with how other headless CMSs deal with this topic.

That's why I suggest–instead of changing the current behavior–to clarify in the readme that both authToken and refreshToken share the permissions of the user used in the login mutation and differ only in their lifespan. Then it would be up to the user which type of token to use and which user to base the token on. You could f.e. create a separate preview user with limited permissions to allow live previewing CMS data like in the nextjs example.

[hatsumatsu]

I also wonder how big the security gain would be with your proposed change: If an attacker gains access to the refreshToken, they could easily use the refreshJwtAuthToken mutation to obtain a valid authToken. The attack surface to me looks very similar to using the actual authToken.