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

API Summary Length #12934

Closed
wants to merge 3 commits into from
Closed

API Summary Length #12934

wants to merge 3 commits into from

Conversation

thewilloftheshadow
Copy link
Contributor

@thewilloftheshadow thewilloftheshadow commented Jan 5, 2021

Fixes #9244

Changes

API now rejects both new challenge and challenges update if summary is longer than maximum length
Adds tests for this new check


UUID: 2eb203f5-1b1e-4e5d-84a4-22f130a1090e

@thewilloftheshadow
Copy link
Contributor Author

Can't get tests to run on my machine so I'm not really sure why these are failing right now

@thewilloftheshadow thewilloftheshadow marked this pull request as ready for review January 7, 2021 03:16
@paglias
Copy link
Contributor

paglias commented Jan 7, 2021

I would move the check to the model with a validator https://mongoosejs.com/docs/validation.html

@@ -200,6 +205,7 @@ api.createChallenge = {
const { user } = res.locals;

req.checkBody('group', apiError('groupIdRequired')).notEmpty();
if (req.body.challenge.summary && req.body.challenge.summary.length > MAX_SUMMARY_SIZE_FOR_CHALLENGES) throw new BadRequest(res.t('summaryTooLong'));
Copy link
Contributor

Choose a reason for hiding this comment

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

what if req.body.challenge is not defined? Also this cose should go after the validation errors

@paglias
Copy link
Contributor

paglias commented Jan 7, 2021

@thewilloftheshadow the tests are failing because you're not checking for the existence of req.body.challenge,

  fullError: TypeError: Cannot read property 'summary' of undefined
      at fn (/website/server/controllers/api-v3/challenges.js:208:28)

@thewilloftheshadow
Copy link
Contributor Author

Got it 👍🏻
This was just my initial test to see if it would catch that field, but I couldn't ever see the full errors from my machine

@paglias
Copy link
Contributor

paglias commented Jan 7, 2021

To see the error you have to:

  • In one terminal run NODE_ENV=test npm start
  • In another one do npm run test:api-v3:integration:separate-server and also set .only on the failing test so only that one will run, then you'll see the error logs in the first terminal window

@thewilloftheshadow
Copy link
Contributor Author

Ahh that's exactly what I needed! Tyty

@SabreCat
Copy link
Member

Closing this PR as abandoned. Worth peeking at for whoever picks this up next, though!

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.

API must enforce maximum length for summaries for Guilds and Challenges
3 participants