Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard blitz generate input against unwanted characters #4024

Merged
merged 11 commits into from
Jan 26, 2023

Conversation

tordans
Copy link
Contributor

@tordans tordans commented Dec 31, 2022

Closes: #4021

What are the changes and their implications?

The input strings passed to blitz g are checked against an allow list of characters via regex. The helper throws if an unwanted character is found.

I think it would be enough to check in prisma/fields.ts. The main reason for added it there was so the tests in prisma/fields.test.ts can be used to test the regex. However, generators/model-generator.ts has the raw string (right?), so this would be the place it first.
To keep the code simpler, we could remove it from generators/model-generator.ts again – let me know what you thing.

Also, please give the regex and everything a good review. I am new new to this :-).

Bug Checklist

  • Changeset added (run pnpm changeset in the root directory)
  • Integration test added (see test docs if needed)

Feature Checklist

@changeset-bot
Copy link

changeset-bot bot commented Dec 31, 2022

🦋 Changeset detected

Latest commit: 3c8d38f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@blitzjs/generator Patch
blitz Patch
@blitzjs/codemod Patch
@blitzjs/auth Patch
@blitzjs/next Patch
@blitzjs/rpc Patch
@blitzjs/config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@itsdillon itsdillon requested a review from flybayer as a code owner January 5, 2023 23:55
@siddhsuresh
Copy link
Member

siddhsuresh commented Jan 11, 2023

Hey @tordans, are you able to commit the change requested? We can merge it then.

@tordans
Copy link
Contributor Author

tordans commented Jan 12, 2023

are you able to commit the change requested? We can merge it then.

@siddhsuresh I looked into it and create #4053 as a follow up. I don't know enough about regex to add tests without spending too much time on it.

I hope this can be merged as a first step and #4053 continued as an improvement.

My primary goal was, to prevent the "catastrophic failure" (as in, "nothings works anymore"). Since it looks like I was the first to have this issue, it also looks like it's not too common, so having a rougher error message might be enough…

@siddhsuresh
Copy link
Member

siddhsuresh commented Jan 12, 2023

are you able to commit the change requested? We can merge it then.

@siddhsuresh I looked into it and create #4053 as a follow up. I don't know enough about regex to add tests without spending too much time on it.

I hope this can be merged as a first step and #4053 continued as an improvement.

My primary goal was, to prevent the "catastrophic failure" (as in, "nothings works anymore"). Since it looks like I was the first to have this issue, it also looks like it's not too common, so having a rougher error message might be enough…

Hey, @tordans, if it's fine with you I could commit the change and the changes to the tests to make it work.
Also, I think we should be able to finish these changes in this PR only, so the second PR shouldn't be necessary

@tordans
Copy link
Contributor Author

tordans commented Jan 13, 2023

@siddhsuresh

Hey, …if it's fine with you I could commit the change and the changes to the tests to make it work.

Absolutely, always! Thanks! :-)

@siddhsuresh siddhsuresh merged commit cb63a0e into blitz-js:main Jan 26, 2023
@blitzjs-bot
Copy link
Contributor

Added @tordans contributions for test

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

Successfully merging this pull request may close these issues.

blitz g with brackets can break the cli
5 participants