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

add autocomplete="OFF" to firefox-proof automagically added hidden fields like _method #42610

Closed
jpwynn opened this issue Jun 26, 2021 · 15 comments

Comments

@jpwynn
Copy link

jpwynn commented Jun 26, 2021

I suggest "bullet-proofing" any Rails-auto-added hidden fields by also auto-adding autocomplete="OFF"

As reported/diagnosed in May of WTFs: https://discuss.rubyonrails.org/t/form-with-first-field-value-is-overriden-with-a-token-like-string/74861/11 there is a 12-year-old Firefox bug that (sporadically, but pretty frequently) overwrites the first hidden_field in a form UNLESS it has autocomplete=OFF

The forum discussion doesn't propose any action other than nag Mozilla (who left it there for 12 years).

The results are to semi-randomly crash perfectly-coded Rails apps if, say, Rails inserts "_method=patch" but it gets submitted as "_method=5f4ledBRFGRYSUpaeJ29y-J0SX6KRSzbr1zSjVvgy1fhGmQAXXzsjLxdswyBtqopEnO6pQAaJTEFUJKXDVyisg" as in the specific example shown below (Firefox,on the left, Chrome on the right).

Because the addition of autocomplete=OFF does fortunately seem to workaround 100%, it seems to me to be a simple, safe, benign, no-side-effects addition. I'd submit a PR if I had any idea how.

sidebyside

@pixeltrix
Copy link
Contributor

Do you have a link to the Firefox bug report?

@pixeltrix
Copy link
Contributor

Nevermind - I can see it in the discussion post

@jpwynn
Copy link
Author

jpwynn commented Jun 26, 2021

There are a few, closed, the one that was reopened is https://bugzilla.mozilla.org/show_bug.cgi?id=520561

@pixeltrix
Copy link
Contributor

Thanks - has anyone tracked down in the Firefox source code where this is happening? Also do you have any idea where the autocomplete values are coming from? Seems like there must be some other web framework using _method and storing a Base64 encoded value in there.

I'm generally dubious about adding browser specific workarounds as they tend to get forgotten or treated as 'acts of faith' (e.g. for years Rails returned a single space in an empty response because of a Safari bug that had long been fixed). However I'm not 💯 percent against the idea - just need to confirm the exact conditions for this bug occurring.

@jpwynn
Copy link
Author

jpwynn commented Jun 26, 2021

Yeah I was hesitant to suggest it, except it is so darned pernicious and gnarly to debug for developers when it does happen, and the fix seems entirely benign (turning off autocomplete for a form field that is hidden hence should NOT be autocompleted anyway).

This other bug report does address reproducibility to some extent:
https://bugzilla.mozilla.org/show_bug.cgi?id=1667657

For my own rails app, while zipping back and forth between a few forms it works fine - until it doesn't and it "stays" broken until I close the browser. Hence the screen gran=b next time it happened.

@pixeltrix
Copy link
Contributor

This other bug report does address reproducibility to some extent: https://bugzilla.mozilla.org/show_bug.cgi?id=1667657

That one is really weird - it's not a generic name like _method that could be set by other sites, but a field name specific to that application. How is that getting a Base64 encoded value from autocomplete? Feels like there's more going on here than just Firefox autocomplete ignoring the fact that a field type is 'hidden'.

@jpwynn
Copy link
Author

jpwynn commented Jun 26, 2021

Right, not field name at all. I originally went to May of WTFs to report a bug where (I thought) the hidden field name had "key" like "video_key" it got stomped on by Rails, then saw that other report on WTFs and realized not the field name, and not Rails per se, but the field position, and Firefox specific.

But i did post the issue because the fix seems entirely benign: "turning off autocomplete for a form field that is hidden anyway hence should NOT be autocompleted anyway under any circumstances."

AFAIK no one knows the conditions "for this bug occurring" since it comes and goes... but what they do know is the conditions for stopping it: autocomplete=OFF really does seem to do the trick.

Ditto another form page with a little more complicated JQuery going on (in-browser video recording)... worked fine until it suddenly started post'ing corrupted video identifiers , but in that case, being my form already using POST, I had control over all the form fields (except the authenticity token, which is usually last, and therefore immune) so I could simply add autocomplete=OFF to all MY hidden fields and have not see it occur again.

I assume there is no "setting" in Rails (like with scaffold generators) that allows someone to override the behavior of whatever magic generates the "_method" hidden field? (If so, then a code change isn't strictly needed, just let devs know how to override the field generator for any hidden field)

So the implications for Rails apps, sans a fix, are simple: "Don't use Firefox, it's not our fault, but just don't"

@pixeltrix
Copy link
Contributor

I assume there is no "setting" in Rails (like with scaffold generators) that allows someone to override the behavior of whatever magic generates the "_method" hidden field? (If so, then a code change isn't strictly needed, just let devs know how to override the field generator for any hidden field)

The simplest option would be to monkey patch ActionView::Helpers::Tags::HiddenField#render like this:

module ActionView
  module Helpers
    module Tags
      class HiddenField < TextField
        def render
          @options[:autocomplete] = "off"
          super
        end
      end
    end
  end
end

Not tested but it'll be something like that should add autocomplete="off" to all your hidden fields constructed using the form.hidden_field method. For the lower level hidden_field_tag method you'll need to override it in a helper like this:

def hidden_field_tag(name, value = nil, options = {})
  super(name, value, options.merge(autocomplete: "off"))
end

So the implications for Rails apps, sans a fix, are simple: "Don't use Firefox, it's not our fault, but just don't"

Please avoid snarky comments like this - it's a 12 year old Firefox bug that they haven't felt bothered to fix so I don't see why Rails should be rushing out a patch release to work around it. I understand you're frustrated but the suggested fix would affect everyone else's apps so we need to fully understand how/why the issue occurs and any unintended consequences of the suggested fix - for example there's a comment about how some HTML validators would fail which could result in peoples CI breaking if they're using such tools to check output.

Do other frameworks output autocomplete=off by default on hidden fields? This doesn't seem to be Rails-specific bug so how have other frameworks 'fixed' this?

@zzak
Copy link
Member

zzak commented Jun 26, 2021

Not sure how relevant this was but after a brief search it seems Chrome doesn't even support autocomplete="off"?

@jpwynn
Copy link
Author

jpwynn commented Jun 27, 2021

autocomplete="off" is currently an official tag, and passes validator.w3.org (using an example with it added to a hidden field) for example the snippet below (with the setting added to all hidden fields) passes w3 validator.

Chrome may or may not act upon an autocomplete="off" setting but it is benign for the setting to be there (or not) in Chrome.

### simple html page with form with hidden fields passes w3 validator ###
<!DOCTYPE html>
<html class='h-full antialiased' lang='en'>
<head>
<title>
MYTITLE
</title>
<meta content='width=device-width, initial-scale=1' name='viewport'>
<meta content='none' name='robots'>
</head>
<body>

<form action="/posts/send_video_create" accept-charset="UTF-8" method="post">
<input type="hidden" name="authenticity_token" value="AAAAAAAAAA" />
<input type="hidden" name="firefoxidiot" id="firefoxidiots" value="dummystring" autocomplete="off" />
<input value="14" autocomplete="off" type="hidden" name="page_id" id="page_id" />
<input type="hidden" name="backupkey" id="backupkey" value="53117ebc9de4d08a3995" autocomplete="off" />
<input value="53117ebc9de4d08a3995" autocomplete="off" type="hidden" name="video_key" id="video_key" />
<input value="TOKEN" id="field_for_video_token" autocomplete="off" type="hidden" name="video_token" />
<input value="127.0.0.1" autocomplete="off" type="hidden" name="author_ip" id="author_ip" />
<input value="Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:89.0) Gecko/20100101 Firefox/89.0" autocomplete="off" type="hidden" name="author_ua" id="author_ua" />
</form>
</body>
</html>

This is a stripped down version of our real Rails web page, just for purposes of confirming the autocomplete on a hidden field doesn't affect validation.

While discovering and debugging, before adding autocomplete, we first tried adding that dummy field "firefoxidiot" first in the list to make it the "target" for Firefox's corruption, but were not 100% confident that would always be the field that Firefox corrupts. Then we discovered the mozilla thread(s) about adding autocomplete=off which solved the issue entirely.

And the don't use Firefox comment was not snark, it was entirely literal, and the pointy end of that stick was (obviously, I thought) waved in Firefox's direction not yours, for precisely the reasons you cite... a) their ignoring for over a decade a bug that provably corrupts form data, which b) is not Rails fault.

@domchristie
Copy link

FWIW I've also had authenticity_token hidden fields autocompleted as well as _method fields. Only tested locally.

@domchristie
Copy link

Not tested but it'll be something like that should add autocomplete="off" to all your hidden fields constructed using the form.hidden_field method. For the lower level hidden_field_tag method you'll need to override it in a helper

Unfortunately this won't work for framework-generated hidden fields such as _method

@ryanfb
Copy link
Contributor

ryanfb commented Jul 31, 2021

Rolling the suggestions above all together and adding some more, here's what's working for me to force autocomplete="off" onto most Rails-generated hidden input tags (as of Rails 6.1.4):
lib/core_extensions/action_view/helpers/date_time_selector.rb:

module CoreExtensions
  module ActionView
    module Helpers
      module DateTimeSelector
        private

        def build_hidden(type, value)
          select_options = {
            type: 'hidden',
            id: input_id_from_type(type),
            name: input_name_from_type(type),
            value: value,
            autocomplete: 'off'
          }.merge!(@html_options.slice(:disabled))
          select_options[:disabled] = 'disabled' if @options[:disabled]

          tag(:input, select_options) + "\n".html_safe
        end
      end
    end
  end
end

lib/core_extensions/action_view/helpers/form_tag_helper.rb:

