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

Handle unencrypted message while getting messages with crypto #158

Merged
merged 9 commits into from
Nov 27, 2023

Conversation

Xavrax
Copy link
Contributor

@Xavrax Xavrax commented Nov 23, 2023

fix: Handle unencrypted message while getting messages with crypto

Handle unencrypted message while getting messages with crypto

@@ -971,6 +971,10 @@ func parseCipherInterface(data interface{}, pnConf *Config, module crypto.Crypto
pnConf.Log.Println("v[pn_other]", v["pn_other"], v, msg)
decrypted, errDecryption := decryptString(module, msg)
if errDecryption != nil {
if strings.Contains(errDecryption.Error(), "decrypt error on decode") {
pnConf.Log.Println("Couldn't decode message. Asumming no encrypted message. Returning raw message...")
return msg, nil

Choose a reason for hiding this comment

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

Don't we want to inform user that there was a problem with message decoding?
I thought in this case we return error to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid that our user will first check if there is any error and their program will fail in case of no encrypted message anyway what is pointless in this case. As I understood we should return Result type if it is possible in your implementation. Go is quite straightforward what means we cannot indicate the "something is not okay but we returning something anyway" in any way. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed approach.


assert.Nil(err)
assert.Equal("test", result)
}

Choose a reason for hiding this comment

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

I am not sure if it is worth adding it here but in Kotlin there are tests(one parameterized) that checks behaviour of the sdk when you try decrypt unencrypted message.

For this message: "this is unencrypted message"
This error is thrown:
PubNubException(errorMessage=Input length must be multiple of 16 when decrypting with padded cipher, pubnubError=PubNubError(name=CRYPTO_ERROR, code=135, message='Error while encrypting/decrypting message. Please contact support with error details.'), jso=null, statusCode=0, affectedCall=null)

For this message: “this is not encrypted message”
This error is thrown:
java.lang.IllegalArgumentException: bad base-64

For user they are handled the same way. The thing is that initially we didn't handle last example properly because this is not PubNub exception but java.lang.IllegalArgumentException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there is currently lack of my knowladge how to prepare good test cases in Go. I wanted to have any test if the function behaves as desired.

Right now I'm trying to catch any base64 error that means it couldn't decode that - what indicates that message is not encrypted or encrypted in other way than using our SDKs.

subscription_manager.go Outdated Show resolved Hide resolved
@Xavrax
Copy link
Contributor Author

Xavrax commented Nov 27, 2023

@pubnub-release-bot release

@Xavrax Xavrax merged commit f5c19c1 into master Nov 27, 2023
8 checks passed
@Xavrax Xavrax deleted the fix/crypto branch November 27, 2023 11:56
@pubnub-release-bot
Copy link
Contributor

🚀 Release successfully completed 🚀

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