Skip to content

fix: allow apostrophes in validatePersonName and unit tests - #9667

Open
UGilfoyle wants to merge 1 commit into
makeplane:previewfrom
UGilfoyle:fix/validate-person-name-apostrophe
Open

fix: allow apostrophes in validatePersonName and unit tests#9667
UGilfoyle wants to merge 1 commit into
makeplane:previewfrom
UGilfoyle:fix/validate-person-name-apostrophe

Conversation

@UGilfoyle

@UGilfoyle UGilfoyle commented Aug 22, 2026

Copy link
Copy Markdown

Description

Fixed an issue in validatePersonName where legitimate names with apostrophes (e.g. O'Brien, D'Angelo, O'Connor, N'Golo) were rejected by validation due to single quotes triggering hasInjectionRiskChars.

  • Added an optional { allowApostrophe?: boolean } parameter to hasInjectionRiskChars.
  • Updated validatePersonName to explicitly allow apostrophes while continuing to block dangerous injection vectors (< > " { } [ ] * ^ ! # %).
  • Added a dedicated test suite with Vitest in @plane/utils.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

N/A (Utility logic & validation fix)

Test Scenarios

  • Added 18 unit tests in packages/utils/tests/validation.test.ts covering:
    • Standard names (John, Jane Doe, Jean-Paul).
    • Names with apostrophes (O'Brien, D'Angelo, O'Connor, N'Golo, D'Souza).
    • International Unicode names (José, Müller, 李明, محمد, Александр).
    • Injection attempts (John<script>, John{admin}, John*).
    • Display names, company/workspace names, and slugs.
  • Ran pnpm --filter=@plane/utils test (18/18 passed).
  • Ran full workspace checks: pnpm turbo run check:types check:lint check:format (All 60 tasks passed).

References

N/A

Summary by CodeRabbit

  • Bug Fixes

    • Improved person-name validation to allow apostrophes in valid names while continuing to detect other unsafe characters.
    • Preserved existing validation behavior for other input types.
  • Tests

    • Added comprehensive coverage for name, slug, injection-risk, Unicode, length, and optional-value validation scenarios.

@CLAassistant

CLAassistant commented Aug 22, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5651516b-8be3-4b3e-b173-0a2b76287723

📥 Commits

Reviewing files that changed from the base of the PR and between e056bbf and 1bb9d32.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • packages/utils/package.json
  • packages/utils/src/validation.ts
  • packages/utils/tests/validation.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Validation updates

Layer / File(s) Summary
Person-name injection handling
packages/utils/src/validation.ts
validatePersonName allows apostrophes during injection checks. hasInjectionRiskChars supports an optional allowApostrophe setting while preserving its default behavior.
Validation test coverage
packages/utils/package.json, packages/utils/tests/validation.test.ts
The utils package adds a Vitest test script and development dependency. Tests cover person, display, company, workspace, slug, and injection-risk validation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 1bb9d

This change allows legitimate apostrophes in person names while retaining the existing blocked-character checks. It is a localized update with passing tests and workspace checks, so no actionable merge-blocking risk remains after normal review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the apostrophe validation fix and the added unit tests.
Description check ✅ Passed The description includes all template sections and provides clear change details, test scenarios, and validation results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants