Skip to content

Fixes #39447 - Guard dangerous host bulk actions with scope hash#11038

Open
sbernhard wants to merge 2 commits into
theforeman:developfrom
ATIX-AG:fix_39447
Open

Fixes #39447 - Guard dangerous host bulk actions with scope hash#11038
sbernhard wants to merge 2 commits into
theforeman:developfrom
ATIX-AG:fix_39447

Conversation

@sbernhard

@sbernhard sbernhard commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

This guard uses digests of user ID, location ID, organization ID, and all host IDs a user has access. This digest is passed to the UI and forwarded to 'some' bulk actions like delete, power off which can really 'destroy' a datacenter managed by foreman. In the bulk action the hash is recalculated and only if the hash stays the same, the bulk action is executed. Not perfect, but I think it solves both issues.

@sbernhard

Copy link
Copy Markdown
Contributor Author

the Katello test calls bulk_destroy directly with only:
{ :search => "id ^ (...)" }

  • bulk_destroy is now guarded by validate_scope_hash!
  • without scope_hash or skip_scope_hash, the controller returns:
    • 409
    • "The host scope changed or is missing integrity data..."

The failing test is here: katello/test/controllers/api/v2/hosts_bulk_actions_controller_extensions_test.rb:11

@jeremylenz jeremylenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @sbernhard! I like this idea a lot; just have a question/concern below.

Comment thread app/services/hosts/bulk_scope_hash.rb Outdated
@sbernhard

Copy link
Copy Markdown
Contributor Author

Found issues in error toast handling for disassociate bulk action: #11045

@sbernhard

Copy link
Copy Markdown
Contributor Author

Currently, the guard is only for the "most critical" bulk actions. It would be pretty easy to extend this for all bulk actions in the hosts controller. Already talked to @nadjaheitmann - she would prefer this. What's your opinion @jeremylenz?

@jeremylenz

Copy link
Copy Markdown
Contributor

@sbernhard Thanks for doing all the performance testing! I agree that "one-shot pluck" looks like a good compromise of increasing performance while still keeping the readability and maintainability of Ruby code. (Still amazing how fast SQL is, though 😄) I also think one-shot pluck is fast enough (based on your numbers) that extending it to all controller actions should be fine. 👍

@nadjaheitmann

Copy link
Copy Markdown
Contributor

@sbernhard @jeremylenz The guard does not save us from implementing location parameter evaluation for the host bulk actions controller which is done in the PR that I have opened.

@jeremylenz jeremylenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested and works well after I made one change.

Comment thread app/controllers/api/v2/hosts_bulk_actions_controller.rb Outdated
Comment thread app/controllers/api/v2/hosts_bulk_actions_controller.rb Outdated
@sbernhard sbernhard force-pushed the fix_39447 branch 2 times, most recently from a5cd7da to 0bb4d49 Compare June 25, 2026 21:05
Comment thread app/controllers/api/v2/hosts_bulk_actions_controller.rb
Comment thread app/controllers/api/v2/hosts_bulk_actions_controller.rb Outdated
Comment thread app/controllers/api/v2/hosts_bulk_actions_controller.rb Outdated
@sbernhard sbernhard force-pushed the fix_39447 branch 3 times, most recently from 7040a69 to 6cceb23 Compare June 29, 2026 14:23
sbernhard and others added 2 commits June 29, 2026 16:45
Compute a scope hash from the current user, current taxonomy context,
and all currently accessible host ids of the user, expose it to
the new hosts page, and require it on bulk actions.
The backend now rejects those actions with 409 when the hash is
missing or stale, which fails closed on cross-tab taxonomy changes
and host set drift.

Co-authored-by: OpenAI Codex <codex@openai.com>
…r toast

Co-authored-by: OpenAI Codex <codex@openai.com>
@sbernhard sbernhard marked this pull request as ready for review July 3, 2026 15:44
@sbernhard

Copy link
Copy Markdown
Contributor Author

We also need:
#11045
Katello/katello#11790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants