Skip to content

Ruby: Model various ActionController methods #11058

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 10 commits into from
Nov 16, 2022

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Oct 31, 2022

ActionController methods have access to a logger method which returns an instance of ActiveSupport::Logger.

The first commit moves the existing ActionController.ql test to be inside the action_controller directory, so that is no longer affected by changes to other test fixtures. I think we should probably do this for the remaining tests (ActionDispatch, ActionView, Core, GraphQL, PosixSpawn) at some point.

Later commits model send_data and body_stream.

Evaluation shows new TPs for log injection, with one FP due to us not considering Object#inspect to be a sanitizer. I've added a commit to fix this FP.

@hmac hmac force-pushed the actioncontroller-logger branch from 9f29317 to 4bea04b Compare October 31, 2022 23:38
@hmac hmac removed the QL-for-QL label Oct 31, 2022
@hmac hmac changed the title Ruby: ActionController logger Ruby: Model various ActionController methods Nov 1, 2022
@hmac hmac force-pushed the actioncontroller-logger branch 2 times, most recently from c0c6c3a to 8ccce80 Compare November 3, 2022 22:25
@hmac hmac marked this pull request as ready for review November 3, 2022 23:18
@hmac hmac requested a review from a team as a code owner November 3, 2022 23:19
@calumgrant calumgrant requested a review from nickrolfe November 7, 2022 09:38
nickrolfe
nickrolfe previously approved these changes Nov 7, 2022
Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

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

Changes LGTM. I've started another DCA run, since the last one had some (presumably transient) failures.

@hmac
Copy link
Contributor Author

hmac commented Nov 7, 2022

That run shows a stage timing (?) slowdown for Regexp::regExpSource(rb/incomplete-hostname-regexp[11], 7/7) on gitlabhq/gitlabhq. Should we be worried about that?

@erik-krogh
Copy link
Contributor

That run shows a stage timing (?) slowdown for Regexp::regExpSource(rb/incomplete-hostname-regexp[11], 7/7) on gitlabhq/gitlabhq. Should we be worried about that?

That is a fluke that you shouldn't worry about.
Your PR doesn't affect that predicate (I checked), so there's no way your PR caused a regression.

But you have merge conflicts beyond the test-files that GitHub is complaining about (e.g. private class ActionControllerContextCall has disappeared on main), so you need to update the PR to reflect those changes.

hmac added 10 commits November 16, 2022 13:46
This test shows that we correctly identify redirect_to and render calls
inside respond_to blocks.
The behaviour of `Object#inspect` depends on whether it has been
overridden by a subclass, but it will typically produce output on a
single line. Calling `inspect` on a String will replace newlines with
`\n`, which is then safe for interpolation into a log line.
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.

A new evaluation still looks good.

👍

@hmac hmac merged commit a6f6936 into github:main Nov 16, 2022
@hmac hmac deleted the actioncontroller-logger branch November 16, 2022 19:21
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.

3 participants