Add Imageproxy to resolve relative Imagepaths #785

newhinton · 2021-11-28T10:29:45Z

Todo, for now see #74

We should have a serious discussion about the proxy. Currently it allows to get ALL files from files, even outside of the Notes-folder. We may want to restrict access to Files somewhere accessible to the notes app, not all over the instance.
Currently only files in the Notes-basefolder are accessible. This removes all security issues posed by allowing abitrary paths.

Also, because of how urls are encoded, ../ is beeing removed (from the url). Therefore, the api only accepts ;;/ as a substitute, and all clients need to replace the ../ syntax before rendering the link (i am open for suggestions on how to fix this). The markdown-sourcefile does not need to be modified, and is not beeing modified by this.

newhinton · 2021-11-29T23:06:24Z

I added a prototype toolbar for image-uploading. It is barebones, and needs to be improved upon before release, do we want to add additional buttons, similar to how nextcloud/text has one?

in that case i would just hide the toolbar in this pull request, and start working on a proper toolbar after this PR with proper styling and icons.

stefan-niedermann

Could you please add an API documentation to https://github.com/nextcloud/notes/blob/master/docs/api/v1.md ? 🙂

newhinton · 2021-11-30T09:57:52Z

@stefan-niedermann Yes, but i would like to wait for a review of those api-endpoints. I am really not sure if i implemented them in a good way, especially about the ../-conversion

korelstar · 2021-12-02T19:47:27Z

Nice, that's very cool! Thank you very much for digging into this! I'm very sorry, that I wasn't able to review this, yet. I will do my very best in order to have a deep look on the weekend!

newhinton · 2021-12-02T23:13:59Z

@korelstar No problem!

It's a bit offtopic, but what do you think about this toolbar idea? The styling is not good at the moment, and some icons are missing, but i think the general idea is not to bad

maybe we should try to mirror what nextcloud text does:
https://github.com/nextcloud/text/blob/master/img/screenshots/screenshot1.png

while it is a really bad idea to copy their markdown editor, i think their ui is very clean

newhinton · 2021-12-03T13:23:14Z

@korelstar Could you give me a hint on how i rebuild the css outside of vue files? I'd lile to add icons but they don't seem to update

newhinton · 2021-12-04T17:49:39Z

I have problems with relative subdirectories and the ../ notation.
This is due to how url's are handled by browsers. We also cannot switch to post-requests, since this does not work with the preview. We have to use a get request which encodes the filepath somehow.

My current idea is to try using base64 encoding, that should get rid of the character issues while easily usable on almost every system that may want to access the api.

newhinton · 2021-12-04T18:09:18Z

base64 encoding works well.
I have also added a local filepicker, however this is more a work in progress, since i have to find out how i can get the app-storage path and the categories to the markdown editor. But it is nice to see it in action!

Also we need to find out if and how we allow both of the picker, local filesystem and the nextcloud one

korelstar · 2021-12-05T18:20:21Z

Thank you very much for your efforts, @newhinton ! I like your approach and I think it's on a good way. It looks that there are some fundamental question to answer resp. decisions to be made. Let's try to collect them.

How to identify an attachment

You've chosen the file name for identifying the attachment. That's a good, straight forward approach. However, I see one problem: if you add an attachment to your note and then change the note's category, the attachment won't be found anymore. Since I think that's a common use case, we have to find a solution for this.
One approach could be to use the attachment's file id instead of the path. However, this is not very intuitive and could cause problems when migrating notes to another Nextcloud instance.
Another approach would combine both ideas, i.e. file id and path are concatenated, e.g. 629|mypicture.jpg. But this wouldn't be intuitive, either. The advantage, however, would be that the server could first try to find a file by the file id, and (if this is not successful) then by it's name (or the other way around). If there was a change, the note should be automatically updated (is this feasible?).
Maybe we find another, better approach?

Source of attachments

Should we allow to attach files to a note from the notes path only or should we allow files from the whole Nextcloud's user path? You've switched to the notes path only for security reasons. I think this is a wise decision, but I want to make this decision clear.

API path

