Skip to content

Conversation

st0012
Copy link
Member

@st0012 st0012 commented May 25, 2022

Background

I want to add recommend configs like this to Rails projects:

# .rdbgrc.rb
DEBUGGER__::CONFIG[:skip_path] = Gem.path.join(":")
DEBUGGER__::CONFIG[:use_short_path] = true

Ideally, it'll reduce the full backtrace output from

=>#0    Post#title at ~/projects/rails-guide-example/app/models/post.rb:7
  #1    block {|attribute=:title|} in validate at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activemodel-7.0.1/lib/active_model/validator.rb:150
  # and 105 frames (use `bt' command for all frames)
(rdbg) bt    # backtrace command
=>#0    Post#title at ~/projects/rails-guide-example/app/models/post.rb:7
  #1    block {|attribute=:title|} in validate at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activemodel-7.0.1/lib/active_model/validator.rb:150
  #2    [C] Array#each at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activemodel-7.0.1/lib/active_model/validator.rb:149
  #3    ActiveModel::EachValidator#validate(record=#<Post id: nil, title: "", content: "", ...) at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activemodel-7.0.1/lib/active_model/validator.rb:149
  #4    block {|target=#<Post id: nil, title: "", content: "", ..., value=nil, block=nil|} in make_lambda at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:423
  #5    block in halting (2 levels) at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:199
  #6    block in default_terminator (2 levels) at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:687
  #7    [C] Kernel#catch at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:686
  #8    block {|target=#<Post id: nil, title: "", content: "", ..., result_lambda=#<Proc:0x0000000113436f70 /Users/st0012/...|} in default_terminator at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:686
  #9    block {|env=#<struct ActiveSupport::Callbacks::Filte...|} in halting at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:200
  #10   block {|b=#<Proc:0x00000001134372e0 /Users/st0012/...|} in invoke_before at ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activesupport-7.0.1/lib/active_support/callbacks.rb:595
  
  # and other 90+ frames

To just app related backtrace

(rdbg) bt    # backtrace command
=>#0    Post#title at ~/projects/rails-guide-example/app/models/post.rb:7
  #36   block {|format=#<ActionController::MimeResponds::Collec...|} in create at ~/projects/rails-guide-example/app/controllers/posts_controller.rb:28
  #38   PostsController#create at ~/projects/rails-guide-example/app/controllers/posts_controller.rb:27

But there are some blockers and I need to make some changes for them.

Changes

  1. Support project-based .rdbgrc(.rb) files. Currently the debugger only reads ~/.rdbgrc(.rb) files but we'll need the flexibility for per-project configurations. Since irb also supports /.irbrc, I think this also fits our tooling convention.

  2. When comparing paths, we need to expand the values for accurate result. This is because frame paths are usually presented unexpanded:

    ~/.rbenv/versions/3.1.0/lib/ruby/3.1.0/irb/workspace.rb
    

    But important Ruby/Rubygem paths are all stored expanded:

    RbConfig::CONFIG["rubylibdir"] #=> "/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/3.1.0"
    Gem.path #=> ["/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0", "/Users/st0012/.gem/ruby/3.1.0"]
    
  3. We shouldn't use FrameInfo#location_str for frame filtering because it's mainly for displaying and its value can be changed by the use_short_path config. As an alternative, I added FrameInfo#real_location for that purpose.

  4. The pty_home_dir should be just the original HOME on CI because

    1. We don't need to alter home path to avoid ~/.rdbgrc pollution on CI
    2. HOME path is usually not assignable on CIs

@st0012 st0012 changed the title Config improvements Support per-project config file and fix backtrace filtering May 25, 2022
@st0012
Copy link
Member Author

st0012 commented Jun 24, 2022

@peterzhu2118 Could I have some look on this too? Thx

@st0012 st0012 force-pushed the config-improvements branch 3 times, most recently from 5a0acbb to 96c6edf Compare July 1, 2022 22:22
@peterzhu2118
Copy link
Member

@st0012 This looks like it's pretty consistently failing on CI. I've retried it and it's still failing.

@st0012
Copy link
Member Author

st0012 commented Jul 2, 2022

Yeah I changed something for testing and it didn't work.
I'm still trying to repro all the path filtering scenarios in tests and I'll give it another try in the weekend.

@st0012 st0012 force-pushed the config-improvements branch from 753e45f to 344d6fc Compare July 3, 2022 11:42
Each project may have its own project-specific debugger configuration,
like different paths to skip. So the debugger should also read rdbgrc
files at the project root, similar to what irb does with irbrc.
@st0012 st0012 force-pushed the config-improvements branch 3 times, most recently from 2548e92 to 629773f Compare July 3, 2022 14:43
st0012 added 3 commits July 3, 2022 15:48
Frame paths are usually presented unexpanded:

```
~/.rbenv/versions/3.1.0/lib/ruby/3.1.0/irb/workspace.rb
```

But important Ruby/Rubygem paths are all stored expanded:

```
RbConfig::CONFIG["rubylibdir"] #=> "/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/3.1.0"
Gem.path #=> ["/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0", "/Users/st0012/.gem/ruby/3.1.0"]
```

So expanding frame paths before matching is necessary.
And on the otherhand, users may also provide unexpanded skip_path.
Therefore we should expand it too.
FrameInfo#location_str uses #pretty_path underneath, which's value can
change when use_short_path is enabled. This makes frame filtering
unreliable and we should avoid it by using a more static attribute
instead.
@st0012 st0012 force-pushed the config-improvements branch from 629773f to 5fdf866 Compare July 3, 2022 14:49
@st0012 st0012 requested a review from peterzhu2118 July 3, 2022 15:12
Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@peterzhu2118 peterzhu2118 merged commit 79e4511 into ruby:master Jul 3, 2022
@st0012 st0012 deleted the config-improvements branch July 3, 2022 15:36
@ko1
Copy link
Collaborator

ko1 commented Jul 3, 2022

At least 1 is not acceptable.

@ko1
Copy link
Collaborator

ko1 commented Jul 3, 2022

@peterzhu2118 please ask my review for such architecture level change.

@ko1
Copy link
Collaborator

ko1 commented Jul 3, 2022

@st0012 1 has security risk so it is not acceptable. Please check other debuggers.

@st0012
Copy link
Member Author

st0012 commented Jul 3, 2022

AFAIK, the major concern is that user may run malicious code by loading the config from a malicious codebase.

If that's true, then here are what I found by checking other debuggers:

  • This threat is present in all major Ruby tools, including irb, pry and byebug.
  • Like Ruby's tools, both Elixir's IEX and Python's pdb support and enable working directory config by default.
  • gdb and lldb requires opt-in configuration to enable such feature.
  • delve (Golang) doesn't support working directory configuration file.

And in Ruby's case, even if we rule out non-official tools like pry and byebug, the malicious code can also be injected in so many other places, like:

  • Gemfile - loaded when running bundle exec
  • Rakefile - loaded in Rake tasks
  • .irbrc - loaded when running irb or rails console

I think for lldb and gdb, disabling working directory config does prevent malicious code loading effectively. But in Ruby's case, it's not nearly as effective. So I don't understand why .rdbgrc will impose a significant threat when compared to other Ruby tools and requires the entire PR being reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants