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

[misc] Use an allowlist for all JS includes #7210

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

Conversation

cincodenada
Copy link

@cincodenada cincodenada commented Mar 24, 2022

An exclusion list does not make much sense here - trying to include literally any file other than markdown and txt files - including dotfiles (if they have an extension), which results in errors lifting if there are things like editor temp files, which seems pretty unintended.

An inclusion list makes more sense here, and we have one already that we're using elsewhere, so go ahead and just use it any time we're including source files.

I ran into this because my editor left behind hidden dotfiles after editing some service files and Sails tried to require them, which was very unexpected.

By way of testing, I've added some files in the sampleapp that shouldn't be loaded as source. If y'all want a more specific test for this I can probably get that going, but these files successfully crash out the tests on master.

@sailsbot
Copy link

Hi @cincodenada! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@sailsbot
Copy link

Hi @cincodenada! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@cincodenada cincodenada changed the title Use BASIC_SUPPORTED_FILE_EXTENSIONS for all JS includes [misc] Use BASIC_SUPPORTED_FILE_EXTENSIONS for all JS includes Mar 24, 2022
@sailsbot
Copy link

Thanks for submitting this pull request, @cincodenada! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

Copy link
Contributor

@luislobo luislobo left a comment

Choose a reason for hiding this comment

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

This seems like a better approach than the original one

Ell Bradshaw and others added 4 commits March 9, 2023 15:40
An exclusion list does not make much sense here - trying to include
literally any file other than markdown and txt files - including
dotfiles - results in errors lifting if there are things like editor
temp files, .gitignore, and so on.

An inclusion list makes more sense here, and we have one already that
we're using elsewhere, so go ahead and just use it any time we're
including source files.
This crashes out the tests, which accomplishes testing this
The guidelines said to add it under master which wasn't there, so I
added it
This broke loading views otherwise
@cincodenada cincodenada changed the title [misc] Use BASIC_SUPPORTED_FILE_EXTENSIONS for all JS includes [misc] Use an allowlist for all JS includes Mar 9, 2023
@cincodenada
Copy link
Author

Hello! I ran into this issue again twice in the last two days, and it materialized in two different ways, so I circled back to check on this. There were tests failing because .ejs files weren't included in the list, so I reworked this to add those, so tests are now passing.

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

Successfully merging this pull request may close these issues.

None yet

3 participants