Skip to content

Conversation

porcellus
Copy link
Collaborator

Summary of change

  • Checking for protected props in JWTDataAPI to make sure it can't break a session
  • Added the same checks for other endpoints - it's more consistent, and it makes sure that the changes do not get saved in the DB
  • We still need the check when serializing the access tokens to ensure. The above checks are mainly for user convenience that is more about security.

Related issues

  • Link to issue1 here
  • Link to issue1 here

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)

Checklist for important updates

  • Changelog has been updated
    • If there are any db schema changes, mention those changes clearly
  • coreDriverInterfaceSupported.json file has been updated (if needed)
  • pluginInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In build.gradle
  • If added a new paid feature, edit the getPaidFeatureStats function in FeatureFlag.java file
  • Had installed and ran the pre-commit hook
  • If there are new dependencies that have been added in build.gradle, please make sure to add them
    in implementationDependencies.json.
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the
      latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

  • Item1
  • Item2

Comment on lines 62 to 65
if (getVersionFromRequest(req).greaterThanOrEqualTo(SemVer.v2_21) &&
Arrays.stream(protectedPropNames).anyMatch(userDataInJWT::has)) {
throw new ServletException(new BadRequestException("The user payload contains protected field"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

these checks should not happen here, instead, update updateSession to have this check, and create another updateSessionBeforeCDI2_21 without the check and mark that as deprecated

Comment on lines 91 to 94

if (Arrays.stream(protectedPropNames).anyMatch(userDataInJWT::has)) {
throw new ServletException(new BadRequestException("The user payload contains protected field"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (Arrays.stream(protectedPropNames).anyMatch(userDataInJWT::has)) {
throw new ServletException(new BadRequestException("The user payload contains protected field"));
}

Comment on lines 73 to 77
if (getVersionFromRequest(req).greaterThanOrEqualTo(SemVer.v2_21) &&
userDataInJWT != null &&
Arrays.stream(protectedPropNames).anyMatch(userDataInJWT::has)) {
throw new ServletException(new BadRequestException("The user payload contains protected field"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (getVersionFromRequest(req).greaterThanOrEqualTo(SemVer.v2_21) &&
userDataInJWT != null &&
Arrays.stream(protectedPropNames).anyMatch(userDataInJWT::has)) {
throw new ServletException(new BadRequestException("The user payload contains protected field"));
}

@rishabhpoddar rishabhpoddar merged commit 1c95466 into 5.0 Apr 6, 2023
@rishabhpoddar rishabhpoddar deleted the fix/throw_for_protected_props branch April 6, 2023 15:33
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