phlex-ruby / phlex-rails

An object-oriented alternative to ActionView for Ruby on Rails.
https://www.phlex.fun
MIT License
229 stars 40 forks source link

Change the way autoload paths are added to application.rb #205

Closed trinitytakei closed 2 months ago

trinitytakei commented 5 months ago

The Problem

As described here: https://discord.com/channels/1082611227827638303/1082612756676620318/1255932550837309573 brakeman fails to parse config.application.rb after the phlex installer adds the extra autoload paths. As a result, it mistakenly reports a "Cross-Site Request Forgery" issue, and thus the scan_ruby job in ci.yml fails. (brakeman and ci.yml with a scan_ruby job are Omakase Rails since 7.2.beta).

The Proposed Solution

After some debugging, I figured out the code to be inserted into config/application.rb that is

🤔

This is not a phlex-rails issue per se (the original code generated by phlex:install is not 'less valid' or worse in any way than the proposed code). However, I believe the fix still makes sense, because it's a simple one that works right now, won't cause any trouble down the line, and will hopefully save some headache for Phlex + brakeman users.

The proper/long term fix of course is to handle this in brakeman. I consulted with a security expert in the know, and he confirmed that brakeman's current parser is indeed not great; Prism to the rescue (at some point in the future).

trinitytakei commented 3 months ago

Just pushed rubocop fixes (I'm normally adding those with git commit -v -a --no-edit --amend - let me know if you prefer to have an explicit commit (and probably squash during merging, unless you are keen on keeping the history)).

trinitytakei commented 3 months ago

So NOW it is weird. Rubocop 'fixed' this:

        ADD_EXTRA_AUTOLOAD_PATHS_CODE = <<-ADD_EXTRA_AUTOLOAD_PATHS_CODE
    config.autoload_paths.push(
      "\#{root}/app/views/components",
      "\#{root}/app/views",
      "\#{root}/app/views/layouts"
    )

                ADD_EXTRA_AUTOLOAD_PATHS_CODE

No idea why the weird indentation?

joeldrapper commented 3 months ago

Sometimes Rubocop does weird auto-correct things with indentation. What do you think about this indentation?

Using <<~ so Ruby strips the leading whitespace. I believe this will only strip the tabs. Additionally, using RUBY as the delineator means some editors will highlight it as Ruby.

CleanShot 2024-08-14 at 12 48 24@2x

joeldrapper commented 3 months ago

Oh no, I’m wrong. It strips the spaces too. 😢

joeldrapper commented 3 months ago

Don’t try too hard to please Rubocop. If Rubocop is wrong, we’ll change the Rubocop configuration.

trinitytakei commented 3 months ago

Using <<~ so Ruby strips the leading whitespace.

Yeah, I toyed with that too, but between Rubocop and how I wanted it to look like, I culdn't figure it out.

Additionally, using RUBY as the delineator means some editors will highlight it as Ruby.

Sounds good. Heredoc syntax highlighting is underrated imo: https://mhenrixon.com/articles/heredoc-syntax-highlighting

joeldrapper commented 3 months ago

Good to merge after the heredoc update. 👍

trinitytakei commented 2 months ago

@joeldrapper Sorry if I'm missing something, but is anything required of me at this point (CI is 🟢)?

joeldrapper commented 2 months ago

All good and merged. Thank you!