Image Uploads

[atomdmac]

Description

Drag images (JPG, GIF or PNG) onto the canvas to add them to the whiteboard. Images are saved to disk into a directory with the name name as the board JSON file.

Notable Code Changes

• Use new class BoardDataList instead of plain JS object for boards in sockets module.
  • Provides access to the boards list via functions which allows for better access control and observability. This is desirable now that multiple objects may attempt to mutate the data.
• Add new dependency multiparty for parsing FormData on server.

TODO

• Delete images when removed from the board
• Support clicking on canvas with image tool to add image
• Delete tmp file after uploading image
• Address possible security issue here
• When serving board image assets, don't block handler function

Future Improvements

• Display upload progress indicator for image
• Support for "Download Image"

[lovasoa]

Thank you very much for contributing this long-requested feature !

I'll try to review it soon, but before:

• can we add tests ? This is a sensitive feature with big security implications, I think it needs tests.
• the image upload endpoint needs to be disabled when the image tool is disabled (using BLOCKED_TOOLS)

[lovasoa]

I see there are also still TODOs in the code. Feel free to switch it back to ready for review when you are ready for me to have a look

[lovasoa]

also, we cannot have blocking code like this in the request handling function

[atomdmac]

I can do some more research on this but do you happen to know if calling a function that returns a promise would solve this issue?

Something like this:

async function readFile(request, response) {
  // ... create promise, read file from disk, and handle response.
}

function handleRequest(request, response) {
  // ... handlers
  case 'board-assets':
     serveFile(request, response);
     break;
  // ... other handlers
}

[lovasoa]

you can look at how it's done in the rest on the code; we use async fs functions

[lovasoa]

This is a big security vulnerability, isn't it ?

[lovasoa]

Anyone can send an image event with the src attribute they want

[atomdmac]

Is the concern that a user of Board A would be able to load images from Board B (which may be a "private" board)?

[lovasoa]

The bigger concern is that they may have all users make requests to their own server

[atomdmac]

Ah OK - so the concern is that an attacker could inject a URL that points to a server that they control, and that serves files with malicious content. Is that right?

[lovasoa]

Yes, they could serve whatever they want, but also log the ip addresses of everyone connected

[atomdmac]

I believe this has been addressed with the latest commit 👍 Let me know if you disagree.

[lovasoa]

this is complicated. Can we remove the src altogether ?

Suggested change

	img.setAttribute("href", getAbsoluteImageUrl(data.src));
	img.setAttribute("href", "./images/" + data.id);

that would stress me less :)

[atomdmac]

How would the browser know where to pull the image to display from?

[lovasoa]

The browser would make a request to boards/{boardname}/images/{image_id}, the server would check that the image exists, and if so, serve it.

[atomdmac]

Yours is a much better approach 😅 This has been updated.

[atomdmac]

Progress update: I've added some tests but for some reason I can't get them to pass in FireFox (though they pass fine in Chromium).

[lovasoa]

Great to see progress ! I don't have a lot of time to spend on wbo at the moment, but I'm determined to merge this once we are confident it works, is secure and maintainable !