Skip to content

Ensure justification is sanitised#109

Merged
MoMannn merged 17 commits into
mainfrom
chore/justification-sanitation
Jul 25, 2025
Merged

Ensure justification is sanitised#109
MoMannn merged 17 commits into
mainfrom
chore/justification-sanitation

Conversation

@MoMannn

@MoMannn MoMannn commented Jul 11, 2025

Copy link
Copy Markdown
Member

Description

  1. Reason for change

Presently any string data provided via justification is rendered verbatim to the confirmation screen, which is a vector for UI redressing

Ensure that justification is sufficiently sanitised before rendering (ideally as early as possible).

The following should be enforced, removed, or sanitized: (plus any other necessary validation or sanitisation)

Maximum length
Control characters
JSON or XML
Excessive whitespace
single or double quotes
In case of null/empty justification use default text: No justification was provided for the permission

  1. Solution

With Zod we do strict sanitation of Justification hitting all the UI redressing vectors, scripts, zero-width characters, dangerous protocols are mod.

Manual testing steps

  1. Open the demo dapp at localhost:8000
  2. In Justification box input text you desire to test.
  3. Click grant permission.

Screenshots/Recordings

Screenshot 2025-07-11 at 14 03 42 Screenshot 2025-07-22 at 11 26 04

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@MoMannn MoMannn requested a review from a team as a code owner July 11, 2025 12:06
cursor[bot]

This comment was marked as outdated.

jeffsmale90
jeffsmale90 previously approved these changes Jul 14, 2025

@jeffsmale90 jeffsmale90 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of nits, plus the shasum mismatch, but this looks great!

Comment thread packages/shared/test/types.test.ts Outdated
Comment thread packages/shared/test/types.test.ts Outdated
cursor[bot]

This comment was marked as outdated.

@MoMannn MoMannn requested a review from jeffsmale90 July 17, 2025 12:25
cursor[bot]

This comment was marked as outdated.

jeffsmale90
jeffsmale90 previously approved these changes Jul 21, 2025

@jeffsmale90 jeffsmale90 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's beautiful!

Can you please look into those Cursor flagged issues above, and resolve the shasum mismatch (my least favourite thing).

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

jeffsmale90
jeffsmale90 previously approved these changes Jul 22, 2025

@jeffsmale90 jeffsmale90 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - I am unable to test with null justification, as this needs to be integrated into the SDK.

Comment thread packages/shared/src/types/7715-permissions-types.ts
Comment thread packages/gator-permissions-snap/test/rpc/rpcHandler.test.ts
@jeffsmale90 jeffsmale90 self-requested a review July 25, 2025 10:27
jeffsmale90
jeffsmale90 previously approved these changes Jul 25, 2025
Comment thread packages/shared/src/types/7715-permissions-types.ts
@MoMannn MoMannn merged commit fb7db53 into main Jul 25, 2025
16 checks passed
@MoMannn MoMannn deleted the chore/justification-sanitation branch July 25, 2025 10:43
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