Skip to content

Ruby: Model Sinatra #11954

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 9 commits into from
Mar 16, 2023
Merged

Ruby: Model Sinatra #11954

merged 9 commits into from
Mar 16, 2023

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Jan 23, 2023

I had hoped to catch a few more CVEs with this, but sadly they require a bit more in the way of modeling. Opening this for review now so others can build on it.

@github-actions github-actions bot added the Ruby label Jan 23, 2023
Comment on lines 171 to 175
/**
* A summary for accessing a local variable in an ERB template.
* This is the second half of the modelling of the flow from the `locals` keyword argument to variables in the ERB template.
* The first half is modeled by `ErbLocalsSummary`.
*/

Check warning

Code scanning / CodeQL

Misspelling

This comment contains the non-US spelling 'modelling', which should instead be 'modeling'.
@hmac hmac marked this pull request as ready for review March 13, 2023 08:33
@hmac hmac requested a review from a team as a code owner March 13, 2023 08:33
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

This looks great - no real suggestions.

*/
class ErbLocalsHashSyntheticGlobal extends SummaryComponent::SyntheticGlobal {
private string id;
private MethodCall erbCall;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that making this an ErbCall leads to non-monotonic recursion problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly right.

/**
* Like `Location.toString`, but displays the relative path rather than the full path.
*/
private string locationRelativePathToString(Location loc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does using this instead of toString make a difference in practice, or is it more a case of neatness/readability of the synthetic global ID?

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 don't know if there's any performance difference or otherwise - I just liked having a slightly neater ID 😀

@hmac hmac merged commit 2c63dba into github:main Mar 16, 2023
@hmac hmac deleted the sinatra branch March 16, 2023 21:47
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