What about the following in order to eliminate the ../ problem (and render base64 unnecessary):
GET /notes/{noteid}/attachment?path={path}
Uploading an attachment could be done with POST /notes/{noteid}/attachment.
If we switch to the file id resp. combined approach (see section "How to identify an attachment"), the path would be: GET /notes/{noteid}/attachment/{attachmentid}?path={path}

Toolbar

A toolbar is a good enhancement, let's discuss details in your new issue #788.

Static CSS

Could you give me a hint on how i rebuild the css outside of vue files? I'd lile to add icons but they don't seem to update

If you change the scss files in the css directory, there is no need for re-building. Building is done by the Nextcloud server automatically.

newhinton · 2021-12-05T18:48:06Z

Could you clarify what you mean with attachment id?

My assumption was that we can only deal with standard "URL's". Those can be relative or absolute, where absolute links are already handled properly.

So we have relative path's which usually point to a local filesystem. So i wanted to resolve this without needing a "new" way to integrate images, as to keep compatibility with other notes apps.

Where would we get an attachement id, and how would we store it?

to your point with ?path=: if it works, it could be a good idea too. However, quickly rewriting the route results in basically nothing, it redirects to the dashboard. I am not sure if it is supported, because it is not documented: https://docs.nextcloud.com/server/latest/developer_manual/basics/routing.html

newhinton · 2021-12-05T18:49:22Z

I will also remove the toolbar in my cleanup before final commit. It is basically necessary as long as we test/change this code, but the ability to upload files (from the ui) should be included with the toolbar, not this PR

newhinton · 2021-12-05T19:09:39Z

Oh and regarding the issue with switching the category: do other apps take care of this? Should we?

One could argue that it is not our job to take care of that, since other notes could also rely on the image beeing in that folder. If we move it, we could break other notes aswell.

The same argument can be made for deleting images from the source folder.

korelstar · 2021-12-05T19:18:40Z

Attachment id would be the Nextcloud file id of that attachment. But I see your point, such a syntax would not be compatible to other apps. Honestly, I don't know if other apps handle the movement scenario. Maybe, they don't, too. Just wanted to list all issues/decisions that we make, to be clear that we are aware of them.

Regarding the ?path= thing: You're correct, we can't declare this in the routes.php. However, it still works, if routes.php declares the url /note/{id}/attachment and the respective controller function has a parameter $path.

newhinton · 2021-12-05T19:32:29Z

Oh yeah, i tried it without declaring it and it works flawless.

...i also found the documentation for it: 😀
https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html?highlight=parameter#getting-request-parameters

i will change it to use parameters instead of base64, it is way more intuitive and readable.
I will also assume that GET /notes/{noteid}/attachment?path={path} is now "final" and will add it to documentation.

However, one thing: we only render images currently, not other content, so "attachment" is not 100% accurate at the moment. We may implement a "download" button for filetypes that we do not recognize in the renderer at a later point.

Though, i like the idea!

korelstar · 2021-12-05T19:35:55Z

However, one thing: we only render images currently, not other content, so "attachment" is not 100% accurate at the moment. We may implement a "download" button for filetypes that we do not recognize in the renderer at a later point.

Though, i like the idea!

Yes, this should be compatible for future enhancements.

newhinton · 2021-12-05T21:48:01Z

I think i am now ready for a final review. I have refactored a lot and changed naming quite a bit, so it should be way better now. (also the toolbar is gone, but the actual upload/selection functions are still there, they can be easily integrated later)

newhinton · 2021-12-06T16:15:12Z

Oh i remembered something:

is this the right way to query the current category?

notes/src/components/EditorEasyMDE.vue

Line 136 in 9f0266c

const categories = store.getters.getCategories()

i found that it does not work reliable, so this may still be broken in my pr.

Also, is there a discussion on why we call it category and not folder?

newhinton · 2021-12-08T15:02:29Z

I made a small modification to also render normal files. It downloads the file on click. I do not know if this is in line with how markdown should handle files, but it can easily be removed

korelstar · 2021-12-11T12:17:52Z

Oh i remembered something:

is this the right way to query the current category?

notes/src/components/EditorEasyMDE.vue

Line 136 in 9f0266c

const categories = store.getters.getCategories()

i found that it does not work reliable, so this may still be broken in my pr.

This should be fine. What do you mean exactly with "does not work reliable"? What happens?

Also, is there a discussion on why we call it category and not folder?

It's our application abstraction layer. For the user, a note is assigned to a category. On the technical backend, a note is stored as a file and the category is stored as folder. But those are different layers. The user does not need to know this. The user is only interested in notes and categories - not in files and folders.

I made a small modification to also render normal files. It downloads the file on click. I do not know if this is in line with how markdown should handle files, but it can easily be removed

I like this!

This branch has conflicts that must be resolved

Could you please fix that conflict (and rebase your PR)? I will do a deeper review later this day.

korelstar

I finally found some time for a deep review :-)
First of all: thank you again for your work on this! I'm writing some general remarks and I did commented some special changes.

Code style

Please use make lint for checking your code style and fix all errors and warnings regarding your changes.

external API

Currently, you've implemented the internal API (NotesController). That's fine and sufficient for now. However, you've documented the external API (docs/api/v1.md) although it is not implemented yet. I think we can postpone this for the next step. Since you're developing very fast and you have opened multiple PRs that are partly conflicting, let's concentrate on small parts.

Controller vs. Service

That controllers are used to connect routes with app logic. But please note, that the actual app logic should be in respective service classes. On the other hand, the service should not contain any code regarding the provided API (e.g. Http stuff), this has to be done in the controller.
To be more concrete: please move most of the method NotesController.getAttachment (except the Response stuff) to NotesService and move the Response stuff from NotesService.createImage to NotesController.

src/components/EditorMarkdownIt.vue

appinfo/routes.php

lib/Controller/NotesController.php

src/components/EditorMarkdownIt.vue

korelstar · 2021-12-11T15:30:51Z

lib/Controller/NotesController.php

+			// calculate how many parentnodes we need to get 'up'
+			$parentcount = substr_count($path, '../');
+			$path = str_replace("../", "", $path);
+
+			$relativeImageNode = $this->root->get($notePath);
+			for ($i = 0; $i < $parentcount; $i++) {
+				$relativeImageNode = $relativeImageNode->getParent();
+			}
+			$targetimage = $this->root->get($relativeImageNode->getPath()."/".$path);


There must be a check, if we are still in the notes path. Currently, I can load images from other users!

ohh yeah, just to understand how you accessed this:

did you prepend '../' the exact same amount to logically get back to "root"? and then append a full, internal nextcloud-path including the different user?

Could you provide me with your testsetup?

I know why and how to fix it, but i cannot replicate it since i always get a second / in my path
Eg.: ?path=../../../test/files/Nextcloud Manual.pdf results in an exception to be thrown, not an image to be served.

I think i fixed it, but since i could not properly reproduce it i cant really verify it. Please try to break it again! I want to be sure this cannot be abused

My test setup:
User A has a note in notes/test/test.md with the following content:

![Attack](../../../../B/files/DSC_0020.jpg)

User B has the image file DSC_0020.jpg in his user directory.

I will test your changes soon.

Thanks, looks like your fix is working. I think your solution on how to fix this is a good approach! 👍

src/components/EditorMarkdownIt.vue

newhinton · 2022-01-20T18:58:03Z

@korelstar did you mean the DCO?

korelstar · 2022-01-20T19:03:16Z

No, I mean the warnings from make lint. You can see them also on https://github.com/nextcloud/notes/pull/785/files

Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>

newhinton · 2022-01-20T19:49:18Z

Interesting that they did not show up in the testruns. Anyway, i fixed them except those i did not cause, and a single one i dont know how to fix:

The file we are serving is actually not a File, but a Node. The FileDisplayResponse expects files, so it complains. It works because File extends Node. Just casting did not seem to work. How do i get a file from a node?

korelstar · 2022-01-20T21:09:12Z

This can be done by inserting assert($targetimage instanceof \OCP\Files\File);

I added this and I also did some of the major remarks from #785 (review) . Will continue with reviewing later.

newhinton · 2022-01-26T09:33:14Z

@korelstar Actually i do have a question regarding my changes to the readme. Since the imageproxy is available through an endpoint, doesn't that count as an external api? Why should it not be documented?

korelstar · 2022-01-30T08:01:09Z

@korelstar Actually i do have a question regarding my changes to the readme. Since the imageproxy is available through an endpoint, doesn't that count as an external api? Why should it not be documented?

The notes app has two REST interfaces: one for the web interface (NotesController, internal API) and one for third-party apps (NotesApiController, external API). The documentation is about the external API.

In your current implementation, the proxy is not available from external, since it uses the NotesController. Therefore we must not document this in the external API documentation.

Of course, the goal is to provide this new feature also to the external API. But let's do this in a separate PR, so we can merge this PR earlier and do further tests with it. Another point: when changing the external API, you will have to write respective tests. This is currently missing.

korelstar · 2022-01-30T10:40:43Z

Just did some last improvements. If you agree, I will merge this now.

newhinton · 2022-01-30T11:27:31Z

The simplification looks good to me, except i cant see how we make sure we did not leave the notes directory. Was the removal of the last if clause wether the resulting node is still in the notes-path intentional?

Edit: I think i figured it out, you do not actually work on a full path, but only on the categories of a note. So when we "overstep" the boundaries we get 'nothing' (in terms of the array you are using) and then we get the child of the notes folder with that array

korelstar · 2022-01-30T12:46:45Z

Edit: I think i figured it out, you do not actually work on a full path, but only on the categories of a note. So when we "overstep" the boundaries we get 'nothing' (in terms of the array you are using) and then we get the child of the notes folder with that array

Exactly!

korelstar · 2022-01-30T13:29:17Z

Next steps are:

1. Add upload feature to web-frontend

