hpi-schuelerklub/workshop-portal

View on GitHub

Showing 208 of 208 total issues

Unescaped model attribute
Open

    <%= @event.custom_application_fields

Cross-site scripting (or XSS) is #3 on the 2013 [OWASP Top Ten](https://www.owasp.org/index.php/Top_10_2013-A3-Cross-Site_Scripting_(XSS\)) web security risks and it pops up nearly everywhere.

XSS occurs when a user-controlled value is displayed on a web page without properly escaping it, allowing someone to inject Javascript or HTML into the page which will be interpreted and executed by the browser..

In Rails 2.x, values need to be explicitly escaped (e.g., by using the h method). Since Rails 3.x, auto-escaping in views is enabled by default. However, one can still use the raw or html_safe methods to output a value directly.

See the Ruby Security Guide for more details.

Query Parameters and Cookies

ERB example:

<%= params[:query].html_safe %>

Brakeman looks for several situations that can allow XSS. The simplest is like the example above: a value from the params or cookies is being directly output to a view. In such cases, it will issue a warning like:

Unescaped parameter value near line 3: params[:query]

By default, Brakeman will also warn when a parameter or cookie value is used as an argument to a method, the result of which is output unescaped to a view.

For example:

<%= raw some_method(cookie[:name]) %>

This raises a warning like:

Unescaped cookie value near line 5: some_method(cookies[:oreo])

However, the confidence level for this warning will be weak, because it is not directly outputting the cookie value.

Some methods are known to Brakeman to either be dangerous (link_to is one) or safe (escape_once). Users can specify safe methods using the --safe-methods option. Alternatively, Brakeman can be set to only warn when values are used directly with the --report-direct option.

Model Attributes

Because (many) models come from database values, Brakeman mistrusts them by default.

For example, if @user is an instance of a model set in an action like

def set_user
  @user = User.first
end

and there is a view with

<%= @user.name.html_safe %>

Brakeman will raise a warning like

Unescaped model attribute near line 3: User.first.name

If you trust all your data (although you probably shouldn't), this can be disabled with --ignore-model-output.

Avoid too many return statements within this method.
Open

      return 1
Severity: Major
Found in app/models/event.rb - About 30 mins to fix

    Potentially dangerous key allowed for mass assignment
    Open

          user_params.permit(:email, :name, :password, :password_confirmation, :role)

    Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash.

    Example:

    User.new(params[:user])

    Unfortunately, if there is a user field called admin which controls administrator access, now any user can make themselves an administrator.

    attr_accessible and attr_protected can be used to limit mass assignment. However, Brakeman will warn unless attr_accessible is used, or mass assignment is completely disabled.

    There are two different mass assignment warnings which can arise. The first is when mass assignment actually occurs, such as the example above. This results in a warning like

    Unprotected mass assignment near line 61: User.new(params[:user])

    The other warning is raised whenever a model is found which does not use attr_accessible. This produces generic warnings like

    Mass assignment is not restricted using attr_accessible

    with a list of affected models.

    In Rails 3.1 and newer, mass assignment can easily be disabled:

    config.active_record.whitelist_attributes = true

    Unfortunately, it can also easily be bypassed:

    User.new(params[:user], :without_protection => true)

    Brakeman will warn on uses of without_protection.

    Loofah 2.0.3 is vulnerable (CVE-2018-8048). Upgrade to 2.1.2
    Open

        loofah (2.0.3)
    Severity: Minor
    Found in Gemfile.lock by brakeman

    rails-html-sanitizer 1.0.3 is vulnerable (CVE-2018-3741). Upgrade to 1.0.4
    Open

        rails-html-sanitizer (1.0.3)
    Severity: Minor
    Found in Gemfile.lock by brakeman

    Parameter value used in file name
    Open

        send_file file_full_path, x_sendfile: true

    Using user input when accessing files (local or remote) will raise a warning in Brakeman.

    For example

    File.open("/tmp/#{cookie[:file]}")

    will raise an error like

    Cookie value used in file name near line 4: File.open("/tmp/#{cookie[:file]}")

    This type of vulnerability can be used to access arbitrary files on a server (including /etc/passwd.

    Potentially dangerous key allowed for mass assignment
    Open

        params.require(:user).permit(:role)

    Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash.

    Example:

    User.new(params[:user])

    Unfortunately, if there is a user field called admin which controls administrator access, now any user can make themselves an administrator.

    attr_accessible and attr_protected can be used to limit mass assignment. However, Brakeman will warn unless attr_accessible is used, or mass assignment is completely disabled.

    There are two different mass assignment warnings which can arise. The first is when mass assignment actually occurs, such as the example above. This results in a warning like

    Unprotected mass assignment near line 61: User.new(params[:user])

    The other warning is raised whenever a model is found which does not use attr_accessible. This produces generic warnings like

    Mass assignment is not restricted using attr_accessible

    with a list of affected models.

    In Rails 3.1 and newer, mass assignment can easily be disabled:

    config.active_record.whitelist_attributes = true

    Unfortunately, it can also easily be bypassed:

    User.new(params[:user], :without_protection => true)

    Brakeman will warn on uses of without_protection.

    User controlled method execution
    Open

            @application_letters.sort_by! { |l| l.user.profile.send(params[:sort]) }

    Using unfiltered user data to select a Class or Method to be dynamically sent is dangerous.

    It is much safer to whitelist the desired target or method.

    Unsafe use of method:

    method = params[:method]
    @result = User.send(method.to_sym)

    Safe:

    method = params[:method] == 1 ? :method_a : :method_b
    @result = User.send(method, *args)

    Unsafe use of target:

    table = params[:table]
    model = table.classify.constantize
    @result = model.send(:method)

    Safe:

    target = params[:target] == 1 ? Account : User
    @result = target.send(:method, *args)

    Including user data in the arguments passed to an Object#send is safe, as long as the method can properly handle potentially bad data.

    Safe:

    args = params["args"] || []
    @result = User.send(:method, *args)

    Method create has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
    Open

      def create
        @profile = Profile.new(profile_params)
        @profile.user_id = current_user.id
    
        existing_profile = Profile.find_by(user: current_user.id)
    Severity: Minor
    Found in app/controllers/profiles_controller.rb - About 25 mins to fix

    Cognitive Complexity

    Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.

    A method's cognitive complexity is based on a few simple rules:

    • Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
    • Code is considered more complex for each "break in the linear flow of the code"
    • Code is considered more complex when "flow breaking structures are nested"

    Further reading

    Method phase has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
    Open

      def phase
        return :draft unless published
        return :application if published && !after_deadline?
        return :selection if published && after_deadline? && !(acceptances_have_been_sent && rejections_have_been_sent)
        return :execution if published && after_deadline? && acceptances_have_been_sent && rejections_have_been_sent
    Severity: Minor
    Found in app/models/event.rb - About 25 mins to fix

    Cognitive Complexity

    Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.

    A method's cognitive complexity is based on a few simple rules:

    • Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
    • Code is considered more complex for each "break in the linear flow of the code"
    • Code is considered more complex when "flow breaking structures are nested"

    Further reading

    Method new has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
    Open

      def new
        if !current_user
          message = I18n.t('application_letters.login_before_creation')
          flash[:event_id] = params[:event_id]
          flash.keep(:event_id)
    Severity: Minor
    Found in app/controllers/application_letters_controller.rb - About 25 mins to fix

    Cognitive Complexity

    Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.

    A method's cognitive complexity is based on a few simple rules:

    • Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
    • Code is considered more complex for each "break in the linear flow of the code"
    • Code is considered more complex when "flow breaking structures are nested"

    Further reading

    Method update_event has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
    Open

      def update_event(event)
        status = get_status
        if status == :acceptance
          event.set_status_notification_flag_for_applications_with_status(:accepted)
          event.acceptances_have_been_sent = true
    Severity: Minor
    Found in app/controllers/emails_controller.rb - About 25 mins to fix

    Cognitive Complexity

    Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.

    A method's cognitive complexity is based on a few simple rules:

    • Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
    • Code is considered more complex for each "break in the linear flow of the code"
    • Code is considered more complex when "flow breaking structures are nested"

    Further reading

    Similar blocks of code found in 2 locations. Consider refactoring.
    Open

      def set_notes
        @request = Request.find(params[:request_id])
        update_params = notes_params
        if !update_params[:notes].empty? && @request.update(update_params)
          redirect_to @request, notice: I18n.t('requests.notice.was_updated')
    Severity: Minor
    Found in app/controllers/requests_controller.rb and 1 other location - About 25 mins to fix
    app/controllers/requests_controller.rb on lines 44..51

    Duplicated Code

    Duplicated code can lead to software that is hard to understand and difficult to change. The Don't Repeat Yourself (DRY) principle states:

    Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

    When you violate DRY, bugs and maintenance problems are sure to follow. Duplicated code has a tendency to both continue to replicate and also to diverge (leaving bugs as two similar implementations differ in subtle ways).

    Tuning

    This issue has a mass of 29.

    We set useful threshold defaults for the languages we support but you may want to adjust these settings based on your project guidelines.

    The threshold configuration represents the minimum mass a code block must have to be analyzed for duplication. The lower the threshold, the more fine-grained the comparison.

    If the engine is too easily reporting duplication, try raising the threshold. If you suspect that the engine isn't catching enough duplication, try lowering the threshold. The best setting tends to differ from language to language.

    See codeclimate-duplication's documentation for more information about tuning the mass threshold in your .codeclimate.yml.

    Refactorings

    Further Reading

    Similar blocks of code found in 2 locations. Consider refactoring.
    Open

      def set_contact_person
        @request = Request.find(params[:request_id])
        update_params = contact_person_params
        if !update_params[:contact_person].empty? && @request.update(update_params)
          redirect_to @request, notice: I18n.t('requests.notice.was_updated')
    Severity: Minor
    Found in app/controllers/requests_controller.rb and 1 other location - About 25 mins to fix
    app/controllers/requests_controller.rb on lines 54..61

    Duplicated Code

    Duplicated code can lead to software that is hard to understand and difficult to change. The Don't Repeat Yourself (DRY) principle states:

    Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

    When you violate DRY, bugs and maintenance problems are sure to follow. Duplicated code has a tendency to both continue to replicate and also to diverge (leaving bugs as two similar implementations differ in subtle ways).

    Tuning

    This issue has a mass of 29.

    We set useful threshold defaults for the languages we support but you may want to adjust these settings based on your project guidelines.

    The threshold configuration represents the minimum mass a code block must have to be analyzed for duplication. The lower the threshold, the more fine-grained the comparison.

    If the engine is too easily reporting duplication, try raising the threshold. If you suspect that the engine isn't catching enough duplication, try lowering the threshold. The best setting tends to differ from language to language.

    See codeclimate-duplication's documentation for more information about tuning the mass threshold in your .codeclimate.yml.

    Refactorings

    Further Reading

    Rule doesn't have all its properties in alphabetical order.
    Open

    .ms-container .ms-list{

    Values of 0 shouldn't have units specified.
    Open

      padding: 5px 0px 0px 5px;

    Element (li.ms-elem-selectable) is overqualified, just use .ms-elem-selectable without element name.
    Open

    .ms-container .ms-selectable li.ms-elem-selectable,

    Don't use IDs in selectors.
    Open

    #event_overview ul {
    Severity: Minor
    Found in app/assets/stylesheets/events.css by csslint

    Don't use IDs in selectors.
    Open

    #applicants_overview {
    Severity: Minor
    Found in app/assets/stylesheets/events.css by csslint

    Rule doesn't have all its properties in alphabetical order.
    Open

    #wsp-event-show-banner {
    Severity: Minor
    Found in app/assets/stylesheets/events.css by csslint
    Severity
    Category
    Status
    Source
    Language