Add encrypt/decrypt filters and scripting methods (#19099)

[erberkson]

Introduced encrypt and decrypt Liquid filters using ASP.NET Core Data Protection, enabling secure string encryption/decryption in templates. Added corresponding scripting methods for JavaScript contexts. Updated documentation and constants, and included comprehensive unit tests for both filters and scripting methods.

[matrixreview]

🔴 MatrixReview — RED

🔎 = doc-backed finding  ·  💭 = AI suggestion  ·  📖 = doc citation  ·  📝 = PR location

Findings: 12 (10 doc-backed, 4 AI suggestions)

🔴 SECURITY — 4 findings (4 doc-backed) · expand 🔽

• 🔎 [SECURITY] The PR introduces new Liquid filters encrypt and decrypt and corresponding JavaScript methods that allow encryption/decryption of arbitrary strings using the ASP.NET Core Data Protection API. T...

  Read more · expand 🔽

  ...his could be misused to store secrets in templates or scripts, which is discouraged per the company's security documentation. The documentation for the protect function warns: 'The protect function is intended for use during development and testing scenarios only. Storing secrets in recipe files for production environments is not recommended and should be avoided.' The new encrypt/decrypt functions serve a similar purpose and should carry the same warning.

  📖 *README_security_section.md lines 72-72* 📝 `src/docs/reference/modules/Scripting/README.md line 74`

• 🔎 [SECURITY] The decrypt filter and JavaScript method return an empty string on decryption failure (e.g., invalid ciphertext). This behavior could mask security issues or lead to data loss if the decryption f...

  Read more · expand 🔽

  ...ails silently. It also differs from the protect function's behavior (which likely throws). Inconsistent error handling can be a security risk.

  📖 *SECURITY.md lines 1-7* 📝 `src/OrchardCore.Modules/OrchardCore.Liquid/Filters/DataProtectionFilters.cs line 53`

• 🔎 [SECURITY] The encryption uses a fixed purpose string 'oc-scripting'. While this is appropriate for a single purpose, the documentation does not specify if this purpose is shared with other operations. If the...

  Read more · expand 🔽

  ... same purpose is used elsewhere, it could allow cross-protection attacks. The purpose should be clearly documented and isolated.

  - *Also flagged by: ARCHITECTURE, STYLE* 📖 *Permissions_security_section.md lines 1-10* 📝 `src/OrchardCore/OrchardCore.Abstractions/OrchardCoreConstants.cs line 102`

• 🔎 [SECURITY] The encrypt and decrypt functions are exposed in Liquid templates and JavaScript scripting, which could be accessible to users with permission to edit templates or scripts. According to the per...

  Read more · expand 🔽

  ...missions documentation, 'ManageTemplates' and 'ManageAdminTemplates' are security-critical permissions. However, if lower-privileged users can inject Liquid or JavaScript (e.g., via content fields), they could potentially encrypt/decrypt data. This expands the attack surface.

  📖 *Permissions_security_section.md lines 1-5* 📝 `src/OrchardCore.Modules/OrchardCore.Liquid/Startup.cs line 80`

🟡 ARCHITECTURE — 3 findings (2 doc-backed, 1 AI) · expand 🔽

• 🔎 [ARCHITECTURE] The PR introduces new Liquid filters encrypt and decrypt that expose data protection operations directly in templates. This violates the architectural principle of keeping business logic out of...

  Read more · expand 🔽

  ... templates and could lead to security misconfigurations if used incorrectly. The architecture documentation emphasizes templates are for rendering shapes and content, not for cryptographic operations.

  📖 *README.md lines 1-267* 📝 `src/OrchardCore.Modules/OrchardCore.Liquid/Filters/DataProtectionFilters.cs line 1`

• 💭 [ARCHITECTURE] The PR adds InternalsVisibleTo for OrchardCore.Tests in the Scripting project. This is a common testing pattern in Orchard Core, but the architecture documentation doesn't explicitly cover test...

  Read more · expand 🔽

  ... project visibility patterns.

  📝 `src/OrchardCore.Modules/OrchardCore.Scripting/OrchardCore.Scripting.csproj line 22`

• 🔎 [ARCHITECTURE] The PR adds comprehensive unit tests for both Liquid filters and scripting methods, following the testing patterns documented in the architecture. The tests use the established SiteContext pattern ...

  Read more · expand 🔽

  ...and follow the unit test structure shown in the documentation.

  📖 *AGENTS_architecture_section.md lines 1-50* 📝 `test/OrchardCore.Tests/Modules/OrchardCore.Liquid/DataProtectionFilterTests.cs line 1`

🟢 LEGAL — No issues found

🟡 STYLE — 2 findings (1 doc-backed, 1 AI) · expand 🔽

• 💭 [STYLE] The PR adds a new test file DataProtectionFilterTests.cs with a test method DecryptFilterShouldReturnNilForInvalidCiphertext. The test uses a raw string literal with triple quotes and string in...

  Read more · expand 🔽

  ...terpolation ($$$"""{{ "{{{invalidCiphertext}}}" | decrypt }}"""). The style guide documentation does not provide explicit rules for test naming or string literal formatting in tests.

  📝 `test/OrchardCore.Tests/Modules/OrchardCore.Liquid/DataProtectionFilterTests.cs line 64`

• 🔎 [STYLE] The PR updates documentation files (README.md for Liquid and Scripting modules) to include new sections for encrypt and decrypt filters/functions. The style guide documentation includes examp...

  Read more · expand 🔽

  ...les of documentation formatting (e.g., tables, code blocks, headers). The new documentation sections appear to follow similar patterns, but there is no explicit rule violation.

  📖 *README_style_section.md (v2) lines 1-30*

🔴 ONBOARDING — 3 findings (3 doc-backed) · expand 🔽

• 🔎 [POLICY_VIOLATION] The PR adds new Liquid filters (encrypt/decrypt) and scripting methods but does not include documentation updates for the release notes of the upcoming release, as required by the contribution ...

  Read more · expand 🔽

  ...guidelines for 'significant new features'.

  📖 *contributing-code_onboarding_section.md lines 15-16*

• 🔎 [POLICY_VIOLATION] The PR adds new functionality (encrypt/decrypt filters and scripting methods) but does not link to an existing issue, as recommended by the contribution guidelines for PRs that address issues.
  📖 contributing-code_onboarding_section.md lines 8-9

• 🔎 [CHORE] The PR description is minimal and does not fully describe the context and rationale for the changes, as suggested by the contribution guidelines. It lacks screenshots/screen recordings (if UX affec...

  Read more · expand 🔽

  ...ted) and a more detailed explanation.

  📖 *contributing-code_onboarding_section.md lines 10-12*

👆 Click expand on any gate above to see full findings with evidence and citations.

---

Powered by MatrixReview · Report incorrect finding

⚙️ Generate Fix