Implement using toolbar (requires #790) or at the first step with a new entry in the three-dot menu. I vote for the last since we don't have any dependencies, then. Would you like to work on this?

2. Public API for third-party apps

discuss public API with third-party app developers (what are their desires?)
add new API methods to NotesApiController
add API documentation (here is the last state: https://github.com/nextcloud/notes/blob/aac2223717065fa216f7be315df3bcde332d7c3a/docs/api/v1.md)
add API tests (see here: https://github.com/nextcloud/notes/blob/master/tests/api/APIv1Test.php)

newhinton · 2022-01-30T15:31:37Z

@korelstar would you mark #74 as resolved? I'd like to claim the bounty for the ticket but i dont think its my place to close the ticket on my own for that

newhinton · 2022-01-31T10:08:50Z

oh and i will continue work on the toolbar aswell, though that might take a couple of weeks

korelstar · 2022-02-06T20:40:07Z

@korelstar would you mark #74 as resolved? I'd like to claim the bounty for the ticket but i dont think its my place to close the ticket on my own for that

I don't think that it's already solved. But you're on a good way! A big part of that issue is about uploading a new image. You've already prepared this, but it's currently not usable. The original issue's requirements for uploading are here: #74 (comment) . In addition, you could add an upload menu item (see #785 (comment)).

newhinton self-assigned this Nov 28, 2021

newhinton changed the title ~~Add Imageproxy to resolve relative Imagepaths~~ [Draft] Add Imageproxy to resolve relative Imagepaths Nov 28, 2021

newhinton requested a review from korelstar November 29, 2021 01:14

newhinton mentioned this pull request Nov 29, 2021

📎 Attachments nextcloud/notes-android#1051

Open

newhinton changed the title ~~[Draft] Add Imageproxy to resolve relative Imagepaths~~ Add Imageproxy to resolve relative Imagepaths Nov 29, 2021

newhinton force-pushed the feature/74/image_display branch 2 times, most recently from fd5d47f to 33bc254 Compare November 29, 2021 23:00

stefan-niedermann requested changes Nov 30, 2021

View reviewed changes

newhinton mentioned this pull request Dec 1, 2021

support for attachments / images [$106] #74

Closed

newhinton mentioned this pull request Dec 4, 2021

Add toolbar to native markdown editor #788

Open

5 tasks

korelstar requested changes Dec 11, 2021

View reviewed changes

korelstar reviewed Dec 18, 2021

View reviewed changes

src/components/EditorMarkdownIt.vue Outdated Show resolved Hide resolved

fix lint issues

366fafe

Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>

code style and clean-up

e7014c8

clean up code

dad79c8

korelstar added 2 commits January 30, 2022 11:37

simplify path check

d05af13

improve path check

d2fe4a3

korelstar approved these changes Jan 30, 2022

View reviewed changes

korelstar merged commit 34d3cc1 into master Jan 30, 2022

korelstar deleted the feature/74/image_display branch January 30, 2022 13:22

korelstar added this to the 4.4.0 milestone Feb 7, 2022

korelstar mentioned this pull request Feb 27, 2022

External API for attachments #826

Open

korelstar unassigned newhinton Feb 27, 2022

This was referenced Mar 19, 2022

Horizontally scale images to text viewport #831

Closed

Improvements on images/attachments #834

Merged

korelstar added feature: backend Related to the PHP backend feature: editor Related to generic parts of the editor, e.g. loading/saving notes feature: Markdown-it Related to preview mode using Markdown-it labels Mar 20, 2022

newhinton mentioned this pull request Feb 24, 2023

feat(editor): Use text editor #958

Merged

15 tasks

Add Imageproxy to resolve relative Imagepaths #785

Add Imageproxy to resolve relative Imagepaths #785

Conversation

newhinton commented Nov 28, 2021 • edited Loading

newhinton commented Nov 29, 2021

stefan-niedermann left a comment

Choose a reason for hiding this comment

newhinton commented Nov 30, 2021

korelstar commented Dec 2, 2021

newhinton commented Dec 2, 2021 • edited Loading

newhinton commented Dec 3, 2021

newhinton commented Dec 4, 2021

newhinton commented Dec 4, 2021

korelstar commented Dec 5, 2021

How to identify an attachment

Source of attachments

API path

Toolbar

Static CSS

newhinton commented Dec 5, 2021

newhinton commented Dec 5, 2021

newhinton commented Dec 5, 2021

korelstar commented Dec 5, 2021

newhinton commented Dec 5, 2021

korelstar commented Dec 5, 2021

newhinton commented Dec 5, 2021

newhinton commented Dec 6, 2021

newhinton commented Dec 8, 2021

korelstar commented Dec 11, 2021 • edited Loading

korelstar left a comment

Choose a reason for hiding this comment

Code style

external API

Controller vs. Service

korelstar Dec 11, 2021

Choose a reason for hiding this comment

newhinton Dec 11, 2021

Choose a reason for hiding this comment

korelstar Dec 11, 2021

Choose a reason for hiding this comment

newhinton Dec 16, 2021

Choose a reason for hiding this comment

newhinton Dec 16, 2021

Choose a reason for hiding this comment

korelstar Dec 18, 2021

Choose a reason for hiding this comment

korelstar Dec 18, 2021

Choose a reason for hiding this comment

newhinton commented Jan 20, 2022

korelstar commented Jan 20, 2022

newhinton commented Jan 20, 2022

korelstar commented Jan 20, 2022

newhinton commented Jan 26, 2022

korelstar commented Jan 30, 2022

korelstar commented Jan 30, 2022

newhinton commented Jan 30, 2022 • edited Loading

korelstar commented Jan 30, 2022

korelstar commented Jan 30, 2022

1. Add upload feature to web-frontend

2. Public API for third-party apps

newhinton commented Jan 30, 2022 • edited Loading

newhinton commented Jan 31, 2022

korelstar commented Feb 6, 2022

newhinton commented Nov 28, 2021 •

edited

Loading

newhinton commented Dec 2, 2021 •

edited

Loading

korelstar commented Dec 11, 2021 •

edited

Loading

newhinton commented Jan 30, 2022 •

edited

Loading

newhinton commented Jan 30, 2022 •

edited

Loading