Use allowlist for rails/rake arguments, not blocklist - #6
Closed
grantcox wants to merge 2 commits into
Closed
Conversation
`rake -e/-p/-E CODE` evaluates CODE inside Rake's own option parser --
before the Rakefile is loaded, without booting Rails -- and then exits,
so the console audit hook records nothing at all. That is weaker than
the `rails runner 'system("bash")'` case the README accepts as best
effort, where Rails at least boots and the invocation is recorded.
Rails hands any command it does not recognise to that same parser with
the whole argv, so `rails -e CODE` and `rails db:migrate -e CODE` reach
it too.
Allowlisted rather than screened. A deny list has to model which of
Rake's short options take an argument, in order to know where a bundle
such as `-Ne` or `-se` stops being flags. Wrong for one option, in this
version of Rake or a later one, and the bundle hides an `-e`. An
allowlist fails the other way: an unlisted option is refused, so being
wrong costs a denial rather than an unlogged shell. It also refuses
what nobody thought to exclude -- `-g`/`--system` loads tasks from
$HOME/.rake, which is /app/.rake on a dyno, and dumping Rake's option
table is the only reason it was noticed.
Options are matched as whole tokens, so `-s` is permitted while `-se`
is a different token and is refused. That keeps the bundling reasoning
out of the wrapper entirely.
Every command gets a list; none is exempt. `rails console` and `rails
runner` parse their own options and never reach Rake, so `-e` there is
the environment -- but they get a list of their own rather than an
exemption, because a mistake in a list is a denial while a mistake in
an exemption is a bypass, silently and with no failing test.
`rails runner -s` was permitted and is now refused: `-s` is not a
runner option in Rails 8.1. What that test pinned -- that the sandbox
denial is scoped to the console -- is still pinned by `rake -s` and
`rails c -s`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thor parses `--sandbox=true` as well as the bare flag, and the rule matched `--sandbox|-s` exactly, so the `=` forms fell through to the option allowlist. They were refused there, but for the wrong reason and with a banner that says nothing about why a sandboxed console is dangerous -- and the coverage was incidental: adding a sandbox-ish entry to the console's allowlist would have reopened the bypass with the whole suite green. Denied whatever value they carry, `false` included. Deciding which values Thor reads as true means modelling the parser, which is the thing the option allowlist exists to avoid. `--no-sandbox` stays allowlisted and is named in the denial, so the operator who wanted the negative form is told the spelling that works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Closing in favor of #8 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In response to #1 (comment)
This PR updates the buildpack checks to have a limited allowlist of arguments for
railsandrakecommands, rather than looking for unsafe arguments to block. There are many arguments, and each can have many aliases (eg-T,--tasks,--task,--tasall mean "rake list tasks"). Having an allowlist and requiring full arguments (eg-Tor--tasks) blocks many possible edge cases.As this introduces a general rejection message, we also tighten up the "no sandbox permitted" check, so the unique error response we have for that continues to apply.