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

variables and mentions #590

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function ($attribute, $value, $fail) {
}
},
],
'confirmation_reply_to' => 'email|nullable',
'confirmation_reply_to' => 'nullable',
'notification_sender' => 'required',
'notification_subject' => 'required',
'notification_body' => 'required',
Expand Down Expand Up @@ -107,6 +107,7 @@ public static function validateEmail($email): bool
public static function formatData(array $data): array
{
return array_merge(parent::formatData($data), [
'notification_subject' => Purify::clean($data['notification_subject'] ?? ''),
'notification_body' => Purify::clean($data['notification_body'] ?? ''),
]);
}
Expand Down
37 changes: 29 additions & 8 deletions api/app/Mail/Forms/SubmissionConfirmationMail.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use App\Events\Forms\FormSubmitted;
use App\Mail\OpenFormMail;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
Expand All @@ -16,13 +17,21 @@ class SubmissionConfirmationMail extends OpenFormMail implements ShouldQueue
use Queueable;
use SerializesModels;

private $formattedData;

/**
* Create a new message instance.
*
* @return void
*/
public function __construct(private FormSubmitted $event, private $integrationData)
{
$formatter = (new FormSubmissionFormatter($event->form, $event->data))
->createLinks()
->outputStringsOnly()
->useSignedUrlForFiles();

$this->formattedData = $formatter->getFieldsWithValue();
Comment on lines +29 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.

⚠️ Potential issue

Handle Potential Exceptions from FormSubmissionFormatter

While constructing the FormSubmissionFormatter, exceptions could be thrown if the formatting fails. Consider adding exception handling to manage potential errors gracefully and provide fallback mechanisms if necessary.

Example:

try {
    $formatter = (new FormSubmissionFormatter($event->form, $event->data))
        ->createLinks()
        ->outputStringsOnly()
        ->useSignedUrlForFiles();

    $this->formattedData = $formatter->getFieldsWithValue();
} catch (\Exception $e) {
    // Handle the exception, log it, or set a default value
    // For example:
    $this->formattedData = [];
    \Log::error('FormSubmissionFormatter failed: ' . $e->getMessage());
}

}

/**
Expand All @@ -34,17 +43,13 @@ public function build()
{
$form = $this->event->form;

$formatter = (new FormSubmissionFormatter($form, $this->event->data))
->createLinks()
->outputStringsOnly()
->useSignedUrlForFiles();

return $this
->replyTo($this->getReplyToEmail($form->creator->email))
->from($this->getFromEmail(), $this->integrationData->notification_sender)
->subject($this->integrationData->notification_subject)
->subject($this->getSubject())
->markdown('mail.form.confirmation-submission-notification', [
'fields' => $formatter->getFieldsWithValue(),
'fields' => $this->formattedData,
'form' => $form,
'integrationData' => $this->integrationData,
'noBranding' => $form->no_branding,
Expand All @@ -67,9 +72,25 @@ private function getReplyToEmail($default)
{
$replyTo = $this->integrationData->confirmation_reply_to ?? null;

if ($replyTo && filter_var($replyTo, FILTER_VALIDATE_EMAIL)) {
return $replyTo;
if ($replyTo) {
$parser = new MentionParser($replyTo, $this->formattedData);
$parsedReplyTo = $parser->parse();
if ($parsedReplyTo && filter_var($parsedReplyTo, FILTER_VALIDATE_EMAIL)) {
return $parsedReplyTo;
}
Comment on lines +76 to +81
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 Refactoring Repetitive Parsing Logic

The parsing logic using MentionParser in getReplyToEmail(), getSubject(), and getNotificationBody() is repetitive. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring this code into a single private method.

Here's how you might refactor the parsing logic:

private function parseIntegrationDataField($field)
{
    $value = $this->integrationData->$field ?? null;
    if ($value) {
        $parser = new MentionParser($value, $this->formattedData);
        return $parser->parse();
    }
    return null;
}

Update the methods accordingly:

private function getReplyToEmail($default)
{
    $parsedReplyTo = $this->parseIntegrationDataField('confirmation_reply_to');

    if ($parsedReplyTo && filter_var($parsedReplyTo, FILTER_VALIDATE_EMAIL)) {
        return $parsedReplyTo;
    }
    return $default;
}

private function getSubject()
{
-   $parser = new MentionParser($this->integrationData->notification_subject, $this->formattedData);
-   return $parser->parse();
+   return $this->parseIntegrationDataField('notification_subject') ?? '';
}

private function getNotificationBody()
{
-   $parser = new MentionParser($this->integrationData->notification_body, $this->formattedData);
-   return $parser->parse();
+   return $this->parseIntegrationDataField('notification_body') ?? '';
}

}
return $default;
}

private function getSubject()
{
$parser = new MentionParser($this->integrationData->notification_subject, $this->formattedData);
return $parser->parse();
}

private function getNotificationBody()
{
$parser = new MentionParser($this->integrationData->notification_body, $this->formattedData);
return $parser->parse();
}
Comment on lines +92 to +96
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

Review the Usage of getNotificationBody()

The getNotificationBody() method is defined but not used within this class. Unused methods can introduce unnecessary complexity. Consider implementing it where appropriate or removing it if it's not needed.

}
55 changes: 55 additions & 0 deletions api/app/Open/MentionParser.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php

namespace App\Open;

class MentionParser
{
private $content;
private $data;

public function __construct($content, $data)
{
$this->content = $content;
$this->data = $data;
}
Comment on lines +13 to +17
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 type hints and return types for improved code clarity

Adding parameter and return type hints enhances readability and helps catch type-related bugs. Consider updating your methods with type declarations:

-public function __construct($content, $data)
+public function __construct(string $content, array $data)

-public function parse()
+public function parse(): string

-private function replaceMentions()
+private function replaceMentions(): string

-private function getData($fieldId)
+private function getData(string $fieldId)

Also applies to: 16-20, 21-41, 43-54


public function parse()
{
return $this->replaceMentions();
}

private function replaceMentions()
{
$pattern = '/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/';
return preg_replace_callback($pattern, function ($matches) {
$fieldId = $matches[1];
$fallback = $matches[2];
$value = $this->getData($fieldId);

if ($value !== null) {
if (is_array($value)) {
return implode(' ', array_map(function ($v) {
return $v;
}, $value));
}
return $value;
} elseif ($fallback) {
return $fallback;
}
return '';
}, $this->content);
}
Comment on lines +65 to +85
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

Clarify the purpose of the unused replaceMentions() method.

The replaceMentions() method appears to be an alternative implementation using regex for parsing mentions. However, it's not currently used in the class. Consider either:

  1. Removing this method if it's no longer needed.
  2. Documenting its purpose if it's intended for future use or as an alternative implementation.
  3. Refactoring the class to use this method if it's meant to be the primary implementation.

Keeping unused methods can lead to confusion and maintenance issues in the future.


private function getData($fieldId)
{
$value = collect($this->data)->filter(function ($item) use ($fieldId) {
return $item['id'] === $fieldId;
})->first()['value'] ?? null;
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

Ensure the collect helper function is accessible in this context

The code uses the collect helper function, which may not be available without the proper import or dependency. To ensure the function operates correctly, consider importing the necessary class or function. If you're using Laravel, add the following import at the top of your file:

use Illuminate\Support\Arr;

Alternatively, if you prefer to use the global helper function, ensure that it is accessible in your application context.

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

Prevent potential null dereference when accessing ['value']

The expression ->first()['value'] ?? null may cause an error if first() returns null, because you cannot access ['value'] on null. Modify the code to safely handle this case:

-$value = collect($this->data)->filter(function ($item) use ($fieldId) {
-    return $item['id'] === $fieldId;
-})->first()['value'] ?? null;
+$firstItem = collect($this->data)->filter(function ($item) use ($fieldId) {
+    return $item['id'] === $fieldId;
+})->first();
+$value = $firstItem['value'] ?? null;
📝 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
})->first()['value'] ?? null;
$firstItem = collect($this->data)->filter(function ($item) use ($fieldId) {
return $item['id'] === $fieldId;
})->first();
$value = $firstItem['value'] ?? null;


if (is_object($value)) {
return (array) $value;
}

return $value;
}
}
21 changes: 21 additions & 0 deletions api/app/Service/HtmlPurifier/OpenFormsHtmlDefinition.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

namespace App\Service\HtmlPurifier;

use HTMLPurifier_HTMLDefinition;
use Stevebauman\Purify\Definitions\Definition;
use Stevebauman\Purify\Definitions\Html5Definition;

class OpenFormsHtmlDefinition implements Definition
{
public static function apply(HTMLPurifier_HTMLDefinition $definition)
{
Html5Definition::apply($definition);

$definition->addAttribute('span', 'mention-field-id', 'Text');
$definition->addAttribute('span', 'mention-field-name', 'Text');
$definition->addAttribute('span', 'mention-fallback', 'Text');
$definition->addAttribute('span', 'mention', 'Bool');
$definition->addAttribute('span', 'contenteditable', 'Bool');
}
}
6 changes: 3 additions & 3 deletions api/config/purify.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php

use Stevebauman\Purify\Definitions\Html5Definition;
use App\Service\HtmlPurifier\OpenFormsHtmlDefinition;

return [

Expand Down Expand Up @@ -40,7 +40,7 @@
'configs' => [

'default' => [
'HTML.Allowed' => 'h1,h2,b,strong,i,em,a[href|title],ul,ol,li,p,br,span,*[style]',
'HTML.Allowed' => 'h1,h2,b,strong,i,em,a[href|title],ul,ol,li,p,br,span[mention|mention-field-id|mention-field-name|mention-fallback|contenteditable],*[style]',
'HTML.ForbiddenElements' => '',
'CSS.AllowedProperties' => 'font,font-size,font-weight,font-style,text-decoration,color,text-align',

Expand Down Expand Up @@ -86,7 +86,7 @@
|
*/

'definitions' => Html5Definition::class,
'definitions' => OpenFormsHtmlDefinition::class,

/*
|--------------------------------------------------------------------------
Expand Down
189 changes: 189 additions & 0 deletions client/components/forms/MentionInput.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
<template>
<InputWrapper v-bind="inputWrapperProps">
<template #label>
<slot name="label" />
</template>

<MentionDropdown
:state="mentionState"
:mentions="mentions"
/>

<div class="relative">
<div
ref="editableDiv"
:contenteditable="!disabled"
class="mention-input"
:style="inputStyle"
:class="[
theme.default.input,
theme.default.borderRadius,
theme.default.spacing.horizontal,
theme.default.spacing.vertical,
theme.default.fontSize,
{
'!ring-red-500 !ring-2 !border-transparent': hasError,
'!cursor-not-allowed dark:!bg-gray-600 !bg-gray-200': disabled,
},
'pr-12'
]"
@input="onInput"
/>
<UButton
type="button"
color="white"
class="absolute right-2 top-1/2 transform -translate-y-1/2 p-1 px-2"
icon="i-heroicons-at-symbol-16-solid"
@click="openMentionDropdown"
/>
</div>

<template
v-if="$slots.help"
#help
>
<slot name="help" />
</template>

<template
v-if="$slots.error"
#error
>
<slot name="error" />
</template>
</InputWrapper>
</template>

<script setup>
import { ref, onMounted, watch, computed } from 'vue'
import { inputProps, useFormInput } from './useFormInput.js'
import InputWrapper from './components/InputWrapper.vue'
import MentionDropdown from './components/MentionDropdown.vue'
const props = defineProps({
...inputProps,
mentions: { type: Array, default: () => [] },
})
const emit = defineEmits(['update:modelValue'])
const { compVal, inputStyle, hasValidation, hasError, inputWrapperProps } = useFormInput(props, { emit })
const editableDiv = ref(null)
const savedRange = ref(null)
const mentionState = ref({
open: false,
onInsert: (mention) => {
insertMention(mention)
mentionState.value.open = false
},
onCancel: () => {
mentionState.value.open = false
restoreSelection()
},
})
const createMentionSpan = (mention) => {
const mentionSpan = document.createElement('span')
mentionSpan.setAttribute('mention-field-id', mention.field.id)
mentionSpan.setAttribute('mention-field-name', mention.field.name)
mentionSpan.setAttribute('mention-fallback', mention.fallback || '')
mentionSpan.setAttribute('contenteditable', 'false')
mentionSpan.setAttribute('mention', 'true')
mentionSpan.textContent = mention.field.name.length > 25 ? `${mention.field.name.slice(0, 25)}...` : mention.field.name
return mentionSpan
}
const insertMention = (mention) => {
const mentionSpan = createMentionSpan(mention)
restoreSelection()
const range = window.getSelection().getRangeAt(0)
// Insert the mention span
range.insertNode(mentionSpan)
Comment on lines +95 to +97
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 null checks for selection and range objects

In functions like insertMention and restoreSelection, window.getSelection() and getRangeAt(0) may return null or cause errors if no selection exists. To prevent runtime exceptions, add null checks to ensure these objects are valid before using them.

Apply this diff to add null checks:

const insertMention = (mention) => {
  const mentionSpan = createMentionSpan(mention)
  restoreSelection()
+ const selection = window.getSelection()
+ if (!selection || selection.rangeCount === 0) {
+   return
+ }
- const range = window.getSelection().getRangeAt(0)
+ const range = selection.getRangeAt(0)
  // Insert the mention span
  range.insertNode(mentionSpan)
  // ...
}

const restoreSelection = () => {
  if (savedRange.value) {
+   const selection = window.getSelection()
+   if (!selection) {
+     return
+   }
    selection.removeAllRanges()
    selection.addRange(savedRange.value)
    editableDiv.value.focus()
  }
}

Also applies to: 131-135


// Move the cursor after the inserted mention
range.setStartAfter(mentionSpan)
range.collapse(true)
// Apply the new selection
const selection = window.getSelection()
selection.removeAllRanges()
selection.addRange(range)
// Ensure the editableDiv is focused
editableDiv.value.focus()
updateCompVal()
}
const openMentionDropdown = () => {
saveSelection()
if (!savedRange.value) {
// If no previous selection, move cursor to the end
const range = document.createRange()
range.selectNodeContents(editableDiv.value)
range.collapse(false)
const selection = window.getSelection()
selection.removeAllRanges()
selection.addRange(range)
savedRange.value = range
}
mentionState.value.open = true
}
const saveSelection = () => {
const selection = window.getSelection()
if (selection.rangeCount > 0) {
savedRange.value = selection.getRangeAt(0)
}
}
const restoreSelection = () => {
if (savedRange.value) {
const selection = window.getSelection()
selection.removeAllRanges()
selection.addRange(savedRange.value)
editableDiv.value.focus()
}
}
const updateCompVal = () => {
compVal.value = editableDiv.value.innerHTML
}
Comment on lines +138 to +140
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

Sanitize user input to prevent XSS vulnerabilities

In updateCompVal, you're assigning editableDiv.value.innerHTML directly to compVal.value, and vice versa. This can introduce XSS vulnerabilities if malicious content is inserted into the editableDiv. To mitigate this, consider sanitizing the HTML content before using it. Utilizing a library like DOMPurify can help sanitize user input.

Apply this diff to implement sanitization:

+import DOMPurify from 'dompurify'

...

const updateCompVal = () => {
-  compVal.value = editableDiv.value.innerHTML
+  compVal.value = DOMPurify.sanitize(editableDiv.value.innerHTML)
}

...

onMounted(() => {
  if (compVal.value) {
-    editableDiv.value.innerHTML = compVal.value
+    editableDiv.value.innerHTML = DOMPurify.sanitize(compVal.value)
  }
})

...

watch(compVal, (newVal) => {
  if (editableDiv.value && editableDiv.value.innerHTML !== newVal) {
-    editableDiv.value.innerHTML = newVal
+    editableDiv.value.innerHTML = DOMPurify.sanitize(newVal)
  }
})

Also applies to: 144-147, 148-152

const onInput = () => {
updateCompVal()
}
onMounted(() => {
if (compVal.value) {
editableDiv.value.innerHTML = compVal.value
}
})
watch(compVal, (newVal) => {
if (editableDiv.value && editableDiv.value.innerHTML !== newVal) {
editableDiv.value.innerHTML = newVal
}
})
defineExpose({
editableDiv,
compVal,
mentionState,
openMentionDropdown,
onInput,
})
</script>
Comment on lines +58 to +161
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

Comprehensive mention handling, but with room for improvement

The script section demonstrates good use of the Composition API and composables. However, there are a few areas that need attention:

  1. Input sanitization: The component doesn't sanitize user input, which could lead to XSS vulnerabilities. Consider using a library like DOMPurify to sanitize the HTML content.

  2. Null checks: Some methods like insertMention and restoreSelection don't have null checks for selection objects, which could lead to runtime errors.

  3. Direct innerHTML assignment: The updateCompVal method directly assigns innerHTML, which could be a security risk. Consider using safer alternatives or ensuring proper sanitization.

To address these issues, consider implementing the following changes:

  1. Add input sanitization:
import DOMPurify from 'dompurify'

const updateCompVal = () => {
  compVal.value = DOMPurify.sanitize(editableDiv.value.innerHTML)
}

onMounted(() => {
  if (compVal.value) {
    editableDiv.value.innerHTML = DOMPurify.sanitize(compVal.value)
  }
})

watch(compVal, (newVal) => {
  if (editableDiv.value && editableDiv.value.innerHTML !== newVal) {
    editableDiv.value.innerHTML = DOMPurify.sanitize(newVal)
  }
})
  1. Add null checks for selection objects:
const insertMention = (mention) => {
  const mentionSpan = createMentionSpan(mention)
  restoreSelection()
  const selection = window.getSelection()
  if (!selection || selection.rangeCount === 0) return
  const range = selection.getRangeAt(0)
  // ... rest of the method
}

const restoreSelection = () => {
  if (savedRange.value) {
    const selection = window.getSelection()
    if (!selection) return
    selection.removeAllRanges()
    selection.addRange(savedRange.value)
    editableDiv.value.focus()
  }
}

These changes will improve the security and robustness of the component.


<style scoped>
.mention-input {
min-height: 1.5rem;
white-space: pre-wrap;
word-break: break-word;
}
.mention-input:empty::before {
content: attr(placeholder);
color: #9ca3af;
}
Comment on lines +165 to +172
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

Ensure placeholder text is accessible

Using the ::before pseudo-element to display placeholder text may not be accessible to screen readers, impacting users relying on assistive technologies. Consider using the placeholder attribute or adding appropriate ARIA attributes to improve accessibility.

Apply this diff to enhance accessibility:

-.mention-input:empty::before {
-  content: attr(placeholder);
-  color: #9ca3af;
-}
+/* Remove the pseudo-element and use a data attribute */
+<div
+  ref="editableDiv"
+  :contenteditable="!disabled"
+  class="mention-input"
+  :style="inputStyle"
+  :class="[ /* existing classes */ ]"
+  @input="onInput"
+  :data-placeholder="placeholder"
+  role="textbox"
+  aria-multiline="true"
+/>

Update the CSS:

.mention-input:empty::before {
  content: attr(data-placeholder);
  color: #9ca3af;
}

.mention-input span[mention] {
max-width: 150px;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
display: inline-flex;
align-items: center;
background-color: #dbeafe;
color: #1e40af;
border: 1px solid #bfdbfe;
border-radius: 0.25rem;
padding: 0 0.25rem;
font-size: 0.875rem;
line-height: 1.25rem;
position: relative;
vertical-align: baseline;
}
</style>
Comment on lines +163 to +190
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 improving placeholder accessibility

The styles are well-structured and scoped to the component, which is good practice. However, the use of a pseudo-element for the placeholder text might not be accessible to screen readers.

Consider using the aria-placeholder attribute instead of the pseudo-element for better accessibility. Here's how you can modify the template and styles:

  1. Update the template:
<div
  ref="editableDiv"
  :contenteditable="!disabled"
  class="mention-input"
  :style="inputStyle"
  :aria-placeholder="placeholder"
  @input="onInput"
  <!-- ... other attributes ... -->
/>
  1. Update the styles:
.mention-input:empty::before {
  content: attr(aria-placeholder);
  color: #9ca3af;
}

This change will make the placeholder text accessible to screen readers while maintaining the visual appearance.

Loading
Loading