module CoreExtensions
  module ActionView
    module Helpers
      module FormTagHelper
        def hidden_field_tag(name, value = nil, options = {})
          super(name, value, options.merge(autocomplete: 'off'))
        end
      end
    end
  end
end

lib/core_extensions/action_view/helpers/tags.rb:

module CoreExtensions
  module ActionView
    module Helpers
      module Tags
        class HiddenField
          def render
            @options[:autocomplete] = 'off'
            super
          end
        end
      end
    end
  end
end

lib/core_extensions/action_view/helpers/url_helper.rb:

module CoreExtensions
  module ActionView
    module Helpers
      module UrlHelper
        mattr_accessor :button_to_generates_button_tag, default: false

        def button_to(name = nil, options = nil, html_options = nil, &block)
          if block
            html_options = options
            options = name
          end
          options      ||= {}
          html_options ||= {}
          html_options = html_options.stringify_keys

          url    = options.is_a?(String) ? options : url_for(options)
          remote = html_options.delete('remote')
          params = html_options.delete('params')

          method     = html_options.delete('method').to_s
          method_tag = %w[patch put delete].include?(method) ? method_tag(method) : ''.html_safe

          form_method  = method == 'get' ? 'get' : 'post'
          form_options = html_options.delete('form') || {}
          form_options[:class] ||= html_options.delete('form_class') || 'button_to'
          form_options[:method] = form_method
          form_options[:action] = url
          form_options[:'data-remote'] = true if remote

          request_token_tag = if form_method == 'post'
                                request_method = method.empty? ? 'post' : method
                                token_tag(nil, form_options: { action: url, method: request_method })
                              else
                                ''
                              end

          html_options = convert_options_to_data_attributes(options, html_options)
          html_options['type'] = 'submit'

          button = if block || button_to_generates_button_tag
                     content_tag('button', name || url, html_options, &block)
                   else
                     html_options['value'] = name || url
                     tag('input', html_options)
                   end

          inner_tags = method_tag.safe_concat(button).safe_concat(request_token_tag)
          if params
            to_form_params(params).each do |param|
              inner_tags.safe_concat tag(:input, type: 'hidden', name: param[:name], value: param[:value],
                                                 autocomplete: 'off')
            end
          end
          content_tag('form', inner_tags, form_options)
        end

        private

        def token_tag(token = nil, form_options: {})
          if token != false && defined?(protect_against_forgery?) && protect_against_forgery?
            token ||= form_authenticity_token(form_options: form_options)
            tag(:input, type: 'hidden', name: request_forgery_protection_token.to_s, value: token, autocomplete: 'off')
          else
            ''
          end
        end

        def method_tag(method)
          tag('input', type: 'hidden', name: '_method', value: method.to_s, autocomplete: 'off')
        end
      end
    end
  end
end

config/initializers/hidden_field_autocomplete.rb:

Rails.application.reloader.to_prepare do
  ActionView::Helpers::DateTimeSelector.prepend CoreExtensions::ActionView::Helpers::DateTimeSelector
  ActionView::Helpers::FormTagHelper.prepend CoreExtensions::ActionView::Helpers::FormTagHelper
  ActionView::Helpers::Tags.prepend CoreExtensions::ActionView::Helpers::Tags
  ActionView::Helpers::UrlHelper.prepend CoreExtensions::ActionView::Helpers::UrlHelper
end

There's still a few stragglers even with this inelegant approach, where something could emit a hidden field without autocomplete="off". This could probably also probably be reworked into a PR.

@jwilsjustin
Copy link

This may help. I was also experiencing this in the name="_method" input generated by the button_to method. I am adding autocomplete="off" to the parent form element.

<%= button_to post_path(post), method: :delete, form: {autocomplete: "off"} do %>
  <i class="delete"></i>
  Delete
<% end %>

This is an easy addition without having to monkey patch.

@ryanfb
Copy link
Contributor

ryanfb commented Sep 19, 2021

I've just published the first version of a Rails plugin gem (rails-hidden_autocomplete) that bundles together my solution from the comment above for easy re-use in other Rails applications:

https://github.com/podqueue/rails-hidden_autocomplete

rafaelfranca added a commit that referenced this issue Sep 22, 2021
Add autocomplete="off" to all generated hidden fields (fixes #42610)
rafaelfranca added a commit that referenced this issue Sep 22, 2021
Add autocomplete="off" to all generated hidden fields (fixes #42610)
andyundso added a commit to simplificator/datatrans that referenced this issue Feb 21, 2022
andyundso added a commit to simplificator/datatrans that referenced this issue Feb 21, 2022
andyundso added a commit to simplificator/datatrans that referenced this issue Feb 21, 2022
gbp added a commit to mysociety/alaveteli that referenced this issue Mar 22, 2022
Change upstream in Rails, added `autocomplete` attributes to hidden form
inputs.

See: rails/rails#42610
gbp added a commit to mysociety/alaveteli that referenced this issue Mar 22, 2022
Change upstream in Rails, added `autocomplete` attributes to hidden form
inputs.

See: rails/rails#42610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants