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

Merging User .config Preferences #297

Closed
Jason3S opened this issue Mar 17, 2023 · 4 comments
Closed

Merging User .config Preferences #297

Jason3S opened this issue Mar 17, 2023 · 4 comments

Comments

@Jason3S
Copy link

Jason3S commented Mar 17, 2023

Question

Does it make sense to "merger" the searchPlaces instead of replacing them? I can understand if a User wants to add their preferred places, but replacing the ones specified by the cli-tool can easily cause things to break.

For example, cspell has multiple places for a configuration file. It has a defined search pattern. Sub folders and parent folders might both have config files. If the .config.js at the root is there to clean up the root, it would mess up the sub folders.

Case in point, if someone adds a .config.js to adjust the location of prettier, they could break cspell or eslint because a "single" config is applied to ALL tools that use Cosmiconfig.

Originally posted by @Jason3S in #296 (comment)

Discussion

The idea of this "meta config" concept was always to put the end user in a position of power over where their config is. I can maybe see some way to notify the user via a console warning when an empty configuration is returned while a searchPlaces override is in place. This would probably include a way to silence said warning, too.

I don't like the idea of merging the places by default. Maybe there could be an option to enable merging? Maybe even just for specified libraries? I don't know what would be best yet, but these ideas should probably be refined within a separate issue.

Originally posted by @d-fischer in #296 (comment)

@Jason3S
Copy link
Author

Jason3S commented Mar 17, 2023

@d-fischer,

What is wrong with merging the searchPlaces? Are not `searchPlaces looked at in order? If the User's preferences come first, won't that work?

@Jason3S
Copy link
Author

Jason3S commented Mar 18, 2023

@d-fischer,

I'm concerned about the security of running .config.js without explicit permissions.

  1. I was surprised it ran when it wasn't expected.
  2. It is another attack vector (If an attacker adds a .config.js to a PR, it will get executed during prettier or eslint).
  3. .config.js is used for other things besides cosmiconfig.
  4. .config.js will crash a tool if used in a package of type module, even if it is valid ESM.

@d-fischer
Copy link
Member

What is wrong with merging the searchPlaces? Are not `searchPlaces looked at in order? If the User's preferences come first, won't that work?

To be honest, on second thought, it might be better to merge by default. Right now, I would consider this to be a breaking change though, and as such I can not introduce this before a major bump. (Which I don't want to do now because there's #152 and #292, both of which I want to have for 9.0, and I kinda want #283 to get released before the major version bump)

Still, I definitely want to enable users to disable merging as an optimization if they so wish.

I was surprised it ran when it wasn't expected.
It is another attack vector (If an attacker adds a .config.js to a PR, it will get executed during prettier or eslint).

Isn't that just the same as other .js config files, which cosmiconfig has been supporting for quite a while?
(also, side note, eslint does not use cosmiconfig)

.config.js is used for other things besides cosmiconfig.

It is designed to be very unlikely to clash with other libraries. Can you give me examples of other things that use it, and do these have problems with this (after the bug fixes)?

.config.js will crash a tool if used in a package of type module, even if it is valid ESM.

#283 should fix that.

@d-fischer
Copy link
Member

Closing this as merging will be default in v9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants