-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Back Button Security Concerns #247
Comments
For reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control Specifically this directive: |
Hi, Unless I'm mistaken, this really is not a bug, and
However, this specification is outdated, and has been replaced by https://tools.ietf.org/html/rfc7234#section-6, which states:
While this does mention that the freshness model does not necessarily apply, it doesn't explicitly tell that the history mechanism should be expected to honor cache directives either, so, instead, we'll take a closer look at Section 4.2:
Let's see if the
So, in conclusion, it's not really a bug, because history mechanisms are not http caches. THAT SAID, since people keep complaining about it, and since we (Inertia) make HTTP requests and use History state as a cache of sorts, we might actually want to consider respecting the
|
While I don't consider the current behaviour of Inertia.js to be "broken", I do understand why people want this behaviour. We could update Inertia to only save the page object in the event that the I wonder if there is a simpler way to handle this problem. Something like this: // Track if the user is logged in (in localStorage) after every Inertia visit.
// This can be done using the Inertia "success" event, and by inspecting the page props.
Inertia.on('success', event => {
window.localStorage.setItem('loggedIn', event.detail.page.props.auth.user !== null)
})
// When the user navigates, check if they are logged in.
// If they are not, stop the propagation to prevent Inertia from restoring the page.
// Then, instead, redirect them to the login page.
window.addEventListener('popstate', event => {
if (window.localStorage.getItem('loggedIn') === 'false') {
event.stopImmediatePropagation()
window.location.href = '/login'
}
}) The only hiccup with this approach is that this will also prevent navigating to other public pages. You could manually check for that though: window.addEventListener('popstate', event => {
// Only prevent history navigation for private pages.
const publicUrls = ['/login', '/reset-password']
if (!publicUrls.includes(event.state.url) && window.localStorage.getItem('loggedIn') === 'false') {
event.stopImmediatePropagation()
window.location.href = '/login'
}
}) And of course, you could get fancy with this, and automatically mark pages as "private" server-side in your page props: window.addEventListener('popstate', event => {
// Only prevent history navigation for private pages.
if (event.state.props.private && window.localStorage.getItem('loggedIn') === 'false') {
event.stopImmediatePropagation()
window.location.href = '/login'
}
}) This actually achieves the desired behaviour very well. The biggest "issue" I see here is that for every click back in history (after logging out), this "eats" one of the history entries. This is because it's NOT possible to prevent ( |
I really appreciate your time spent looking into this You both have a much better idea that I do as to what would work best in the context of inertia. So I can't really add anything to that discussion. I don't want to speak for anyone else, but I think anyone who is looking for this behavior would be ok with a few trade offs. If an app doesn't have "cache-content: no-store" headers, I imagine it would have no impact at all. Let me know if I can help test or anything else. |
Hello everyone, I seem to also have a problem with this one. It goes back to dashboard even after log out. I tried the solution of @reinink but it didn't work for me. Any ideas on how to work around this? |
I think I have found a work around. I just modified the code of @reinink to my own preference. Thanks. |
I've continued to explore this from the server side which would really be the ideal place to solve it, but unfortunately no combination of headers seems to get the browser (Chrome in my current testing) to behave as expected. I decided to give it a quick poke based on the snippets @reinink shared above and was able to get something that roughly matches my goals, specifically:
let lastRequestMethod = null
Inertia.on('start', (event) => {
lastRequestMethod = event.detail.visit.method
})
window.addEventListener('popstate', (event) => {
if (lastRequestMethod !== 'get') {
event.stopImmediatePropagation()
window.location.href = event.state.url
}
}) It only adds this behavior for the single preceding request, but I believe it would degrade to the normal browser behavior, so I may be able to convince myself that this is enough and is a tolerable amount of mucking about in core browser logic. |
Please place this information in the documentation on your site. |
Can you please share your code what changes did you make, and where should I place the code in the layout file or app.js? |
I've run across this issue, and was also able to replicate it by logging out (post request) and then hitting back to be logged in again. I actually have it in a different context, where we have a processing screen that shows, and customers hitting back get confused and think things are being double processed. Is there any way to disable the caching completely, or on a per request basis? |
@reinink I've tried out this method and it kinda works for the first time i hit the back button. I get to see the page for like a second, and then go to the login page. Anything more i can try? |
Any new approach till now? |
@reinink That worked perfectly for me. I'm using Ziggy with Laravel and I was able to use this solution to verify where a user should be based on a globally accessible variable in Vue and by looking at named routes prefixed with a common name: // safety for checking for valid login session
window.addEventListener('popstate', e => {
const route = this.route()._unresolve(e.state.url);
const authRoute = /^auth\./i;
if (!route || !route.name) {
return;
}
if (this.$app.loggedIn && authRoute.test(route.name)) {
e.stopImmediatePropagation();
this.$inertia.visit(this.route('home'));
} else if (!this.$app.loggedIn && !authRoute.test(route.name)) {
e.stopImmediatePropagation();
this.$inertia.visit(this.route('auth.login'));
}
}); |
Hi all My team and I managed to solve it.For context our app only has one public route //app.js
window.addEventListener('popstate', async (e) => {
e.stopImmediatePropagation();
try{
// to check if application session is still valid
const res = await fetch('/dashboard/state')
if(res.ok){
window.location.replace(e.state.url);
}
}catch(e){
window.location.replace("/dashboard/user/login");
}
},) Hope the information above is helpful! |
Reloading the browser after clicking the back button ensures that pages are still accessible. This is my trick: window.addEventListener('popstate', function (event: PopStateEvent): void {
location.reload()
}); |
Hey folks, I don't anticipate us making any changes to how Inertia handles this, but I think my original workaround is still valid, so if this is something you're looking for I'd recommend trying that approach 👍 |
Hi @reinink, thank you for sharing your workaround. I appreciate the effort put into finding solutions. However, after examining it, I believe there are still significant flaws that need addressing. While the suggested workaround attempts to mitigate the issue, it's important to highlight a potential vulnerability that remains. With JavaScript disabled after logging out, sensitive information can still be extracted from the browser's history. This poses a serious concern, especially in scenarios where users are accessing sensitive data from non-trusted computers. Imagine a scenario where a user logs out from a shared or non-trusted computer assuming their data is safe. If a malicious actor exploits this flaw, they could easily turn off JavaScript and access all the props from the previous pages, compromising the user's privacy and security. In my opinion, the suggested solution to check for protected pushState(page: Page): void {
this.page = page;
const deepcopy = JSON.parse(JSON.stringify(page));
deepcopy.props = {};
window.history.pushState(deepcopy, "", page.url);
}
protected replaceState(page: Page): void {
this.page = page;
const deepcopy = JSON.parse(JSON.stringify(page));
deepcopy.props = {};
window.history.replaceState(deepcopy, "", page.url);
}
protected handlePopstateEvent(event: PopStateEvent): void {
if (event.state !== null) {
const page = event.state;
const visitId = this.createVisitId();
Axios({
method: "get",
url: hrefToUrl(page.url).href,
headers: {
Accept: "text/html, application/xhtml+xml",
"X-Requested-With": "XMLHttpRequest",
"X-Inertia": true,
...(page.version
? { "X-Inertia-Version": page.version }
: {}),
},
})
.then((response) => {
if (!this.isInertiaResponse(response)) {
return Promise.reject({ response });
}
const pageResponse: Page = response.data;
Promise.resolve(
this.resolveComponent(pageResponse.component),
).then((component) => {
if (visitId === this.visitId) {
if (
hrefToUrl(pageResponse.url).href !==
window.location.href
) {
window.location.href = pageResponse.url;
return;
}
this.swapComponent({
component,
page: pageResponse,
preserveState: false,
}).then(() => {
this.restoreScrollPositions();
fireNavigateEvent(pageResponse);
});
}
});
})
.catch(() => {
// redirect to the location
window.location.href = page.url;
});
} else {
const url = hrefToUrl(this.page.url);
url.hash = window.location.hash;
this.replaceState({ ...this.page, url: url.href });
this.resetScrollPositions();
}
} I believe adopting a solution like this would better safeguard user data and address the concerns raised. What are your thoughts on this approach? |
@prattcmp having such a passive/aggressive tone gets us nowhere. Instead of complaining why don't you help out by attempting a PR that could both solve the issue and still takes into account the concerns from the core team? |
Hey folks, just left some comments about this particular concern in a related PR: #1784 (comment) Hoping to find a way to address this 👍 |
Thanks a lot @reinink! |
I believe there may be some issues around the way Inertia handles it's ajax calls in the presence of Cache-Control headers that are put in place for security. See the link below for the closed issue where this is discussed.
The basic premise of cache control headers is that if you add 'no-cache, no-store' to cache control the app will require a full page reload even when a user clicks on the back button in the browser. This is a great way to prevent sensitive data from being displayed after a user logs out of an application and the back button is clicked. Since the app now has to make a full server request due to no cache history, the information will not be displayed and the user will be redirected back to the login page by the server.
When using Inertia this behavior is not present. If you log into an app with Inertia where the Cache-control headers are present to protect back button clicks, logout and press the back button you still see the sensitive pages.
Is there a way to check for this header and redirect to login if the session is no longer valid?
Originally posted by @drfraker in #102 (comment)
The text was updated successfully, but these errors were encountered: