Skip to content

Update input validation#110

Merged
MoMannn merged 19 commits into
devfrom
chore/number-validation
Aug 6, 2025
Merged

Update input validation#110
MoMannn merged 19 commits into
devfrom
chore/number-validation

Conversation

@MoMannn

@MoMannn MoMannn commented Jul 14, 2025

Copy link
Copy Markdown
Member

Description

In this PR we update input validation from the point of developer setting permission parameters:

  1. we make startTime nullable -> defaults to Date.now
  2. if startTime is defined its validated that the date is today or later
  3. tokenAddress is validated that is really is an address
  4. expiryTime is validated that is in the future
  5. validation is moved to zod where feasable

Manual testing steps

  1. Go to http://localhost:8000/
  2. Change inputs into invalid versions
  3. Should fail directly before opening up the snap

Screenshots/Recordings

Before

Uploading Screen Recording 2025-07-31 at 13.01.09.mov…

After

Screen.Recording.2025-07-31.at.12.56.47.mov

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 14, 2025 13:56
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@socket-security

socket-security Bot commented Jul 31, 2025

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​metamask/​delegation-toolkit@​0.13.0-rc.1851009394100

View full report

cursor[bot]

This comment was marked as outdated.

@MoMannn MoMannn changed the title [Draft] Update input validation Update input validation Jul 31, 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.

Looking good!

I'm definitely looking forward to de-duplicating the permission type logic!

Comment thread packages/gator-permissions-snap/src/permissions/contextValidation.ts Outdated
Comment thread packages/gator-permissions-snap/src/permissions/erc20TokenPeriodic/caveats.ts Outdated
Comment thread packages/gator-permissions-snap/src/utils/validate.ts Outdated
@MoMannn MoMannn changed the base branch from main to dev August 1, 2025 08:10
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@MoMannn MoMannn requested a review from jeffsmale90 August 1, 2025 10:47
jeffsmale90
jeffsmale90 previously approved these changes Aug 5, 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.

Snapshot conflicts aside, this looks great!

cursor[bot]

This comment was marked as outdated.

jeffsmale90
jeffsmale90 previously approved these changes Aug 5, 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.

🍏

setStartTime(null);
} else {
setStartTime(Number(inputValue));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Invalid Input Handling Causes NaN StartTime

The handleStartTimeChange function in ERC20TokenStreamForm, NativeTokenPeriodicForm, ERC20TokenPeriodicForm, and NativeTokenStreamForm components directly converts user input to a number without validating if it's a valid numeric string. This can result in NaN being set as the startTime state for non-numeric inputs, leading to confusing validation errors.

Additional Locations (3)
Fix in Cursor Fix in Web

@MoMannn MoMannn requested a review from jeffsmale90 August 6, 2025 07:32
@MoMannn MoMannn merged commit b9c58f8 into dev Aug 6, 2025
16 checks passed
@MoMannn MoMannn deleted the chore/number-validation branch August 6, 2025 07:44
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