-
Notifications
You must be signed in to change notification settings - Fork 38
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
FileInclude: check for restricted files. #122
FileInclude: check for restricted files. #122
Conversation
|
@dmtrmrv Have you seen the I think it would be a good idea to combine your work with that sniff rather than having it as a separate sniff. Logic wise I imagine your check would come first and throw an error if any of these file names are found. If not, the already existing check which throws a warning should be executed. |
Thanks for the feedback, guys. @jrfnl yes, I think this makes sense. Should I update the PR? And what about admin.php? I have mixed feelings about not including that file. On one hand, like @ernilambar mentioned, the filename is too generic, on the other, I prefer to call theme-related files something like theme-admin.php or project-admin.php to avoid any confusion, but that's just me. |
And another thing, I can't quite figure out why the checks are failing. Could you point me in the right direction so I can fix it? Thanks! |
Please do.
That is something for someone on the TRT to have an opinion on /cc @grappler @justintadlock
If you click on the "Details" link next to the failed build/commit, it will take you to a page listing all the builds tested. Any build with a red |
Thank you for the PR! 👍 Regarding The sniff is failing because it is running out of memory. How much memory is the sniff using when you are running it on a file locally? |
@grappler No problem, thanks for the comment! Will update the PR. Also, could you expand a bit on real life examples that should be restricted? Regarding the memory, I was testing different include/require cases separately and there were no problems with the memory usage. But now, after I ran the same sniff on the actual test file, it ran out of memory on my local machine as well. I'll ty to use |
Just wondering how you're getting on with this @dmtrmrv ... |
@jrfnl Sorry for the delay guys, shame on me. Will update the PR today. |
@dmtrmrv No worries, just making sure this PR has not been abandoned. |
Looks like it's all working now, but some tests are still failing. First, it was because I didn't write the |
Please undo that. Upstream is merged periodically into the As for the build failure:
|
8f4088f
to
271b8c7
Compare
Thanks for the clarification, all should be alright now. |
Status:
@dmtrmrv Hi Dmitry, new life has been breathed into this project. Are you still interested in bringing this sniff over the finish line and would you have time to work on that with me over the next few weeks ? |
Hi, @jrfnl. Sorry for a delay with the follow-up. If the question is still relevant, I'll be glad to get this over the finish line. |
This PR is addressing issues #87 and #44. Took me a while, but I managed to finish this (I think).
The approach is really simple, the sniff detects one of the include/require statements and checks if it is used to include one of the restricted files. If yes, it throws an error.
Any feedback is highly appreciated as I'm new to writing sniffs.