-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: master
Are you sure you want to change the base?
Conversation
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 |
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 |
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:
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. |
bb7e932
to
81802c3
Compare
There was a problem hiding this 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
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
569ed65
to
9253abc
Compare
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 |
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.