Skip to content
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

File node: Add fileWorkingDirectory to customise how relative paths are resolved #2932

Merged
merged 1 commit into from Apr 27, 2021

Conversation

knolleary
Copy link
Member

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Currently if a relative path is used in the File nodes, it is resolved relative to the working directory of the Node-RED process.

That is non-obvious to new users and they don't necessarily know what the cwd is.

This change introduces RED.settings.fileWorkingDirectory that allows the user to set a specific directory to be used to resolve relative paths.

This PR only touches the File nodes. We should review other nodes that handle local paths and update them to honour this setting.

Checklist

  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@hardillb
Copy link
Member

hardillb commented Apr 12, 2021

I like this, I was talking about how securely add a persistent volume to a multi-tenant system earlier to allow the use of the file nodes and this was one of the things that came up.

It might be worth looking to see if we can add a mode to force absolute paths to be rooted to here as well. e.g. path.join( RED.settings.fileWorkingDirectory, inputPath)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 67.399% when pulling aac2a8f on file-cwd-setting into ba56665 on dev.

@dceejay
Copy link
Member

dceejay commented Apr 14, 2021

@hardillb - where does inputPath come from ?

At the same time as above we should block relative paths to prevent path traversal escapes.

@hardillb
Copy link
Member

@dceejay it was just meant to represent what ever the user had configured.

And yes we can use something like path.resolve() and then check is starts with RED.settings.fileWorkingDirectory to catch relative path escapes.

I'll try and find an hour to have a poke at the code properly

@dceejay
Copy link
Member

dceejay commented Apr 14, 2021

Personally I think we should also block access to the in use .node-red directory to prevent access to creds files settings etc - either accidentally or maliciously.

@dceejay
Copy link
Member

dceejay commented Apr 14, 2021

It should also display(but not allow edit) the setting in the html file so the user knows where they are appending to.

@knolleary
Copy link
Member Author

This setting is just about being able to define the root path rather than leaving it to the working directory of the Node-RED process. That's it.

I agree there's a separate option that would be useful to restrict the file nodes to only that directory - but it is a separate option that needs defining.

Personally I think we should also block access to the in use .node-red directory to prevent access to creds files settings etc - either accidentally or maliciously.

I understand what you mean, but I'm not sure it's that simple. For example, there are plenty of legitimate reasons for a flow to read/write to the .node-red directory; a flow could just be using that dir as a useful place to store its own content.

A combination of fileWorkingDirectory and the TBD setting to restrict access to just that dir, would be a way to prevent access. But we have to acknowledge the fact a flow could make use of the fs module in the Function node to access whatever it wanted.

@dceejay
Copy link
Member

dceejay commented Apr 14, 2021

Agreed - a specific node could do whatever it wants so can use .node-red - but I think the generic file node should be safe by default. It could be exactly that dir and not include children so .node-red/public could be allowed. Not sure it needs an extra setting.

@knolleary
Copy link
Member Author

There is the suggestion of an extra setting to limit access to whatever fileWorkingDirectory is set to. That is separate to your additional suggestion of restricting .node-red.

If we do any sort of restriction by default of the .node-red directory then we will definitely need an option to turn that off - to give users a way to migrate without having to change their flows. But - this PR is about fileWorkingDirectory, so these comments are not the place to be designing different options.

@dceejay
Copy link
Member

dceejay commented Apr 14, 2021

ok - so re fileWorkingDirectory specifically

  • should report that path to the config screen so the user knows what to add for the file path/name from there.

@knolleary knolleary merged commit 5305506 into dev Apr 27, 2021
Node-RED 2.0 Planning automation moved this from In progress to Done Apr 27, 2021
@knolleary knolleary deleted the file-cwd-setting branch May 3, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants