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

Implement payload checksum generation and validation #174

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

mrigsby
Copy link
Contributor

@mrigsby mrigsby commented Jan 18, 2025

  • Added _caclulateChecksum() and _validateChecksum() to models/CBWIREController.cfc using the hmac() function
    • The hmac() function looks for a module setting named secret and if not found falls back to a hash of the cbwire root path hash( moduleSettings.moduleRootPath )
  • Implemented both functions in models/Component.cfc
  • Implemented _caclulateChecksum() in test-harness/tests/specs/CBWIRESpec.cfc incomingRequest()
  • If the checksum is not found in the payload or there is a checksum validation failure it throws CBWIRECorruptPayloadException

Note: All tests using models/CBWIREController.cfc handleRequest() function are now using the checksum functionality and are passing. The test-harness/tests/specs/CBWIRESpec.cfc incomingRequest() function also now passes the snapshot as a JSON string which matches how Livewire posts the snapshot on incoming requests.

@grantcopley, let me know if you see anything I missed or suggestions.

@bdw429s
Copy link

bdw429s commented Jan 18, 2025

This pull request has been mentioned on Ortus Solutions Community. There might be relevant details there:

https://community.ortussolutions.com/t/page-expired-error/10454/11

Copy link
Collaborator

@grantcopley grantcopley left a comment

Choose a reason for hiding this comment

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

@mrigsby Looks good but I've noted a few changes, mainly around avoiding regex parsing. Also if you could add some test cases that would be awesome.

Thanks so much for the work on this!

/**
* Calculates a checksum for the component's data.
*
* @payload string | The name of the computed property.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arguments in comments for method need to be updated

@@ -73,6 +73,7 @@ component singleton {
}
// Perform additional deserialization of the component snapshots
local.payload.components = local.payload.components.map( function( _comp ) {
_validateChecksum( arguments._comp.snapshot );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to change this up a bit. You are passing in the snapshot as a string into _validateChecksum but we are already deserializing it below this line into an object. I would move this to after the deserialization call so that we don't have to use regex methods ( which are slow ) for parsing the checksum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I was too caught up with the issue of unordered structs in CF as opposed to JS and PHP, thus causing potentially different JSON strings after serializeJSON/deserializeJSON that I didn't even think about deserializing to get checksum value, doing a replace on the original payload string to remove the checksum and then verifying... I will get that fixed up shortly.

/**
* Validates checksum for the component's data from snapshot.
*
* @payload string | The name of the computed property.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arguments in comments need fixing

@@ -992,7 +992,7 @@ component output="true" {
* @return String The generated checksum.
*/
function _generateChecksum() {
return "f9f66fa895026e389a10ce006daf3f59afaec8db50cdb60f152af599b32f9192";
return "f9f66fa895026e389a10ce006daf3f59afaec8db50cdb60f152af599b32f9192IMHERE";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this method can be removed entirely now right?

*
* @return void
*/
function _validateChecksum( snapshot ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change method to accept snapshot as a struct so we can avoid regex parsing

Copy link
Contributor Author

@mrigsby mrigsby Jan 20, 2025

Choose a reason for hiding this comment

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

Livewire passes the payload as a JSON string. The crux of the issue is that we can't use deserialize the payload, remove the checksum key, then serialize the struct and expect to get the same JSON string that was originally used to generate the checksum like livewire does. PHP and JS both keep struct/object ordering, but CFML doesn't and deserializeJSON() doesn't have an option to produce an ordered struct.

I can avoid the regex completely by deserializing the payload, getting the checksum value, using replace on the original JSON string payload to clear out the value of the checksum, then verifying the checksum.

Note: I changed the payload to always include and empty string for the checksum before generating the checksum.

I hope I am clearly explaining the issue. I will try to get this all cleaned up in the next little bit so you can take a look at the changes.

@mrigsby
Copy link
Contributor Author

mrigsby commented Jan 20, 2025

  • Added Test "can return a valid JSON snapshot string with a valid checksum" that generates a snapshot then validates the checksum
  • Added Test "throws error when snapshot is tampered with" that generates a snapshot then changes a data property and expects error
  • removed the use of regular expression
  • removed _generateChecksum() function that is no longer needed
  • Added previously overlooked checksum functionality for lazy loading wires

The _caclulateChecksum( snapshot ) takes in a struct and returns a JSON string, and _validateChecksum( snapshot ) function takes in a JSON string and throws an error if its invalid. This comment might shed some light on why I did it that way.

Let me know if you are ok with how I reworked_validateChecksum( snapshot ) so it doesn't use regex. I believe I have addressed everything mentioned in the comments.

@mrigsby
Copy link
Contributor Author

mrigsby commented Jan 20, 2025

  • Added Test "can return a valid JSON snapshot string with a valid checksum" that generates a snapshot then validates the checksum
  • Added Test "throws error when snapshot is tampered with" that generates a snapshot then changes a data property and expects error
  • removed the use of regular expression
  • removed _generateChecksum() function that is no longer needed
  • Added previously overlooked checksum functionality for lazy loading wires

The _caclulateChecksum( snapshot ) takes in a struct and returns a JSON string, and _validateChecksum( snapshot ) function takes in a JSON string and throws an error if its invalid. This comment might shed some light on why I did it that way.

Let me know if you are ok with how I reworked_validateChecksum( snapshot ) so it doesn't use regex. I think I addressed everything mentioned in the comments!

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.

3 participants