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

Enhance security of the Complete message for GraphQL over WebSocket protocol #5531

Merged
merged 2 commits into from Apr 2, 2024

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Mar 26, 2024

Motivation:
When constructing the Complete message in the GraphQL over WebSocket Protocol, appending a string directly to the ID can lead to malformed messages if the input is manipulated by the user. This vulnerability could potentially allow users to create arbitrary responses by inputting malformed IDs.

Modification:

  • Serialize the Complete message using a map instead of concatenating strings directly.

Result:

  • The Complete message in the GraphQL over WebSocket Protocol is now constructed securely.

@minwoox minwoox added the defect label Mar 26, 2024
@minwoox minwoox added this to the 1.28.0 milestone Mar 26, 2024
…rotocol

Motivation:
When constructing the `Complete` message in the GraphQL over WebSocket Protocol,
appending a string directly to the ID can lead to malformed messages if the input is manipulated by the user.
This vulnerability could potentially allow users to create arbitrary responses by inputting malformed IDs.

Modification:
- Serialize the `Complete` message using a map instead of concatenating strings directly.

Result:
- The `Complete` message in the GraphQL over WebSocket Protocol is now constructed securely.
@minwoox minwoox force-pushed the graphql_ws_complete_message branch from 8cea2b8 to 96a9a1e Compare March 26, 2024 14:29
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 74.04%. Comparing base (bcca9d3) to head (73c2c69).
Report is 8 commits behind head on main.

Files Patch % Lines
...p/armeria/server/graphql/GraphqlWSSubProtocol.java 50.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5531      +/-   ##
============================================
- Coverage     74.04%   74.04%   -0.01%     
+ Complexity    20853    20852       -1     
============================================
  Files          1807     1807              
  Lines         76743    76762      +19     
  Branches       9789     9792       +3     
============================================
+ Hits          56826    56838      +12     
- Misses        15293    15300       +7     
  Partials       4624     4624              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

👍👍

@jrhee17 jrhee17 merged commit 8263e80 into line:main Apr 2, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants