Skip to content

WebDAV: update to v5 #6780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

WebDAV: update to v5 #6780

wants to merge 6 commits into from

Conversation

PTR-inc
Copy link
Contributor

@PTR-inc PTR-inc commented Feb 14, 2025

The currently used webdav package is v4 using an older axios dependency, giving a security warning in npm audit. The v5 version is an esm package, so converted the upload function accordingly. Now it's up to date again, also dependency-wise.

@si458
Copy link
Collaborator

si458 commented Feb 20, 2025

you are using import instead of require
wont this break existing installs?
also we dont specify "type": "module" in our package.json, so again wont this break meshcentral?

@PTR-inc
Copy link
Contributor Author

PTR-inc commented Feb 21, 2025

@si458 I checked a few ways of using esm in commonjs and this seems to me the best way so not to have to change too much.

  • no need for "type": "module" in package.json, as that would indeed break stuff.
  • by moving the webdav part to it's own function out of the main performCloudBackup function, everything else remains untouched. No need to alter core flows.

Could also use const webDAV = require('webdav'); instead of the import line, but they are functionally the same here. Dynamic import is fully supported since node v15. If you want I can change it to require?

@si458
Copy link
Collaborator

si458 commented Feb 21, 2025

@PTR-inc would u mind please! Sorry to be a pain!

I want to try keep everything similar/the same

So if they all stay as require, then in the future it be easier to swop it all out :)

@PTR-inc
Copy link
Contributor Author

PTR-inc commented Feb 21, 2025

Done.

db.js Outdated
if (func) { func('Uploading using WebDAV to: ' + parent.config.settings.autobackup.webdav.url); }
});
}
const { stat } = await import ('fs/promises');
Copy link
Collaborator

Choose a reason for hiding this comment

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

please can you change this to using fs.statsync instead?
https://nodejs.org/docs/latest-v16.x/api/fs.html#fsstatsyncpath-options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to using pipeline instead of .pipe. No stat needed anymore. Cleaner, more resillient and futureproof. Tested it on 16.0.0 and latest 16, 22 & 23.

@PTR-inc
Copy link
Contributor Author

PTR-inc commented Feb 25, 2025

And reverted the require back to use import, as v16 doesn't allow using require for es modules.

@si458
Copy link
Collaborator

si458 commented Mar 18, 2025

this ok to merge still?

@PTR-inc
Copy link
Contributor Author

PTR-inc commented Mar 18, 2025

@si458 Certainly, if you are ok with the changes/async way. Just did a quick test against the current master and all fine.

@PTR-inc
Copy link
Contributor Author

PTR-inc commented Mar 30, 2025

@si458 Is there something keeping this on hold?

@si458
Copy link
Collaborator

si458 commented Mar 30, 2025

no sorry just working my way through fixing a file transfer bug first then move onto this
its on my to-do to merge/verify for you 👍

@si458
Copy link
Collaborator

si458 commented Apr 12, 2025

this still good to go?

@PTR-inc PTR-inc marked this pull request as draft April 14, 2025 20:41
@PTR-inc
Copy link
Contributor Author

PTR-inc commented Apr 14, 2025

Read about an issue somewhere else with trailing slashes and webdav, so checked the webdav rfc's and added a slashcheck for the foldername and moved it to the setup defaults sections.

@PTR-inc PTR-inc marked this pull request as ready for review April 14, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants