-
Notifications
You must be signed in to change notification settings - Fork 745
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
Update the existing check to report on the status of Telerik #5077
Conversation
I tested the following scenario:
|
In my Telerik Identifier project - https://github.com/IowaComputerGurus/DnnTelerikIdentifier/blob/main/src/IowaComputerGurus.Dnn.TelerikIdentifier/Controllers/ContentController.cs I had to wrap the call to get referenced assemblies in a try-catch because of certain third-party modules that deployed native compiled dll's that couldn't be loaded by .NET for inspection. Do we believe we should do something similar here? One example of those exceptions from a bug report we had
|
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.
I think @mitchelsellers is talking about this pr right? https://github.com/IowaComputerGurus/DnnTelerikIdentifier/pull/1/files
Other than that, it looks awesome, thanks a lot for that contribution. Do you guys think we should add some note about Telerik being forcibly removed in a future version?
Other than @mitchelsellers concern, I am approving this.
Yes, it was a concious decision not to catch the exceptions because I noticed that exceptions are caught by the AuditChecks class, here. It seems exceptions that occur in the security checks are shown in the Notes panel. But I agree, maybe it's better not to show these raw exception errors and wrap the code in a try...catch to compose a more user friendly error message and avoid disclosing server physical paths and cluttering the UI with technical details. |
Given the high percentage of DNN users that will see this, I think it should be swallowed and only noted. (At least 3 popular third-party solutions include these types of assemblies.) But as @valadas said, this is awesome otherwise |
Ok, with the last set of commits I addressed the following:
Here's a screenshot showing all of the above: I think this is now ready for review. |
We want this to target the 9.11.0 branch, right? |
@bdukes I don't see a reason this could not be in a possible 9.10.3, do you have any concerns about it ? |
It's not a bug fix and it's part of the larger effort for 9.11. No specific concerns |
Cool, works for me |
Oh wait it was tagged 9.11 but still targetted develop when I merged, sorry... |
I wouldn't worry about reverting |
Ok, I just changed the milestone, sorry for the confusion... |
Closes #4947
Summary
This pull request replaces the existing
CheckTelerikVulnerability
security check with the newCheckTelerikPresence
security check. As defined in #4947, the new check will:Wording and logic for Telerik dependency detection adapted from the Dnn Telerik Identifier.