notes icon indicating copy to clipboard operation
notes copied to clipboard

Switching Note - Failed to push (empty) steps

Open theatischbein opened this issue 8 months ago • 16 comments

Steps to reproduce

  1. Open a note
  2. Change to another note
  3. See 403 error in JS console

Expected behaviour

Switching between notes should not generate an error.

Actual behaviour

Switching between notes triggers an 403 pushing step error.

Requests/Logs

Excerpt from the log console:

[ERROR] text: Failed to push the steps to the server ...
[ERROR] text: failed to write to document - not allowed Object { app: "text", uid: "xxx", level: 3 }
[ERROR] text: Unexpected Error "Failed to apply steps. Retry!" Object { app: "text", uid: "xxx", level: 3, error: Error }

Actual request body that fails:

XHRPOST
https://cloud.xxx.xxx/index.php/apps/text/session/542243/push
[HTTP/2 403  106ms]

	
awareness	"xxx"
baseVersionEtag	"6800baba35a17"
documentId	542243
filePath	""
sessionId	3620
sessionToken	"xxx"
steps	[]
token	null
version	71331

I notice that filePath and steps are empty. Die documentId in body and URL also equals the current open note before switching to new note.

Server

  • Notes app version: 4.12.0
  • Nextcloud version: 31.0.2
  • OS: Linux xxx 6.13.8-arch1-1 nextcloud/notes#1 SMP PREEMPT_DYNAMIC Sun, 23 Mar 2025 17:17:30 +0000 x86_64 GNU/Linux
  • Web server (from outside to nextcloud):
  • PHP version: PHP 8.3.19 (cli)
  • Database: 11.7.2-MariaDB-ubu2404-log

Nextcloud configuration:

``` { "system": { "instanceid": "***REMOVED SENSITIVE VALUE***", "passwordsalt": "***REMOVED SENSITIVE VALUE***", "secret": "***REMOVED SENSITIVE VALUE***", "trusted_domains": { "1": "owncloud.xxx.xxx", "2": "cloud.xxx.xxx", "3": "onlyoffice.xxx.xxx", "4": "cloud" }, "trusted_proxies": "***REMOVED SENSITIVE VALUE***", "forwarded_for_headers": [ "HTTP_X_FORWARDED_FOR" ], "datadirectory": "***REMOVED SENSITIVE VALUE***", "dbtype": "mysql", "version": "31.0.2.1", "dbname": "***REMOVED SENSITIVE VALUE***", "dbhost": "***REMOVED SENSITIVE VALUE***", "dbtableprefix": "oc_", "dbuser": "***REMOVED SENSITIVE VALUE***", "dbpassword": "***REMOVED SENSITIVE VALUE***", "logtimezone": "Europe\/Berlin", "installed": true, "memcache.local": "\\OC\\Memcache\\APCu", "memcache.locking": "\\OC\\Memcache\\Redis", "memcache.distributed": "\\OC\\Memcache\\Redis", "redis": { "host": "***REMOVED SENSITIVE VALUE***", "port": 6379 }, "theme": "", "loglevel": 3, "maintenance": false, "mail_from_address": "***REMOVED SENSITIVE VALUE***", "mail_smtpmode": "smtp", "mail_domain": "***REMOVED SENSITIVE VALUE***", "mail_smtphost": "***REMOVED SENSITIVE VALUE***", "app_install_overwrite": [ "tasks", "documents", "ownnote", "richdocuments", "spreed" ], "mysql.utf8mb4": true, "simpleSignUpLink.shown": false, "lost_password_link": "disabled", "ldapIgnoreNamingRules": false, "ldapProviderFactory": "OCA\\User_LDAP\\LDAPProviderFactory", "updater.release.channel": "stable", "encryption.legacy_format_support": false, "encryption.key_storage_migrated": false, "encryption_skip_signature_check": true, "default_phone_region": "DE", "overwrite.cli.url": "https:\/\/cloud.riotcat.org", "overwriteprotocol": "https", "maintenance_window_start": 4, "log_rotate_size": 104857600, "apps_paths": [ { "path": "\/var\/www\/html\/apps", "url": "\/apps", "writable": false }, { "path": "\/var\/www\/html\/custom_apps", "url": "\/custom_apps", "writable": true } ], "mail_smtpauth": 1, "mail_sendmailmode": "smtp", "mail_smtpname": "***REMOVED SENSITIVE VALUE***", "mail_smtppassword": "***REMOVED SENSITIVE VALUE***", "memories.exiftool": "\/var\/www\/html\/custom_apps\/memories\/bin-ext\/exiftool-amd64-glibc", "memories.vod.path": "\/var\/www\/html\/custom_apps\/memories\/bin-ext\/go-vod-amd64", "enabledPreviewProviders": [ "OC\\Preview\\Image" ], "memories.gis_type": 1 } } ```

Client

Please complete the following information.

  • Browser (incl. version): Firefox 137.0 (64bit)
  • OS: Linux xxx 6.14.2-arch1-1 nextcloud/notes#1 SMP PREEMPT_DYNAMIC Thu, 10 Apr 2025 18:43:59 +0000 x86_64 GNU/Linux

theatischbein avatar Apr 17 '25 10:04 theatischbein

We can confirm this bug. Nextcloud (29.0.15 and 29.0.16) Note 4.12.0

I think this bug occurred with version 4.12.0, till version 4.11.0 all was fine.

Downgrading to Notes 4.11.0 solved the problem.

theroch avatar May 06 '25 07:05 theroch

The change in 3da9c1095fd777e6a0b011c562c29e47eac99f5f, which migrates to files:node:updated seems to be the cause of this issue.

Before this change text only emit('files:file:updated', { fileid: this.fileId }), now it seems to be files:node:updated is never thrown?

In my case is this.relativePath on line 522 always empty and so the fileNode is never initialized and is empty too:

if (this.currentSession?.userId && this.relativePath?.length) {
				const node = new File({
					id: this.fileId,
					source: generateRemoteUrl(`dav/files/${this.currentSession.userId}${this.relativePath}`),
					mime: this.mime,
				})
				fetchNode(node)Add commentMore actions
					.then((n) => { this.fileNode = n })
					.catch(err => logger.warn('Failed to fetch node', { err }))
			}

Can somebody confirm this?

theroch avatar Jun 23 '25 06:06 theroch

I confirm your information. I have added some trace to see that this.relativePath is always empty. So no "node" instantiation is possible.

So notes/src/components/NoteRich.vue needs to be patched to create editor with a valid "relativePath" attribute (adding "filePath: this.note.internalPath" to createEditor arguments).

gaudryc avatar Jun 24 '25 09:06 gaudryc

After patching notes/src/components/NoteRich.vue file (patch applied in notes/js/notes-main.js directly) to initialize editor with filePath attribute, the text app now fails while fetching node :

text/src/components/Editor.vue
	...
	onOpened({ document, session }) {
		...
		if (this.currentSession?.userId && this.relativePath?.length) {
			const node = new File({
				id: this.fileId,
				source: generateRemoteUrl(`dav/files/${this.currentSession.userId}${this.relativePath}`),
				mime: this.mime,
			})
			fetchNode(node)
				.then((n) => { this.fileNode = n })
				.catch(err => logger.warn('Failed to fetch node', { err }))
		}
	},
	...

The File constructor attributes are now good; id, source and mime are valid:

  • id : numeric
  • source : "https://domain:port/remote.php/dav/files/userid/relativepath"
  • mime : text/markdown.

But the fetchNode method fails because it tries to "PROPFIND" the note file with a wrong path "https://domain:port/remote.php/dav/files/userid/filename" instead of "https://domain:port/remote.php/dav/files/userid/relativepath".

In fact the fetchNode method uses the node.path attribute which is a wrong : "/filename" instead of "/relativepath". So the File constructor returns a wrong path.

When you edit an md file in Files, the text app behaves the same.

So there are 2 issues:

  1. the notes app has to be patched to initialize the editor with a filePath attribute.
  2. the File constructor has to be patched also to not modify the source URL.

gaudryc avatar Jun 24 '25 11:06 gaudryc

Finally, here is the two necessary patches.

For th notes version:

