Skip to content
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

remove redirect for setup wazard #301

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thomaseude
Copy link

@thomaseude thomaseude commented Dec 2, 2023

Currently if we :set_steps with a route look like : flows_onboarding_user_path(Wicked::FIRST_STEP, new_business: "yes"). It'll redirect_to first_step and overwrite my :set_steps.

The problem is Wicked::FIRST_STEP, it'll work perfectly if I replace by the step_id

Below the code that I try to achieve :

CONTROLLER

class Flows::OnboardingUserController < ApplicationController
  include Wicked::Wizard
  before_action :set_steps
  before_action :setup_wizard

  def show
     ...
  end

  def update
    ...
  end
  
   def set_steps
    if params[:steps]
      self.steps = params[:steps]
    elsif params[:new_business] == "yes"
      self.steps = [:platforms, :business_activity, :links, :reply]
    else
        self.steps = [:reply, :platforms, :business_activity, :company, :user]
    end
  end
end

ROUTE
flows_onboarding_user_path(**Wicked::FIRST_STEP**, new_business: "yes")

@schneems
Copy link
Member

This code would create an open render vulnerability where any attacker could manipulate your app to display every file on disk, including your environment variables or other secrets files

self.steps = params[:steps]

I would not accept a patch to fix this specific example use case as it is insecure.

@schneems
Copy link
Member

Looks like the tests are failing. Is main working or is it from your patch?

I would accept a patch to run tests on main on a chron if we don’t already have that in there.

@thomaseude
Copy link
Author

thomaseude commented Dec 12, 2023

Thanks for your feedback !

I correct and do something like that, it should fix the vulnerability problem

class Flows::OnboardingUserController < ApplicationController
  include Wicked::Wizard
  before_action :set_steps
  before_action :setup_wizard

  VALID_STEPS = [:platforms, :business_activity, :links, :reply, :company, :user].freeze
  ....
   def set_steps
    if params[:steps]
      self.steps = params[:steps].select { |step| VALID_STEPS.include?(step.to_sym) }
    elsif params[:new_business] == "yes"
      self.steps = [:platforms, :business_activity, :links, :reply]
    else
        self.steps = [:reply, :platforms, :business_activity, :company, :user]
    end
  end
end

Test are falling here, I don't know why :
/wicked/test/integration/navigation_test.rb

test 'pointer to first' do
    visit(bar_path(Wicked::FIRST_STEP))
    assert_has_content?('first')
  end

  test 'pointer to last' do
    visit(bar_path(Wicked::LAST_STEP))
    assert_has_content?('last_step')
  end

On main branch (without my update), test work well

Maybe instead of to change the code, we could update the doc, I find a way to make my use case working. I add step = :platforms

class Flows::OnboardingUserController < ApplicationController

  VALID_STEPS = [:platforms, :business_activity, :links, :reply, :company, :user].freeze
  ...
   def set_steps
    if params[:steps]
      self.steps = params[:steps].select { |step| VALID_STEPS.include?(step.to_sym) }
    elsif params[:new_business] == "yes"
      step = :platforms
      self.steps = [:platforms, :business_activity, :links, :reply]
    end
   end
end

Also. something that I'd like to confirm with you, every time that my page will be reload, :set_steps and :setup_wizard will be called. There is no better way to keep in mind the steps that set as params like I do above ?

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.

2 participants