Skip to content

fix: csrf check failed on public share with password - #44369

Merged
juliusknorr merged 3 commits into
masterfrom
bugfix/csrf-failed-on-public-share-with-password
Apr 2, 2024
Merged

fix: csrf check failed on public share with password#44369
juliusknorr merged 3 commits into
masterfrom
bugfix/csrf-failed-on-public-share-with-password

Conversation

@luka-nextcloud

@luka-nextcloudluka-nextcloud commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

Summary

"CSRF check failed" on public share with password

TODO

  • ...

Checklist

@luka-nextcloudluka-nextcloud self-assigned this Mar 20, 2024
@solracsfsolracsf added this to the Nextcloud 29 milestone Mar 21, 2024
@solracsfsolracsf added the 3. to review Waiting for reviews label Mar 21, 2024
@AltahrimAltahrim mentioned this pull request Mar 21, 2024
Comment threadcore/js/publicshareauth.js Outdated
Comment on lines +57 to +69
document.addEventListener('DOMContentLoaded', function() {
var form = document.getElementById('password-input-form');
if (form) {
form.addEventListener('submit', async function(event) {
event.preventDefault();
var requestToken = document.getElementById('requesttoken');
if (requestToken) {
requestToken.value = await OC.fetchRequestToken();
}
form.submit();
});
}
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to move this into a "modern" js module that goes through webpack? then you can import @nextcloud/router directly and we avoid adding a new property to the dated OC global

@ChristophWurst
ChristophWurst requested review from a team, Pytal, emoral435 and szaimen and removed request for a teamMarch 21, 2024 18:09
@AltahrimAltahrim mentioned this pull request Mar 25, 2024
Comment threadcore/src/main.js
Comment on lines +63 to +67
if (requestToken) {
const url = generateUrl('/csrftoken')
const resp = await Axios.get(url)
requestToken.value = resp.data.token
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use grab the currently known CSRF token from @nextcloud/auth to avoid the additional request?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is for solving issue that the currently known CSRF token might not be the latest.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an existing CSRF token update mechanism that pulls a fresh token every 30 seconds. Is that not sufficient?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not sufficient. User might submit form during the gap time and see the CSRF failed error randomly.

@emoral435emoral435 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a valid solution to me - however, haven't tested it fully

@skjnldsvskjnldsv mentioned this pull request Mar 28, 2024
81 tasks
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud
luka-nextcloudforce-pushed the bugfix/csrf-failed-on-public-share-with-password branch from 6d5b7ea to a42c68dCompareMarch 29, 2024 08:51
@juliusknorr
juliusknorr merged commit 31c6379 into masterApr 2, 2024
@juliusknorr
juliusknorr deleted the bugfix/csrf-failed-on-public-share-with-password branch April 2, 2024 07:59
@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

@juliushaertl@ChristophWurst@Altahrim This PR was missed from the release.

@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

/backport to stable29

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[Bug]: CSRF check failed on public share with password

6 participants

@luka-nextcloud@ChristophWurst@emoral435@blizzz@juliusknorr@solracsf