Skip to content

Conversation

@ChristophWurst
Copy link
Member

I found a bug in the nextcloud-axios (now @nextcloud/axios) package: we read the OC.requesttoken just once. But the token can actually change over time.

As there is no simple solution to intercept every request with Axios (only PUT, POST, DELETE and PATCH), I updated the package (and @nextcloud/auth) to listen to an event on the new event bus.

The server now emits the events so that other modules that need the token can stay in sync.

This PR is split into two parts

  • Emit the CSRF token change event
  • Update to @nextcloud/axios which listens to the event (via @nextcloud/auth) and thus always uses the correct token

To test this PR you can either wait for the CSRF token to expire or just run _nc_event_bus.emit('csrf-token-update', {token: 123}). On the apps page, for example, you should see 412 errors if you emit an invalid token.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

I like it!

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 4, 2019
@skjnldsv skjnldsv force-pushed the fix/axios-csrf-token-update branch from fc91769 to 329059d Compare October 5, 2019 08:27
@skjnldsv
Copy link
Member

skjnldsv commented Oct 5, 2019

/compile amend /

@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the fix/axios-csrf-token-update branch from 329059d to 12378f4 Compare October 5, 2019 09:52
@skjnldsv
Copy link
Member

skjnldsv commented Oct 5, 2019

modified: apps/accessibility/js/accessibility.js
12 | modified: apps/accessibility/js/accessibility.js.map
13 | modified: apps/oauth2/js/oauth2.js
14 | modified: apps/oauth2/js/oauth2.js.map
15 | modified: apps/settings/js/vue-settings-admin-security.js
16 | modified: apps/settings/js/vue-settings-admin-security.js.map
17 | modified: apps/settings/js/vue-settings-apps-users-management.js
18 | modified: apps/settings/js/vue-settings-apps-users-management.js.map
19 | modified: apps/settings/js/vue-settings-personal-security.js
20 | modified: apps/settings/js/vue-settings-personal-security.js.map
21 | modified: apps/twofactor_backupcodes/js/settings.js
22 | modified: apps/twofactor_backupcodes/js/settings.js.map
23 | modified: apps/workflowengine/js/workflowengine.js
24 | modified: apps/workflowengine/js/workflowengine.js.map
25 | modified: core/js/dist/login.js
26 | modified: core/js/dist/login.js.map
27 | modified: core/js/dist/main.js
28 | modified: core/js/dist/main.js.map
29 | modified: core/js/dist/maintenance.js
30 | modified: core/js/dist/maintenance.js.map

I don't understand why 🤔

@skjnldsv skjnldsv force-pushed the fix/axios-csrf-token-update branch 2 times, most recently from 727184f to 8167caa Compare October 5, 2019 16:37
@skjnldsv
Copy link
Member

skjnldsv commented Oct 5, 2019

I don't understand why

Let's try if the lockfile was an issue, since tests are using npm ci but we aren't in the makefile nor the npmbuildbot 🤔

EDIT: the answer is no ^^'

11 | modified: apps/accessibility/js/accessibility.js.map
12 | modified: apps/oauth2/js/oauth2.js
13 | modified: apps/oauth2/js/oauth2.js.map
14 | modified: apps/settings/js/vue-settings-admin-security.js
15 | modified: apps/settings/js/vue-settings-admin-security.js.map
16 | modified: apps/settings/js/vue-settings-apps-users-management.js
17 | modified: apps/settings/js/vue-settings-apps-users-management.js.map
18 | modified: apps/settings/js/vue-settings-personal-security.js
19 | modified: apps/settings/js/vue-settings-personal-security.js.map
20 | modified: apps/twofactor_backupcodes/js/settings.js
21 | modified: apps/twofactor_backupcodes/js/settings.js.map
22 | modified: apps/workflowengine/js/workflowengine.js
23 | modified: apps/workflowengine/js/workflowengine.js.map
24 | modified: core/js/dist/login.js
25 | modified: core/js/dist/login.js.map
26 | modified: core/js/dist/main.js
27 | modified: core/js/dist/main.js.map
28 | modified: core/js/dist/maintenance.js
29 | modified: core/js/dist/maintenance.js.map

Signed-off-by: Christoph Wurst <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@skjnldsv skjnldsv force-pushed the fix/axios-csrf-token-update branch from 8167caa to 81613d2 Compare October 5, 2019 17:06
@skjnldsv
Copy link
Member

skjnldsv commented Oct 5, 2019

Built it again and suddenly it works --"

@skjnldsv skjnldsv merged commit 5c21d11 into master Oct 5, 2019
@skjnldsv skjnldsv deleted the fix/axios-csrf-token-update branch October 5, 2019 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants