Skip to content

Ruby: try/try! as code execution #11022

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

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Ruby: try/try! as code execution #11022

merged 2 commits into from
Nov 7, 2022

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Oct 27, 2022

Recognise Object#try(!) as a code execution. These methods are ActiveSupport extensions. Evaluation shows a slow-down for canvas-lms, but I think that's because it contains a lot of calls to try (848 vs 389 for gitlabhq). New evaluation seems fine.

@hmac hmac force-pushed the try-code-injection branch from cbb5c67 to 0dd63c0 Compare October 30, 2022 22:53
@hmac hmac marked this pull request as ready for review October 31, 2022 00:38
@hmac hmac requested a review from a team as a code owner October 31, 2022 00:38
@calumgrant calumgrant requested a review from erik-krogh November 7, 2022 09:38
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Optional cosmetic suggestion.

* `Object#try!` behaves similarly but raises `NoMethodError` if the
* receiver is not `nil` and does not respond to the method.
*/
class TryCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class TryCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode {
class TryCallCodeExecution extends CodeExecution::Range instanceof DataFlow::CallNode {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll open a new PR for this change to avoid having to ask for re-review on this one.

@hmac hmac merged commit d392cda into github:main Nov 7, 2022
@hmac hmac mentioned this pull request Nov 7, 2022
@hmac hmac deleted the try-code-injection branch November 7, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants