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 reCAPTCHA support and update captcha provider handling #647

Merged
merged 10 commits into from
Dec 18, 2024
3 changes: 3 additions & 0 deletions api/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ STRIPE_TEST_DEFAULT_PRICING_YEARLY=
H_CAPTCHA_SITE_KEY=
H_CAPTCHA_SECRET_KEY=

RE_CAPTCHA_SITE_KEY=
RE_CAPTCHA_SECRET_KEY=
Comment on lines +75 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Documentation needed for reCAPTCHA environment variables

The new reCAPTCHA environment variables are not documented in the environment variables configuration file (docs/configuration/environment-variables.mdx). Please add the following to the Configuration Environment Variables table:

  • RE_CAPTCHA_SITE_KEY: Site key for reCAPTCHA integration
  • RE_CAPTCHA_SECRET_KEY: Secret key for reCAPTCHA integration
🔗 Analysis chain

Verify documentation for new reCAPTCHA configuration

The environment variables are correctly added. Please ensure these new variables are documented in:

  1. Installation guide
  2. Configuration documentation
  3. Deployment checklist
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if documentation exists for new variables
fd -e md -e mdx | xargs rg -l "RE_CAPTCHA_(SITE|SECRET)_KEY"

Length of output: 62


Script:

#!/bin/bash
# Let's first check if there are any markdown files in docs or similar directories
fd -e md -e mdx

# Also check for README files that might contain documentation
fd README

Length of output: 907


Script:

#!/bin/bash
# Let's check the environment variables documentation and installation guides
cat docs/configuration/environment-variables.mdx
cat docs/deployment/local-deployment.mdx

Length of output: 5373


MUX_WORKSPACE_ID=
MUX_API_TOKEN=

Expand Down
7 changes: 6 additions & 1 deletion api/app/Http/Requests/AnswerFormRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use App\Rules\StorageFile;
use App\Rules\ValidHCaptcha;
use App\Rules\ValidPhoneInputRule;
use App\Rules\ValidReCaptcha;
use App\Rules\ValidUrl;
use App\Service\Forms\FormLogicPropertyResolver;
use Illuminate\Foundation\Http\FormRequest;
Expand Down Expand Up @@ -118,7 +119,11 @@ public function rules()

// Validate hCaptcha
if ($this->form->use_captcha) {
$this->requestRules['h-captcha-response'] = [new ValidHCaptcha()];
if ($this->form->captcha_provider === 'recaptcha') {
$this->requestRules['g-recaptcha-response'] = [new ValidReCaptcha()];
} elseif ($this->form->captcha_provider === 'hcaptcha') {
$this->requestRules['h-captcha-response'] = [new ValidHCaptcha()];
}
Comment on lines +122 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add default case handling for captcha validation.

While the current implementation handles both reCAPTCHA and hCaptcha, it lacks handling for unexpected captcha_provider values. This could lead to a security issue where no captcha validation is applied even when use_captcha is true.

Consider adding a default case:

 if ($this->form->use_captcha) {
     if ($this->form->captcha_provider === 'recaptcha') {
         $this->requestRules['g-recaptcha-response'] = [new ValidReCaptcha()];
     } elseif ($this->form->captcha_provider === 'hcaptcha') {
         $this->requestRules['h-captcha-response'] = [new ValidHCaptcha()];
+    } else {
+        // Default to hCaptcha if provider is invalid
+        $this->requestRules['h-captcha-response'] = [new ValidHCaptcha()];
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

}

// Validate submission_id for edit mode
Expand Down
1 change: 1 addition & 0 deletions api/app/Http/Requests/UserFormRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ public function rules()
'can_be_indexed' => 'boolean',
'password' => 'sometimes|nullable',
'use_captcha' => 'boolean',
'captcha_provider' => ['sometimes', Rule::in(['recaptcha', 'hcaptcha'])],

// Custom SEO
'seo_meta' => 'nullable|array',
Expand Down
1 change: 1 addition & 0 deletions api/app/Models/Forms/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class Form extends Model implements CachableAttributes
'submitted_text',
'redirect_url',
'use_captcha',
'captcha_provider',
'closes_at',
'closed_text',
'max_submissions_count',
Expand Down
51 changes: 51 additions & 0 deletions api/app/Rules/ValidReCaptcha.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

namespace App\Rules;

use Closure;
use Illuminate\Contracts\Validation\ImplicitRule;
use Illuminate\Support\Facades\Http;

class ValidReCaptcha implements ImplicitRule
{
public const RECAPTCHA_VERIFY_URL = 'https://www.google.com/recaptcha/api/siteverify';

private $error = 'Invalid CAPTCHA. Please prove you\'re not a bot.';

/**
* Determine if the validation rule passes.
*
* @param string $attribute
* @param mixed $value
* @return bool
*/
public function passes($attribute, $value)
{
if (empty($value)) {
$this->error = 'Please complete the captcha.';

return false;
}

return Http::asForm()->post(self::RECAPTCHA_VERIFY_URL, [
'secret' => config('services.re_captcha.secret_key'),
'response' => $value,
])->json('success');
}
Comment on lines +30 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance reCAPTCHA verification security and reliability

The verification request should include additional security measures and error handling:

  1. Include the user's IP address in the verification request (recommended by Google)
  2. Add timeout and error handling for the HTTP request
     return Http::asForm()->post(self::RECAPTCHA_VERIFY_URL, [
         'secret' => config('services.re_captcha.secret_key'),
         'response' => $value,
+        'remoteip' => request()->ip(),
-    ])->json('success');
+    ])
+    ->timeout(5)
+    ->throw(function($response, $e) {
+        $this->error = 'CAPTCHA verification failed. Please try again.';
+        return false;
+    })
+    ->json('success');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return Http::asForm()->post(self::RECAPTCHA_VERIFY_URL, [
'secret' => config('services.re_captcha.secret_key'),
'response' => $value,
])->json('success');
}
return Http::asForm()->post(self::RECAPTCHA_VERIFY_URL, [
'secret' => config('services.re_captcha.secret_key'),
'response' => $value,
'remoteip' => request()->ip(),
])
->timeout(5)
->throw(function($response, $e) {
$this->error = 'CAPTCHA verification failed. Please try again.';
return false;
})
->json('success');
}

public function validate(string $attribute, mixed $value, Closure $fail): void
{
if (!$this->passes($attribute, $value)) {
$fail($this->message());
}
}

/**
* Get the validation error message.
*
* @return string
*/
public function message()
{
return $this->error;
}
}
5 changes: 5 additions & 0 deletions api/config/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@
'secret_key' => env('H_CAPTCHA_SECRET_KEY'),
],

're_captcha' => [
'site_key' => env('RE_CAPTCHA_SITE_KEY'),
'secret_key' => env('RE_CAPTCHA_SECRET_KEY'),
],

'canny' => [
'api_key' => env('CANNY_API_KEY'),
],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class () extends Migration {
public function up()
{
Schema::table('forms', function (Blueprint $table) {
$table->string('captcha_provider')->default('hcaptcha')->after('use_captcha');
});
}

public function down()
{
Schema::table('forms', function (Blueprint $table) {
$table->dropColumn('captcha_provider');
});
}
};
23 changes: 22 additions & 1 deletion api/tests/Feature/Forms/FormCaptchaTest.php
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
<?php

it('create form with captcha and raise validation issue', function () {
it('create form with hcaptcha and raise validation issue', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace, [
'use_captcha' => true,
'captcha_provider' => 'hcaptcha',
]);

$this->postJson(route('forms.answer', $form->slug), [])
Expand All @@ -18,3 +19,23 @@
],
]);
});

it('create form with recaptcha and raise validation issue', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace, [
'use_captcha' => true,
'captcha_provider' => 'recaptcha',
]);

$this->postJson(route('forms.answer', $form->slug), [])
->assertStatus(422)
->assertJson([
'message' => 'Please complete the captcha.',
'errors' => [
'g-recaptcha-response' => [
'Please complete the captcha.',
],
],
]);
});
1 change: 1 addition & 0 deletions client/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ NUXT_PUBLIC_CRISP_WEBSITE_ID=
NUXT_PUBLIC_ENV=
NUXT_PUBLIC_GOOGLE_ANALYTICS_CODE=
NUXT_PUBLIC_H_CAPTCHA_SITE_KEY=
NUXT_PUBLIC_RE_CAPTCHA_SITE_KEY=
NUXT_API_SECRET=secret
76 changes: 76 additions & 0 deletions client/components/forms/RecaptchaV2.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<template>
<div>
<div
ref="recaptchaContainer"
class="g-recaptcha"
:data-sitekey="sitekey"
:data-theme="theme"
/>
</div>
</template>

<script setup>
const props = defineProps({
sitekey: {
type: String,
required: true
},
theme: {
type: String,
default: 'light'
}
})

const emit = defineEmits(['verify', 'expired'])
const recaptchaContainer = ref(null)

onMounted(async () => {
if (!document.querySelector('script[src*="recaptcha/api.js"]')) {
const script = document.createElement('script')
script.src = 'https://www.google.com/recaptcha/api.js'
script.async = true
script.defer = true
document.head.appendChild(script)

await new Promise((resolve) => {
script.onload = resolve
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using a more secure script loading approach

The current implementation directly creates and appends a script tag. Consider using CSP-compliant script loading or a dedicated script loader utility.

-    const script = document.createElement('script')
-    script.src = 'https://www.google.com/recaptcha/api.js'
-    script.async = true
-    script.defer = true
-    document.head.appendChild(script)
+    await useScriptLoader('https://www.google.com/recaptcha/api.js', {
+      async: true,
+      defer: true,
+      nonce: YOUR_CSP_NONCE // Add nonce for CSP compliance
+    })

Committable suggestion skipped: line range outside the PR's diff.


// Wait for grecaptcha to be available
await new Promise((resolve) => {
const checkGrecaptcha = () => {
if (window.grecaptcha?.ready) {
resolve()
} else {
setTimeout(checkGrecaptcha, 100)
}
}
checkGrecaptcha()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add timeout to prevent infinite polling

The grecaptcha availability check could potentially run indefinitely. Add a timeout mechanism.

   await new Promise((resolve) => {
+    const MAX_RETRIES = 50; // 5 seconds
+    let retries = 0;
     const checkGrecaptcha = () => {
       if (window.grecaptcha?.ready) {
         resolve()
+      } else if (retries >= MAX_RETRIES) {
+        console.error('reCAPTCHA failed to load')
+        resolve()
       } else {
+        retries++
         setTimeout(checkGrecaptcha, 100)
       }
     }
     checkGrecaptcha()
   })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await new Promise((resolve) => {
const checkGrecaptcha = () => {
if (window.grecaptcha?.ready) {
resolve()
} else {
setTimeout(checkGrecaptcha, 100)
}
}
checkGrecaptcha()
})
await new Promise((resolve) => {
const MAX_RETRIES = 50; // 5 seconds
let retries = 0;
const checkGrecaptcha = () => {
if (window.grecaptcha?.ready) {
resolve()
} else if (retries >= MAX_RETRIES) {
console.error('reCAPTCHA failed to load')
resolve()
} else {
retries++
setTimeout(checkGrecaptcha, 100)
}
}
checkGrecaptcha()
})


window.recaptchaCallback = (token) => {
emit('verify', token)
}

window.recaptchaExpiredCallback = () => {
emit('expired')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using global window functions for callbacks

Using global window functions can lead to naming conflicts and security issues. Consider using a more encapsulated approach.

-  window.recaptchaCallback = (token) => {
-    emit('verify', token)
-  }
-
-  window.recaptchaExpiredCallback = () => {
-    emit('expired')
-  }
+  const callbacks = {
+    verify: (token) => emit('verify', token),
+    expired: () => emit('expired')
+  }
+
+  // Use these callbacks directly in grecaptcha.render

Committable suggestion skipped: line range outside the PR's diff.


try {
window.grecaptcha.render(recaptchaContainer.value, {
sitekey: props.sitekey,
theme: props.theme,
callback: 'recaptchaCallback',
'expired-callback': 'recaptchaExpiredCallback'
})
} catch (error) {
console.error('Error rendering reCAPTCHA:', error)
}
})

onBeforeUnmount(() => {
delete window.recaptchaCallback
delete window.recaptchaExpiredCallback
})
</script>
54 changes: 41 additions & 13 deletions client/components/open/forms/OpenForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,31 @@
<!-- Captcha -->
<template v-if="form.use_captcha && isLastPage">
<div class="mb-3 px-2 mt-2 mx-auto w-max">
<vue-hcaptcha
ref="hcaptcha"
:sitekey="hCaptchaSiteKey"
:theme="darkMode?'dark':'light'"
@opened="setMinHeight(500)"
@closed="setMinHeight(0)"
/>
<has-error
:form="dataForm"
field-id="h-captcha-response"
/>
<template v-if="form.captcha_provider === 'recaptcha'">
<RecaptchaV2
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Vue refs instead of direct DOM access for captcha responses

Accessing DOM elements directly with document.getElementsByName() is not recommended in Vue.js, as it can lead to potential runtime errors and goes against Vue's reactivity principles. Using Vue refs ensures better integration with the Vue framework and improves code reliability.

Apply the following changes:

In the template (line 82~):

Add a ref attribute to the RecaptchaV2 component:

<RecaptchaV2
+ ref="recaptcha"
  :sitekey="recaptchaSiteKey"
  :theme="darkMode ? 'dark' : 'light'"
  @verify="onRecaptchaVerify"
  @expired="onRecaptchaExpired"
/>

In the script (lines 389~ to 395~):

Update the submitForm method to use refs:

if (this.form.captcha_provider === 'recaptcha') {
-  this.dataForm['g-recaptcha-response'] = document.getElementsByName('g-recaptcha-response')[0]?.value
-  window.grecaptcha?.reset()
+  this.dataForm['g-recaptcha-response'] = this.$refs.recaptcha.getResponse()
+  this.$refs.recaptcha.reset()
} else if (this.form.captcha_provider === 'hcaptcha') {
-  this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value
+  this.dataForm['h-captcha-response'] = this.$refs.hcaptcha.getResponse()
  this.$refs.hcaptcha.reset()
}

Note: Ensure that you handle cases where the refs might not exist to prevent runtime errors. For example:

if (this.form.captcha_provider === 'recaptcha' && this.$refs.recaptcha) {
  // ...
}

Also applies to: 389-395

:sitekey="recaptchaSiteKey"
:theme="darkMode ? 'dark' : 'light'"
@verify="onRecaptchaVerify"
@expired="onRecaptchaExpired"
/>
<has-error
:form="dataForm"
field-id="g-recaptcha-response"
/>
</template>
<template v-if="form.captcha_provider === 'hcaptcha'">
<vue-hcaptcha
ref="hcaptcha"
:sitekey="hCaptchaSiteKey"
:theme="darkMode?'dark':'light'"
@opened="setMinHeight(500)"
@closed="setMinHeight(0)"
/>
<has-error
:form="dataForm"
field-id="h-captcha-response"
/>
</template>
</div>
</template>

Expand Down Expand Up @@ -214,6 +228,9 @@ export default {
hCaptchaSiteKey() {
return useRuntimeConfig().public.hCaptchaSiteKey
},
recaptchaSiteKey() {
return useRuntimeConfig().public.recaptchaSiteKey
},
/**
* Create field groups (or Page) using page breaks if any
*/
Expand Down Expand Up @@ -369,8 +386,13 @@ export default {
if (!this.isAutoSubmit && this.formPageIndex !== this.fieldGroups.length - 1) return

if (this.form.use_captcha && import.meta.client) {
this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value
this.$refs.hcaptcha.reset()
if (this.form.captcha_provider === 'recaptcha') {
this.dataForm['g-recaptcha-response'] = document.getElementsByName('g-recaptcha-response')[0]?.value
window.grecaptcha?.reset()
} else if (this.form.captcha_provider === 'hcaptcha') {
this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value
this.$refs.hcaptcha.reset()
}
}

if (this.form.editable_submissions && this.form.submission_id) {
Expand Down Expand Up @@ -593,6 +615,12 @@ export default {
} catch (e) {
console.error(e)
}
},
onRecaptchaVerify(token) {
this.dataForm['g-recaptcha-response'] = token
},
onRecaptchaExpired() {
this.dataForm['g-recaptcha-response'] = null
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,23 @@
label="Bot Protection"
help="Protects your form from spam and abuse with a captcha"
/>
<FlatSelectInput
v-if="form.use_captcha"
name="captcha_provider"
:form="form"
:options="captchaOptions"
class="mt-4"
label="Select a captcha provider"
/>
</SettingsSection>
</template>

<script>
import { useWorkingFormStore } from '../../../../../stores/working_form'
<script setup>
const workingFormStore = useWorkingFormStore()
const { content: form } = storeToRefs(workingFormStore)

export default {
components: { },
props: {},
setup () {
const workingFormStore = useWorkingFormStore()
return {
workingFormStore,
form: storeToRefs(workingFormStore).content,
crisp: useCrisp()
}
}
}
const captchaOptions = [
{ name: 'reCAPTCHA', value: 'recaptcha' },
{ name: 'hCaptcha', value: 'hcaptcha' },
]
</script>
1 change: 1 addition & 0 deletions client/runtimeConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export default {
appUrl: process.env.NUXT_PUBLIC_APP_URL || '',
env: process.env.NUXT_PUBLIC_ENV || 'local',
hCaptchaSiteKey: process.env.NUXT_PUBLIC_H_CAPTCHA_SITE_KEY || null,
recaptchaSiteKey: process.env.NUXT_PUBLIC_RE_CAPTCHA_SITE_KEY || null,
gtmCode: process.env.NUXT_PUBLIC_GTM_CODE || null,
amplitudeCode: process.env.NUXT_PUBLIC_AMPLITUDE_CODE || null,
crispWebsiteId: process.env.NUXT_PUBLIC_CRISP_WEBSITE_ID || null,
Expand Down
Loading