FEATURE: improve honeypot and challenge logic

This feature amends it so instead of using one challenge and honeypot statically per site we have a rotating honeypot and challenge value which changes every hour. This means you must grab a fresh copy of honeypot and challenge value once an hour or account registration will be rejected. We also now cycle the value of the challenge when after successful account registration forcing an extra call to hp.json between account registrations Client has been made aware of these changes. Additionally this contains a JavaScript workaround for: https://bugs.chromium.org/p/chromium/issues/detail?id=987293 This is client side code that is specific to Chrome user agent and swaps a PASSWORD type honeypot with a TEXT type honeypot.
2024-11-22 11:15:05 +08:00 · 2019-10-16 16:53:31 +11:00 · 2019-10-16 16:53:31 +11:00 · d5d8db7fa8
commit d5d8db7fa8
parent 645faa847b
5 changed files with 198 additions and 98 deletions
--- a/app/assets/javascripts/discourse/controllers/create-account.js.es6
+++ b/app/assets/javascripts/discourse/controllers/create-account.js.es6
@ -24,7 +24,6 @@ export default Ember.Controller.extend(
    login: Ember.inject.controller(),

    complete: false,
-    accountPasswordConfirm: 0,
    accountChallenge: 0,
    formSubmitted: false,
    rejectedEmails: Ember.A([]),
@ -191,8 +190,36 @@ export default Ember.Controller.extend(
    @on("init")
    fetchConfirmationValue() {
      return ajax(userPath("hp.json")).then(json => {
+        this._challengeDate = new Date();
+        // remove 30 seconds for jitter, make sure this works for at least
+        // 30 seconds so we don't have hard loops
+        this._challengeExpiry = parseInt(json.expires_in, 10) - 30;
+        if (this._challengeExpiry < 30) {
+          this._challengeExpiry = 30;
+        }
+
+        const confirmation = document.getElementById(
+          "new-account-confirmation"
+        );
+        if (confirmation) {
+          confirmation.value = json.value;
+        }
+
+        // Chrome autocomplete is buggy per:
+        // https://bugs.chromium.org/p/chromium/issues/detail?id=987293
+        // work around issue while leaving a semi useable honeypot for
+        // bots that are running full Chrome
+        if (confirmation && navigator.userAgent.indexOf("Chrome") > 0) {
+          const newConfirmation = document.createElement("input");
+
+          newConfirmation.type = "text";
+          newConfirmation.id = "new-account-confirmation";
+          newConfirmation.value = json.value;
+
+          confirmation.parentNode.replaceChild(newConfirmation, confirmation);
+        }
+
        this.setProperties({
-          accountPasswordConfirm: json.value,
          accountChallenge: json.challenge
            .split("")
            .reverse()
@ -201,85 +228,102 @@ export default Ember.Controller.extend(
      });
    },

+    performAccountCreation() {
+      const attrs = this.getProperties(
+        "accountName",
+        "accountEmail",
+        "accountPassword",
+        "accountUsername",
+        "accountChallenge"
+      );
+
+      attrs["accountPasswordConfirm"] = document.getElementById(
+        "new-account-confirmation"
+      ).value;
+
+      const userFields = this.userFields;
+      const destinationUrl = this.get("authOptions.destination_url");
+
+      if (!Ember.isEmpty(destinationUrl)) {
+        $.cookie("destination_url", destinationUrl, { path: "/" });
+      }
+
+      // Add the userfields to the data
+      if (!Ember.isEmpty(userFields)) {
+        attrs.userFields = {};
+        userFields.forEach(
+          f => (attrs.userFields[f.get("field.id")] = f.get("value"))
+        );
+      }
+
+      this.set("formSubmitted", true);
+      return Discourse.User.createAccount(attrs).then(
+        result => {
+          this.set("isDeveloper", false);
+          if (result.success) {
+            // invalidate honeypot
+            this._challengeExpiry = 1;
+
+            // Trigger the browser's password manager using the hidden static login form:
+            const $hidden_login_form = $("#hidden-login-form");
+            $hidden_login_form
+              .find("input[name=username]")
+              .val(attrs.accountUsername);
+            $hidden_login_form
+              .find("input[name=password]")
+              .val(attrs.accountPassword);
+            $hidden_login_form
+              .find("input[name=redirect]")
+              .val(userPath("account-created"));
+            $hidden_login_form.submit();
+          } else {
+            this.flash(
+              result.message || I18n.t("create_account.failed"),
+              "error"
+            );
+            if (result.is_developer) {
+              this.set("isDeveloper", true);
+            }
+            if (
+              result.errors &&
+              result.errors.email &&
+              result.errors.email.length > 0 &&
+              result.values
+            ) {
+              this.rejectedEmails.pushObject(result.values.email);
+            }
+            if (
+              result.errors &&
+              result.errors.password &&
+              result.errors.password.length > 0
+            ) {
+              this.rejectedPasswords.pushObject(attrs.accountPassword);
+            }
+            this.set("formSubmitted", false);
+            $.removeCookie("destination_url");
+          }
+        },
+        () => {
+          this.set("formSubmitted", false);
+          $.removeCookie("destination_url");
+          return this.flash(I18n.t("create_account.failed"), "error");
+        }
+      );
+    },
+
    actions: {
      externalLogin(provider) {
        this.login.send("externalLogin", provider);
      },

      createAccount() {
-        const attrs = this.getProperties(
-          "accountName",
-          "accountEmail",
-          "accountPassword",
-          "accountUsername",
-          "accountPasswordConfirm",
-          "accountChallenge"
-        );
-        const userFields = this.userFields;
-        const destinationUrl = this.get("authOptions.destination_url");
-
-        if (!Ember.isEmpty(destinationUrl)) {
-          $.cookie("destination_url", destinationUrl, { path: "/" });
-        }
-
-        // Add the userfields to the data
-        if (!Ember.isEmpty(userFields)) {
-          attrs.userFields = {};
-          userFields.forEach(
-            f => (attrs.userFields[f.get("field.id")] = f.get("value"))
+        if (new Date() - this._challengeDate > 1000 * this._challengeExpiry) {
+          this.fetchConfirmationValue().then(() =>
+            this.performAccountCreation()
          );
+        } else {
+          this.performAccountCreation();
        }
-
-        this.set("formSubmitted", true);
-        return Discourse.User.createAccount(attrs).then(
-          result => {
-            this.set("isDeveloper", false);
-            if (result.success) {
-              // Trigger the browser's password manager using the hidden static login form:
-              const $hidden_login_form = $("#hidden-login-form");
-              $hidden_login_form
-                .find("input[name=username]")
-                .val(attrs.accountUsername);
-              $hidden_login_form
-                .find("input[name=password]")
-                .val(attrs.accountPassword);
-              $hidden_login_form
-                .find("input[name=redirect]")
-                .val(userPath("account-created"));
-              $hidden_login_form.submit();
-            } else {
-              this.flash(
-                result.message || I18n.t("create_account.failed"),
-                "error"
-              );
-              if (result.is_developer) {
-                this.set("isDeveloper", true);
-              }
-              if (
-                result.errors &&
-                result.errors.email &&
-                result.errors.email.length > 0 &&
-                result.values
-              ) {
-                this.rejectedEmails.pushObject(result.values.email);
-              }
-              if (
-                result.errors &&
-                result.errors.password &&
-                result.errors.password.length > 0
-              ) {
-                this.rejectedPasswords.pushObject(attrs.accountPassword);
-              }
-              this.set("formSubmitted", false);
-              $.removeCookie("destination_url");
-            }
-          },
-          () => {
-            this.set("formSubmitted", false);
-            $.removeCookie("destination_url");
-            return this.flash(I18n.t("create_account.failed"), "error");
-          }
-        );
      }
    }
  }
--- a/app/assets/javascripts/discourse/templates/modal/create-account.hbs
+++ b/app/assets/javascripts/discourse/templates/modal/create-account.hbs
@ -83,7 +83,7 @@
          <tr class="password-confirmation">
            <td><label for='new-account-password-confirmation'>{{i18n 'user.password_confirmation.title'}}</label></td>
            <td>
-              {{input type="password" value=accountPasswordConfirm id="new-account-confirmation" autocomplete="new-password"}}
+              <input autocomplete="new-password" id="new-account-confirmation" type="password">
              {{input value=accountChallenge id="new-account-challenge"}}
            </td>
          </tr>
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@ -414,6 +414,9 @@ class UsersController < ApplicationController
      authentication.finish
      activation.finish

+      secure_session[HONEYPOT_KEY] = nil
+      secure_session[CHALLENGE_KEY] = nil
+
      # save user email in session, to show on account-created page
      session["user_created_message"] = activation.message
      session[SessionController::ACTIVATE_USER_KEY] = user.id
@ -467,7 +470,14 @@ class UsersController < ApplicationController
  end

  def get_honeypot_value
-    render json: { value: honeypot_value, challenge: challenge_value }
+    secure_session.set(HONEYPOT_KEY, honeypot_value, expires: 1.hour)
+    secure_session.set(CHALLENGE_KEY, challenge_value, expires: 1.hour)
+
+    render json: {
+      value: honeypot_value,
+      challenge: challenge_value,
+      expires_in: SecureSession.expiry
+    }
  end

  def password_reset
@ -660,7 +670,6 @@ class UsersController < ApplicationController
        security_keys_enabled = email_token_user&.security_keys_enabled?
        second_factor_token = params[:second_factor_token]
        second_factor_method = params[:second_factor_method].to_i
-        security_key_credential = params[:security_key_credential]
        confirm_email = false
        @security_key_required = security_keys_enabled

@ -1368,22 +1377,21 @@ class UsersController < ApplicationController
    render json: success_json
  end

-  private
+  HONEYPOT_KEY ||= 'HONEYPOT_KEY'
+  CHALLENGE_KEY ||= 'CHALLENGE_KEY'
+
+  protected

  def honeypot_value
-    Digest::SHA1::hexdigest("#{Discourse.current_hostname}:#{GlobalSetting.safe_secret_key_base}")[0, 15]
+    secure_session[HONEYPOT_KEY] ||= SecureRandom.hex
  end

  def challenge_value
-    challenge = $redis.get('SECRET_CHALLENGE')
-    unless challenge && challenge.length == 16 * 2
-      challenge = SecureRandom.hex(16)
-      $redis.set('SECRET_CHALLENGE', challenge)
-    end
-
-    challenge
+    secure_session[CHALLENGE_KEY] ||= SecureRandom.hex
  end

+  private
+
  def respond_to_suspicious_request
    if suspicious?(params)
      render json: {
--- a/lib/secure_session.rb
+++ b/lib/secure_session.rb
@ -6,15 +6,36 @@ class SecureSession
    @prefix = prefix
  end

+  def self.expiry
+    @expiry ||= 1.hour.to_i
+  end
+
+  def self.expiry=(val)
+    @expiry = val
+  end
+
+  def set(key, val, expires: nil)
+    expires ||= SecureSession.expiry
+    $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s)
+    true
+  end
+
  def [](key)
-    $redis.get("#{@prefix}#{key}")
+    $redis.get(prefixed_key(key))
  end

  def []=(key, val)
    if val == nil
-      $redis.del("#{@prefix}#{key}")
+      $redis.del(prefixed_key(key))
    else
-      $redis.setex("#{@prefix}#{key}", 1.hour, val.to_s)
+      $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s)
    end
+    val
+  end
+
+  private
+
+  def prefixed_key(key)
+    "#{@prefix}#{key}"
  end
 end
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@ -5,6 +5,41 @@ require 'rails_helper'
 describe UsersController do
  let(:user) { Fabricate(:user) }

+  describe "#full account registration flow" do
+    it "will correctly handle honeypot and challenge" do
+
+      get '/u/hp.json'
+      expect(response.status).to eq(200)
+
+      json = JSON.parse(response.body)
+
+      params = {
+        email: 'jane@jane.com',
+        name: 'jane',
+        username: 'jane',
+        password_confirmation: json['value'],
+        challenge: json['challenge'].reverse,
+        password: SecureRandom.hex
+      }
+
+      secure_session = SecureSession.new(session["secure_session_id"])
+
+      expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(json["value"])
+      expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(json["challenge"])
+
+      post '/u.json', params: params
+
+      expect(response.status).to eq(200)
+
+      jane = User.find_by(username: 'jane')
+
+      expect(jane.email).to eq('jane@jane.com')
+
+      expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(nil)
+      expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(nil)
+    end
+  end
+
  describe '#perform_account_activation' do
    let(:token) do
      return @token if @token.present?
@ -1020,22 +1055,15 @@ describe UsersController do

    shared_examples 'honeypot fails' do
      it 'should not create a new user' do
+        User.any_instance.expects(:enqueue_welcome_message).never
+
        expect {
          post "/u.json", params: create_params
        }.to_not change { User.count }
-        expect(response.status).to eq(200)
-      end

-      it 'should not send an email' do
-        User.any_instance.expects(:enqueue_welcome_message).never
-        post "/u.json", params: create_params
        expect(response.status).to eq(200)
-      end

-      it 'should say it was successful' do
-        post "/u.json", params: create_params
        json = JSON::parse(response.body)
-        expect(response.status).to eq(200)
        expect(json["success"]).to eq(true)

        # should not change the session
@ -3361,7 +3389,6 @@ describe UsersController do
        end

        it 'succeeds on correct password' do
-          session = {}
          ApplicationController.any_instance.stubs(:secure_session).returns("confirmed-password-#{user.id}" => "true")
          post "/users/create_second_factor_totp.json"