search

mashpie / i18n-node

Lightweight simple translation module for node.js / express.js with dynamic json storage. Uses common __('...') syntax in app and templates.
MIT License
3.08k stars 421 forks source link

What the heck with file writes/updates?? #298

Open augnustin opened 7 years ago

augnustin commented 7 years ago

I still cannot believe this has been coded.

This package, if eventually used in production, should not be able to write into files. What a wonderful security breach!!

There should be tools for maintaining translations (eg. a CLI, a git-diff like) but they must be separated from the app logic, which should access translations in a read-only mode.

And the answer it depends on the environment, in dev it is fine is not acceptable! :smile: The logic separation is absolutely mandatory!

AlanAboudib commented 7 years ago

I had a similar problem where my language JSON files in the locale folder got rewritten each time there was a bug. Some times it replaced the files with another language or replaced the key/value pairs with key/key pairs. Did you have a similar problem?

silnose commented 7 years ago

Maybe this help Issue 226

mashpie commented 4 years ago

Defaults will change and deprecate writing files in production to prevent unintended issues.

No aim to start a discussion, but wanted to leave some words to clarify:

Writing files can not be seen as a security risk per se. Think of loggers, transcoders, static caches or similar. Writing files is required to fulfil a lot of features in varying scopes, let there be users, business-logic, (non-)functional requirements or even DX.

In case of i18n-node module it was introduced as a feature request to provide a tiny translator-service which was able to track newly added keys from other newly deployed services by adding them to all existing language files. Those files have been maintained by another external translation service via file-uploads (automatic or scheduled) which itself triggered the rest of translation jobs in terms of continuous i18n/l10n - so, well this actually is app-logic in that use-case.

Still for writing files validation & sanitising should catch malicious data. Native JSON.stringfy() is "sanitising" all data written to disk and in contrast to most of existing json-stringify or various yaml packages it is not vulnerable to code injection or prototype pollution, etc. In fact, writing json by native serialisation is much more reliable and less error prune than letting humans write those files.

And as you mentioned separation... separation of concern is crucial - totally agree! Concern is not dependent on environment - totally agree! But concerns will be different depending on use-case or business requirements. i18n as a library should provide tools to implement solutions for various requirements - implementation details in the end depend on developers.

augnustin commented 4 years ago

This does start a discussion. :smile:

I agree some app logic can write to files, but in this case, it writes to file that are subversionned and parsed back in the app, which makes me worry.

I understand that you have specific needs in your case, but then don't publish this as an official node package, and keep it for yourself. Or if you want others to have access to this content editor, create a subpackage that works tightly with i18n-node which would be something like i18n-node-editor, would be in devDependencies and would do the writing stuff.

No offence intended here, but this is not very professionnal IMHO and brings every NPM user at risks, as it was seen in the past.