Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address history back button security issue (full page loads) #1784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicholaspufal
Copy link

@nicholaspufal nicholaspufal commented Feb 1, 2024

Disclaimer: I'm open to ideas on how to address this in a better way here. This isn't the same as the popstate custom handler people have proposed in other issues/PRs.

Summary of the problem

From a security standpoint, there are two situations where Inertia restores props from its cache (i.e. window.history.state) that are concerning at the moment:

  1. During client-side navigation (e.g. after clicking on an InertiaLink component) a user goes back in the browser's history
  • This relies on the popstate API to restore the app's state, has been discussed in a few issues in this project, and it's not what this PR is about. Still warrants attention though.
  1. After full page loads a user goes back in the browser's history
  • This is what this PR is about

👨‍💻 This is the code affecting behavior related to number 2 from above: https://github.com/inertiajs/inertia/blob/master/packages/core/src/router.ts#L157-L163

  • page here represents fresh Inertia props. When a backend request is performed, and Inertia props are provided via the rendered HTML, this is what page here includes
  • window.history.state on the other hand represents stale props. This is the state of the Inertia props prior to what page contains.

The issue is that the code is always giving preference to those stale props. Because Inertia props are the bread and butter of Inertia, they often contain sensitive information from the users, information which is cleared when they log out, but because Inertia pulls those from its cache (without any sort of configuration around this behaviour, or without checking if page's props may be fresher) sensitive information may be leaked to third-parties in Inertia apps.

Reproducing this problem

from an Inertia app

  1. User A is logged in and at the path /foobar
  2. User A signs out and this causes a full /foobar page load
  3. User A leaves the shared computer
  4. User B goes back in the browser's history
  5. User B can see all sensitive information from User A

@zcuric
Copy link

zcuric commented Feb 7, 2024

@nicholaspufal Hi, does this also fixes the undefined issue that I've address here #1786?

@nicholaspufal
Copy link
Author

@zcuric I'm not sure, I have never encountered that scenario tbh. If you have a solid way to reproduce that, could you use my branch and test it out please?

@zcuric
Copy link

zcuric commented Feb 8, 2024

@nicholaspufal It is fixed, I just tested it in my project!

So to reproduce it, on the master branch:

  1. Go to a homepage, for example
  2. Go to another page
  3. Click back and you should see something like this:
    image

This is caused by the same issue you fixed. My PR #1786 addresses just above mentioned issue. Your PR needs to be merged, especially if it's a security issue. I hope someone will look at this. 🙏🏻

@Hasan-Mir
Copy link

Just wanted to mention that this PR fixes the useRemember's bug #1766 as well.

@nicholaspufal
Copy link
Author

@jessarcher @reinink given comments above, please let me know if there is anything you need me to do to expedite this — and if you are ok with the approach here.

Thank you!

@alucic
Copy link

alucic commented Feb 14, 2024

@jessarcher Please let me know if I can assist somehow to get this PR out, this would help us a lot. Every single back button is throwing an error and spamming our Sentry channel. The error is generic Error so it is hard to archive it or ignore it. Thanks

@zcuric
Copy link

zcuric commented Feb 21, 2024

Just so you know, we used https://github.com/ds300/patch-package to apply patches to @inertiajs/core to fix these two issues. We applied, this PR and #1786.

@WillRy
Copy link

WillRy commented May 7, 2024

This security flaw is serious, it even prevented me from using inertia js in a company. I was banned from using it due to this security issue.

@jessarcher @reinink

There are other issues that mention the problem, but all the solutions so far are workarounds.

Look this others issues:

@stefanocurnis
Copy link

Please guys, this is serious. Any chance to bring attention to this via @lepikhinb ?

@reinink
Copy link
Member

reinink commented May 28, 2024

Hey folks, just want to let you know that this is on our radar and we've been having discussions internally on how to best handle signing out of an Inertia.js app and clearing all the client-side history.

Unfortunately the browser's history APIs don't make this easy — there is no way to clear history state data. From the research I've been doing I think our best bet is to simply track a unique visit ID in the history state and then save the actual page data in localStorage, and then provide some type of mechanism within the Inertia router to clear all this history when logging out of your app. Possibly something like this:

import { router } from `@inertiajs/react`

router.clearHistory()

Then when a user goes back or forward in history Inertia can check if that unique visit ID exists in the history state (in local storage), and if it doesn't it can perform a full inertia visit — and if the user has been logged out they'll then be redirected to the login page.

We might even be able to add some sort of server-side header that does this client-side history clearing automatically doing something like this:

public function logout(Request $request): RedirectResponse
{
    Auth::logout();

    // ...

    Inertia::clearHistory();

    return redirect('/');
}

That's my working idea right now at least. Need to actually prototype this and see how it works. It's a little scary switching from history state to local storage for page data storage, but maybe it will just work 🤞

As for this particular PR, I'm not 100% sure it's the right change. Yes, it is a partial fix to the security concerns being addressed in #102 and #247, but it's not the right solution for non-auth related situations where you do want to use the data in the history state when going back or forward in your history. Just because it's a "full page" history visit doesn't necessarily mean it should read fresh data from the server. Anyway, going to leave this PR open either way as I may change my mind on this while working on these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application Crash with useRemember Hook During Browser History Navigation
7 participants