Web Server route add put-file#9075
Conversation
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Confirmed: linonetwo has already signed the Contributor License Agreement (see contributing.md) |
📊 Build Size Comparison:
|
| Branch | Size |
|---|---|
| Base (master) | 2528.4 KB |
| PR | 2530.7 KB |
Diff: ⬆️ Increase: +2.2 KB
|
If you think PR title is too simple, consider #7542 |
|
At the moment it's not possible to upload files to the file server for security reasons. I think this should be a core plugin. So users the include the server plugin will need to take care, that no "evil" files are uploaded. Just my thoughts. |
|
Thanks @linonetwo, looks good. Please could you add the |
|
@Jermolene Added. @pmario Don't worry, this is as not-secure as PUT tiddler, as long as user expose it on the internet. The intended usecase is on LAN, or under a random password like #7469 |
|
Shouldn't this be writing the response stream directly to the file system rather than receiving it as a buffer first? Something like this? (Code untested, please test first) exports.bodyFormat = "stream";
...
const stream = fs.createWriteStream();
stream.on("error", function(){
response.writeHead(500).end();
});
stream.on("end", function(){
if(!response.headersSent)
response.writeHead(200).end();
});
request.pipe(stream);
I completely agree. Full takeover of the server computer can be effected using only a JavaScript tiddler that gets loaded as a module and executed on the server. All JavaScript is fully privileged in Node TiddlyWiki, and uploading static files is far less dangerous. |
|
@Arlen22 You are right, the buffer way is simplier, and GET was using that, but we could switch to stream, because it will support larger files like videos. And since we are using stream, how about let GET static file also use stream? And since we are using stream, how about also add Range 206 And seems I'm the first one that use the |
linonetwo
changed the title
|
So are we just doing buffer in this PR and then stream in a different PR? Do both PRs get merged? |
|
@Arlen22 This also use stream. This is PUT, another is GET. |
|
For reference, here is a version written a few years ago for use with the FileUploads plugin: /*\
title: $:/plugins/sq/node-files-PUT-support/server-route-upload.js
type: application/javascript
module-type: route
PUT /^\/files\/(.+)$/
Upload binary files to `files/` directory via HTTP PUT
\*/
(function() {
/*jslint node: true, browser: true */
/*global $tw: false */
"use strict";
const fs = require("fs");
const path = require("path");
exports.method = "PUT";
exports.platforms = ["node"];
exports.path = /^\/files\/(.+)$/;
exports.bodyFormat = "stream";
exports.handler = function(request, response, state) {
const MAX_UPLOAD_SIZE = parseInt(process.env.MAX_UPLOAD_SIZE || "10000000"); // 10MB default
let title = state.params[0];
try {
title = decodeURIComponent(title);
} catch(e) {
response.writeHead(400, {"Content-Type": "text/plain"});
return response.end("Invalid URL encoding.");
}
const basePath = path.resolve($tw.boot.wikiTiddlersPath, "../files");
const targetPath = path.normalize(path.join(basePath, title));
// Prevent directory traversal
if(!targetPath.startsWith(basePath)) {
response.writeHead(400, {"Content-Type": "text/plain"});
return response.end("Invalid file path.");
}
// Ensure the target directory exists
try {
$tw.utils.createDirectory(path.dirname(targetPath));
} catch(e) {
console.error("Failed to create directory:", e);
response.writeHead(500, {"Content-Type": "text/plain"});
return response.end("Server error");
}
// Prepare to write the file
const outStream = fs.createWriteStream(targetPath);
let totalBytes = 0;
let limitExceeded = false;
request.on("data", chunk => {
totalBytes += chunk.length;
if(totalBytes > MAX_UPLOAD_SIZE) {
limitExceeded = true;
response.writeHead(413, {"Content-Type": "text/plain"});
response.end("File too large.");
request.destroy();
outStream.destroy();
}
});
// Pipe the stream and handle result
request.pipe(outStream);
outStream.on("finish", () => {
if(limitExceeded) return;
console.log(`External file saved: ${title}`);
response.setHeader("Content-Type", "application/json");
response.end(JSON.stringify({
status: "204",
title: title
}));
});
outStream.on("error", err => {
console.error("Write stream error:", err);
response.writeHead(500, {"Content-Type": "text/plain"});
response.end("Server error");
});
};
}()); |
|
Do not be alarmed! I am closing and then immediately re-opening this PR in order to force the new linting CI to run. It lints just the changed lines in the PR, making sure that new code fits the new coding guidelines, without requiring contributors to fix the linting for the entire file. |
github-project-automation
bot
moved this from Needs feedback
to Done
in Planning for v5.4.0
|
Confirmed: Jermolene has already signed the Contributor License Agreement (see contributing.md) |
editions/tw5.com/tiddlers/webserver/WebServer API_ Put File.tid
Outdated
Show resolved
Hide resolved
saqimtiaz
changed the title
Co-authored-by: Mario Pietsch <pmariojo@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds a PUT endpoint for uploading files to TidGi Mobile's web server to enable syncing external file attachments back to the desktop version. The implementation provides a new /files/<pathname> route that accepts file uploads and stores them in the wiki's files directory.
- Adds a new PUT /files endpoint for file uploads with proper directory creation and error handling
- Provides comprehensive API documentation including headers, parameters, and response codes
- Implements security measures to prevent directory traversal attacks
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| editions/tw5.com/tiddlers/webserver/WebServer API_ Put File.tid | Adds API documentation for the new PUT /files endpoint |
| core/modules/server/routes/put-file.js | Implements the PUT file upload route handler with streaming support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
✅ Change Note StatusAll change notes are properly formatted and validated! 📝 $:/changenotes/5.4.0/#9075Type: enhancement | Category: nodejs
🔗 #9075 👥 Contributors: linonetwo 📖 Change Note GuidelinesChange notes help track and communicate changes effectively. See the full documentation for details. |
|
Thanks @linonetwo. I wonder if it might be worth considering automatically adding a meta file for uploaded files? Are you also considering adding routes to delete and list attachments? |
That could be a parameter. External attachment plugin won't need this because
I think that should be handled by filesystem plugin, when related tiddler is deleted? But this makes me consider, why not reuse existing PUT tiddler server route? With extra parameter to make I have to think about this tomorrow, it is too late and I can't think clearly. |
|
Before merging this, we should test that this works with the FileUploads plugin and the PUT uploader module. |
Enhance PUT file handler to enforce an upload size limit (MAX_UPLOAD_SIZE env var, default 1GB), track incoming bytes and return 413 Payload Too Large when exceeded, destroy the request/stream and remove any partial file. Improve error handling by cleaning up partial uploads on stream error/close. Add optional meta file writing when query parameter meta=true (writes a .meta sibling with title and inferred type). Also tighten path rejection (returns 403 for invalid paths) and update the webserver API tiddler to document the new meta parameter and 413 response.
✅ Change Note StatusAll change notes are properly formatted and validated! 📝 $:/changenotes/5.4.0/#9075Type: enhancement | Category: nodejs
🔗 #9075 👥 Contributors: linonetwo 📖 Change Note GuidelinesChange notes help track and communicate changes effectively. See the full documentation for details. |
|
I add Use WebServer API: Put Tiddler, and execute an action widget to externalize the tiddler content is the right way. We should limit the route so tool list for LLM will be shorter. |
github-project-automation
bot
moved this from In progress
to Done
in Planning for v5.5.0
Me and some user would like to add file as external attachment on TidGi Mobile.
To sync it back to desktop, nodejs server needs to support PUT file. And of course, I can add it to plugin instead of in the core, since no one else tried to add this route, means only TidGi mobile users will use it.