--- notes/src/components/NoteRich.vue	2025-06-24 15:02:10.000000000 +0200
+++ notes/src/components/NoteRich.vue	2025-06-24 15:02:06.000000000 +0200
@@ -91,12 +91,13 @@
 			this?.editor?.destroy()
 			this.loading = true
 			this.shouldAutotitle = undefined
 			this.editor = (await window.OCA.Text.createEditor({
 				el: this.$refs.editor,
 				fileId: parseInt(this.noteId),
+				filePath: this.note.internalPath,
 				readOnly: false,
 				onUpdate: ({ markdown }) => {
 					if (this.note) {
 						const unsaved = !!(this.note?.content && this.note.content !== markdown)
 						if (this.shouldAutotitle === undefined) {
 							const title = this.getTitle(markdown)

For the text app:

--- text/src/services/WebdavClient.ts	2025-06-24 02:54:38.000000000 +0200
+++ text/src/services/WebdavClient.ts	2025-06-24 15:05:32.000000000 +0200
@@ -12,12 +12,12 @@
 import type { Node } from '@nextcloud/files'
 
 export const client = davGetClient()
 
 export const fetchNode = async (node: Node): Promise<Node> => {
 	const propfindPayload = davGetDefaultPropfind()
-	const result = (await client.stat(`${davRootPath}${node.path}`, {
+	const result = (await client.stat(`${node.root}${node.path}`, {
 		details: true,
 		data: propfindPayload,
 	})) as ResponseDataDetailed<FileStat>
 	return davResultToNode(result.data)
 }

gaudryc avatar Jun 24 '25 13:06 gaudryc

I opened the following ticket on the text app, proposing this solution. https://github.com/nextcloud/text/issues/7351

Now you need to patch notes.

The rest of the world has to wait until both apps are released.

gaudryc avatar Jun 24 '25 13:06 gaudryc

The previous patches resolve the issue https://github.com/nextcloud/notes/issues/1538, not the current issue.

gaudryc avatar Jun 24 '25 15:06 gaudryc

@gaudryc thank you very much for the patch.

I've tested both patches with nextcloud 30.x and can confirm, that they solve the issue in https://github.com/nextcloud/notes/issues/1538

The problem with the 403 forbidden still exists, so it seems to have another cause.

theroch avatar Jun 24 '25 16:06 theroch

After adding some tracing code, I see that these HTTP 403 errors are due to the data being sent: documentId, sessionId, and sessionToken. These values ​​are provided to the find method of the SessionMapper class (text/lib/Db/SessionMapper.php). This method attempts to retrieve all columns from the "oc_text_sessions" table that match the provided values. If no results are found, a DoesNotExistException is thrown, the session is considered null, and then an InvalidSessionException exception is thrown to the Dispatcher, which returns a 403 error.

The database doesn't contain these values ​​because they are destroyed by a POST /session/{documentId}/close request.

So this problem is caused by the fact that the request "/session/{documentId}/push" arrives after the request "/session/{documentId}/close".

gaudryc avatar Jun 27 '25 12:06 gaudryc

After running some tests, I have two questions: • Is it certain that the editor's destroy method actually and completely destroys the JavaScript object? That no further HTTP requests are issued after destruction? • What is the purpose of the onClose method of the Notes rich editor? When is this method called? And by what or by whom?

Indeed, the problem is the following: • Display Notes • Select a note if none is selected • Verify that the note is edited in rich mode • In the browser console, verify that the editor only generates push and sync requests for this note (these requests concern the same documentId) • Select another note. • In the browser console, verify that the editor now generates push and sync requests for this note AND THE PREVIOUS one. Is this really the expected behavior?

gaudryc avatar Jun 30 '25 07:06 gaudryc

Another issue, observed on Nextcloud 31.0.6 and the text app 5.0.0 (same with Nextcloud 29.0.16.4 and text 3.10.1): Although some text files are saved and the editor's "Undo" and "Redo" buttons are disabled when editing them (from the "Files" section), the oc_text_steps table still contains several rows for these files. The only way to remove these rows is to delete the file. Is this the expected behavior of the text editor? Should a new request be opened for the text app?

gaudryc avatar Jun 30 '25 07:06 gaudryc

In the browser console, verify that the editor now generates push and sync requests for this note AND THE PREVIOUS one. Is this really the expected behavior?

Thanks for debugging this problem @gaudryc! You're right that this is not the expected behaviour. After switching a note, the old editor instance and its sync service should be destroyed and no longer send requests.

@max-nextcloud just merged a few PRs that touch the code base that creates and destroys the editor. Could you try whether your issues are still reproducible with latest Text main branch?

mejo- avatar Jun 30 '25 14:06 mejo-

Although some text files are saved and the editor's "Undo" and "Redo" buttons are disabled when editing them (from the "Files" section), the oc_text_steps table still contains several rows for these files. The only way to remove these rows is to delete the file. Is this the expected behavior of the text editor?

Yes, I guess this is expected. As long as the document editing session exists in the database (see oc_text_documents and oc_text_sessions in the database), the steps need to stay there as well. Otherwise, other participants of the session could not fetch the required steps from the server.

mejo- avatar Jun 30 '25 14:06 mejo-

@gaudryc am I right that only the PR against notes is missing to fix the original issues in this ticket? If so, then I'd prefer to transfer it to the nextcloud/notes project.

mejo- avatar Jun 30 '25 14:06 mejo-

I have tested both text-stable31 (https://github.com/nextcloud/text/commit/62c37ade32613a47942da5b579308d5ea1b672bc) and text-stable30 (https://github.com/nextcloud/text/commit/d239466e90c4976bf1f89c808a8c29c3efbfdab5). With these versions, the text app no longer send sync and push requests for multiple documentId. That'is OK.

But the current issue is still there.

None of the changes I made to the Notes app had any effect on this extra push request.

When using the aforementioned text-stable31 application with notes4.12.1, after switching (without saving), the browser's network console contains: Image You can see that:

  • The sync request comes from the editor itself (OK).
  • The close request comes from the switching click (OK).
  • The push request (with HTTP 403) seems to come from nowhere (????).

The explanation is beyond my skills.

gaudryc avatar Jul 02 '25 13:07 gaudryc

The push request (with HTTP 403) seems to come from nowhere (????).

It seems to stem from an awareness update. (this._awarenessupdateHandler in the trace). When the yjs websocket provider is destroyed it disconnects sending an awareness update to tell everyone it will be gone https://github.com/nextcloud/text/blob/main/src/services/y-websocket.js#L541 This triggers a send - but the send is delayed like all other sends as we wait for a few 100ms to send multiple updates in one go. In the meantime the close request closes the connection making the push fail.

Looking at Editor.vue it seems we first send all remaining steps, then close the connection and then destroy the provider which triggers the awareness update. We should probably destroy the provider first.

https://github.com/nextcloud/text/blob/092b3e4ad0d02894125ba960ffb0e41c4edb1f0d/src/components/Editor.vue#L748-L762

max-nextcloud avatar Jul 26 '25 17:07 max-nextcloud