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

Validation #397

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Validation #397

wants to merge 7 commits into from

Conversation

BassOfBass
Copy link
Collaborator

Fix mentioned in #396 (comment).

@BassOfBass
Copy link
Collaborator Author

@mayeaux
Correct me if I am wrong, but channelName, channel description and comment are only affected by these endpoints:

  • POST /:channel/:media - comments
  • POST /account/profile - ChannelName and channel description
    So I've added validation and sanitization to middleware of these endpoints.

@mayeaux
Copy link
Owner

mayeaux commented Dec 28, 2020

Looks really good! I like the JSDocs usage I was planning to start that soon so I'm glad you got the ball rolling.

Quick question what's this for:
const express = require("express"); //JSDoc types only

Other than that I can merge this right away, thanks my friend!

@BassOfBass
Copy link
Collaborator Author

Quick question what's this for:
const express = require("express"); //JSDoc types only

It's for better type inference. VSCode comes pre-bundled with Typescript which adds a lot of type info and tooltips and also infers types off JSDoc comments.

@mayeaux
Copy link
Owner

mayeaux commented Dec 30, 2020

@BassOfBass : check the review:

this works for you? it throws an error for me

TypeError: req.sanitize(...).trim(...).escape is not a function
at exports.postSignup (/Users/anthony/Development/nodetube/controllers/backend/account.js:148:37)

@BassOfBass
Copy link
Collaborator Author

@mayeaux Guess this version of validator can't into chaining.

@BassOfBass
Copy link
Collaborator Author

@mayeaux please review again.

Copy link
Collaborator

@alechash alechash left a comment

Choose a reason for hiding this comment

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

Looks good after validation chaining fix

I believe validator now supports chaining. Any reason not to just update validator in dependencies?

@BassOfBass
Copy link
Collaborator Author

@mr-winson
The newest version works as collection of functions instead of middleware, so it will require complete rewrite of every req.assert() instance. But more importantly there should be a single source of truth in regards to validation constraints in the first place,

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.

None yet

3 participants