How to includes "," or "*" char self in --allow-fs-read?

node --experimental-permission --allow-fs-read=/path,with,comma,and,*/entry entry

They are valid unix path name, and as an app developer, I can't control what parent directory user's pc use.

Currently I can only shutdown the whole app when user's directory has , or *, that's not good...

Thanks for the feedback. Probably, we'll need to switch the delimiter to a semicolon (;). I'll work on that.

@RafaelGSS

My two cents:

Though this feature is experimental, change dilimiter is break change about safe.

And ; is still a possible path char. Maybe --allow-fs-read=/path,1 --allow-fs-read=/path,2 can resolve the problem of delimiter? Many cli use this rule, like --conditions=.

@RafaelGSS

* char ambiguity is another problem, we can think about it as an individual later problem.

@LongTengDao

@LongTengDao Would it work if you escape the , delimiter?

I think for your use case the only reasonable choice is to pass those files through a config file.

@RafaelGSS

What encoding does --allow-fs-read= accept to escape special charactors? It support file:/// with encodeURIComponent? Or a RegExp's source, like --allow-fs-read=^/path\*[12]/sub,name\..+$s? You can directly use ^ leading value like:

RegExp(value.slice(0,value.lastIndexOf('$')+1), value.slice(value.lastIndexOf('$')+1))

@RafaelGSS

A config data is good, but the value could change for each time fork, so a json arg should better be an alt solution, like:

child_process.fork(
    ...{
        execArgv: [
            `--allow-fs-read=${JSON.stringify([ '/path,1' ])}`
        ]
    }
)

Otherwise, I need to generate a temp config file for each fork, and unlink that after using, to control fs io indirectly...

It's two complex for just support ,, and still can't escape *, so I think the escaping solution is better.

(--experimental-policy can expect a static file path, because it just used for dependencies which can be certain while developping, but --experimental-permission is for any user io or gui actions.)

It only accepts file paths with wildcards (*). Accepting JSON is impractical. We can either accept an array:

--allow-fs-read=/path-1 --allow-fs-read=/path-2

or you should send it through a config file. I'll see how feasible is the array approach.

@RafaelGSS

--experimental-policy accept file:/// url, if we also support --allow-fs-read=file:///path%2C1/%2A,file:///path%2C2/* may resolve this problem.

@RafaelGSS

@RafaelGSS

Hi! I'm working on this and here is what I thought.

Changing the behaviour to an array would be a breaking change. Therefore it would be a good option to check on the length of the array and preserve the current behaviour when array is of length 1, then split.

The problem there would be if I only have one file which contains a comma. Besides, this wouldn't solve the issue for people wanting to use the single string.

An option would be to add a new flag permissions-path-delimiter which defaulted to comma but the user could change to whatever character which doesn't conflict with their paths.

We could implement just the new flag, the array approach –knowing the caveat– or both.

WDYT?

CC @RafaelGSS

We can also read that configuration from a config file. We can adapt the current policy.json to accept permission model config too

Is there a preferred way or should we implement them all?

I think, for now, we can go with: --permission-fs-path-delimiter and flag it as experimental as well.

cc: @nodejs/security-wg

src,permission: add permission-fs-path-delimiter flag

@tniessen

Hi 👋🏻.

There is an standing PR that addresses the part of comma as part of paths by adding a new flag --permission-fs-path-delimiter that allows the user to choose a different delimiter.

During the discussion of said PR @tniessen brought up that it might not be a good idea to have comma as default separator. As we have seem it conflicts with some valid paths and it doesn't conform with path.delimiter. Therefore I would like to expose some of the alternatives and seek a wider consensus.

Keep the PR as is. The pros for this would be not having a breaking change. The cons is that needs to add a new flag for a problem which could be solved with a cleaner CLI option.
Accept multiple --allow-fs* flags. This is already used in --watch-path, which is also experimental atm. In this case we would have to decide if we keep the comma separator option in case there is only one flag, which would be non-breaking. If we do keep it we would still have the problem when only one path with commas is passed, although idk how likely is that. If not we would have a breaking change.
Change the default separator with path.delimiter. This would have no conflicts with valid paths and it is already being used in NODE_PATH envvar, which is the only stable related example I could find.
Any combination of the previous.

IMO given the experimental nature and relatively short live of these flags going for the path delimiter option seems the most clean way going forward as it wouldn't add new flags or documentation and it is consistent with the only stable example I could find.

WDYT?

@RafaelGSS

I think the best approach depends on whether this syntax will ever be used for anything besides file system paths. For example, there are still discussions around environment variables. If this is supposed to be a more generic list format than just file system paths, then that should be planned for now.

Accepting multiple flags seems very reasonable to me. (Then again, I have not been following this discussion, so I trust your judgement on that.)

Change the default separator with path.delimiter. This would have no conflicts with valid paths

That's not entirely true. On POSIX systems, file names can technically contain path.delimiter. However, shells are often unable to handle such file paths, so Node.js wouldn't be the only tool that is unable to do so. Still, I believe @RafaelGSS made it a priority to have a portable syntax, and using path.delimiter limits portability.

For example, there are still discussions around environment variables. If this is supposed to be a more generic list format than just file system paths, then that should be planned for now.

For now, environment variables aren't planned. But, if we include that I agree it should follow the same pattern.

I think we should go with option 2. It seems the smoothest breaking change for the end users. We can also:

If a single path is passed and this path contains a comma
We emit a warning saying that paths are now handled by multiple --allow-fs- flags.

Would it make sense?

ping @nodejs/security-wg

I prefer to use --permission-fs-path-delimiter=// (any other char will still meet possible conflict) in my use case to resolve delimiter problem forever without break change.

And * conflict can also use --permission-fs-path-matcher= to resolve. Then I may use --permission-fs-path-delimiter=//,// --permission-fs-path-matcher=//*// to end this issue. This way can also add other special functional char in the furture via --permission-fs-path-xxx=.

@LongTengDao

@LongTengDao The problem with --permission-fs-path-* is the design issue. Imagine how many flags would we need in order to include custom options. Could you share a bit more about your use case? In which cases users are using * as filepath?

a gui sandbox software, drag a folder in and run it. i can't control what's included in users' folder path. currently, i can only shut down the software when path including special chars.

@Ceres6

We've discussed it in the last security team meeting and we're tempted to follow the option 2. cc @Ceres6

If a single path is passed and this path contains a comma
We emit a warning saying that paths are now handled by multiple --allow-fs- flags.

We emit a warning and split with commas or not?
Regarding the wildcard, are we going to implement something for that?

We follow multiple cli flags approach: --allow-fs-read=/tmp/ --allow-fs-read=/path,2/index.js. However, since it's a breaking change, we could check if users are using the old approach and thus emit a warning about the new usage. Example:

$ node --experimental-permission --allow-fs-read=/tmp/,/path/index.js index.js
Warning: the allow-fs-read CLI flag has recently changed. Each file/directory should have its own --allow-fs-read call. Example: --allow-fs-read=/folder1/ --allow-fs-read=/folder2/
...

Probably a better description should be used.

src,permission: add multiple allow-fs-* flags

I forgot we still have to deal with * (wildcards). I will open a new issue for that.

@RafaelGSS

@RafaelGSS I can implement that too. I'm not sure what options do we have for that apart from escaping...

Navigation Menu

Repository navigation

How to includes "," or "*" char self in --allow-fs-read? #1039

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

How to includes "," or "*" char self in --allow-fs-read? #1039

Description

Activity

RafaelGSS commented on Jun 26, 2023

LongTengDao commented on Jun 28, 2023

RafaelGSS commented on Jun 28, 2023

LongTengDao commented on Jun 28, 2023

RafaelGSS commented on Jun 28, 2023

LongTengDao commented on Jun 28, 2023

LongTengDao commented on Jul 4, 2023

Ceres6 commented on Jul 21, 2023

RafaelGSS commented on Jul 21, 2023

Ceres6 commented on Jul 21, 2023

RafaelGSS commented on Jul 21, 2023

Ceres6 commented on Aug 1, 2023

tniessen commented on Aug 1, 2023

RafaelGSS commented on Aug 1, 2023

LongTengDao commented on Aug 1, 2023

LongTengDao commented on Aug 1, 2023

RafaelGSS commented on Aug 2, 2023

LongTengDao commented on Aug 2, 2023

RafaelGSS commented on Aug 3, 2023

Ceres6 commented on Aug 3, 2023

RafaelGSS commented on Aug 4, 2023

RafaelGSS commented on Sep 22, 2023

Ceres6 commented on Sep 23, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions