feat(n8n Form Trigger Node): Form Improvements

[dana-gill]

Summary

• Allows users to use HTML in Form descriptions and also sanitizes the HTML to prevent Javascript from being executed.
  For this PR, we are only allowing a select number of HTML elements. We can easily add more by adding more to the sanitize options.
• We are also populating the rendered Form's description metadata with the form description
• Add OG metadata to improve sharing across the Internet

Node for testing:

{
  "nodes": [
    {
      "parameters": {
        "formTitle": "form",
        "formDescription": "<script>alert('hello world')</script>\n<i>hello</i>",
        "formFields": {
          "values": [
            {
              "fieldLabel": "name"
            },
            {
              "fieldLabel": "age",
              "fieldType": "number",
              "requiredField": true
            }
          ]
        },
        "options": {}
      },
      "type": "n8n-nodes-base.formTrigger",
      "typeVersion": 2.2,
      "position": [
        -100,
        -60
      ],
      "id": "51631ae1-11ac-4289-942d-159e90013470",
      "name": "On form submission",
      "webhookId": "eb1eed7e-5b3e-4582-962f-fd955d9f44f1"
    }
  ],
  "connections": {
    "On form submission": {
      "main": [
        []
      ]
    }
  },
  "pinData": {}
}

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/NODE-2227/sanitize-rendered-html-for-form-descriptions
https://linear.app/n8n/issue/NODE-2228/render-html-for-form-descriptions
https://linear.app/n8n/issue/NODE-2226/description-parameter-html-formatting
https://linear.app/n8n/issue/NODE-2237/form-title-and-form-description-metadata

Review / Merge checklist

• PR title and summary are descriptive. (conventions)
• Docs updated or follow-up ticket created.
• Tests included.
• PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

[michael-radency]

LGTM, some suggestions:

• update parameter description to mention that field accepts HTML
• add 'target' and 'rel' to allowedAttributes
• extend allowedTags by 'u', 'sub', 'sup', 'code', 'pre', 'span', 'br'

is there a way to tell if not allowed attributes/tags used? if so worth adding post execution hint

[dana-gill]

is there a way to tell if not allowed attributes/tags used? if so worth adding post execution hint

I think we would be able to do this by doing something like context.getNodeParameter('formDescription', '') === sanitize(context.getNodeParameter('formDescription', '')) so this is possible.

Do you have an idea of the best way to add the post execution hint? From my understanding, formWebhook is called when the Form is rendered.

[michael-radency]

is there a way to tell if not allowed attributes/tags used? if so worth adding post execution hint

I think we would be able to do this by doing something like context.getNodeParameter('formDescription', '') === sanitize(context.getNodeParameter('formDescription', '')) so this is possible.

Do you have an idea of the best way to add the post execution hint? From my understanding, formWebhook is called when the Form is rendered.

never mind, I just tested - NodeExecutionOutput hints does not work with trigger and we do not have helper to sanitize HTML in expressions, so we could not define displayCondition in node's hints property either

[michael-radency]

LGTM 🎉

[cypress]

n8n    Run #8756

Run Properties:   Failed #8756  •  5285dd15a0: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 dana-gill 🗃️ e2e/*

Project	n8n
Branch Review	node-2228-html-form-description
Run status	Failed #8756
Run duration	04m 15s
Commit	5285dd15a0: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 dana-gill 🗃️ e2e/*
Committer	Dana Lee
View all properties for this run ↗︎

---

Test results
Failures	4
Flaky	1
Pending	0
Skipped	0
Passing	277
View all changes introduced in this branch ↗︎

---

Tests for review

  30-langchain.cy.ts • 2 failed tests

View Output Video

Test		Artifacts
Langchain Integration > should add and use Manual Chat Trigger node together with Agent node		Test Replay Screenshots Video
Langchain Integration > should render runItems for sub-nodes and allow switching between them		Test Replay Screenshots Video

  19-execution.cy.ts • 2 failed tests

View Output Video

Test		Artifacts
Execution > should send proper payload for manual node run		Test Replay Screenshots Video
Execution > should successfully execute partial executions with nodes attached to the second output		Test Replay Screenshots Video

  6-code-node.cy.ts • 0 failed tests

View Output

Test		Artifacts

  45-workflow-selector-parameter.cy.ts • 0 failed tests

View Output

Test		Artifacts

  41-editors.cy.ts • 0 failed tests

View Output

Test		Artifacts

The first 5 failed specs are shown, see all 52 specs in Cypress Cloud.

  e2e/12-canvas.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Canvas Node Manipulation and Navigation > should preserve connections after rename & node-view switch		Test Replay Screenshots Video

[github-actions]

✅ All Cypress E2E specs passed

[michael-radency]

should we have separate parameter in options for form description metatada with fallback to description?

[dana-gill]

should we have separate parameter in options for form description metatada with fallback to description?

I thought about doing that especially during this Slack conversation. I get the feeling that we would want to do this as part of an initiative to allow users to set metadata themselves since that would require a bit more specing (I think it was suggested to maybe use a fixedCollection as well). I also added it under Various Requests in this doc. As a result, I thought of taking a leaner and less invasive approach since having to add an additional field would require touching many parts of the Form node -- at least 3-4 functions, multiple test cases, and the *.d.ts files. I thought, "If adding this one extra parameter takes this much work, maybe it's worth waiting until allowing users to set metadata so that we can use time wisely." Let me know if you think this makes sense :)

[michael-radency]

LGTM

[github-actions]

⚠️ Some Cypress E2E specs are failing, please fix them before merging