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

Sanitize dictionaries before serialization #82

Merged
merged 8 commits into from
Nov 2, 2023

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Oct 25, 2023

What does this PR do?

Sanitize dictionaries before serialization

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

I've tried multiple approaches for proper sanitization but all of them have edge cases or get overly complex.
Similar thoughts as here

// NOTE: Ideally we would use the NSCoding behaviour but it gets needlessly complex
// given we only need this for sending to the API

Codable types only would be awesome but the interoperability with ObjC is suboptimal.

Using Generics would be ideal as well but again bad interoperability with ObjC.
I guess we can improve this in the future if we see it but I'd not write tons of code right now, the older version relies onJSONSerialization as well.

I checked https://github.com/SwiftyJSON/SwiftyJSON/blob/master/Source/SwiftyJSON/SwiftyJSON.swift
It just fails if there's a nonserializable type.
Also, https://swiftunboxed.com/stdlib/json-decoder-decodable/ but generics are not well played.
Tried to implement my own version of AnyCodable as well but entered the rabbit whole of https://github.com/asensei/AnyCodable/tree/master

We came to the conclusion that there are too many edge cases, and we have since then completely moved away from fuzzy decoding of Any type.

JSONSerialization.isValidJSONObject always requires an Object or Array, so I cannot just use it for each value of the dict.

What are the relevant tickets?

Screenshots or screencasts (if UI/UX change)

Questions:

  • Does the docs need an update?
  • Are there any security concerns?
  • Do we need to update engineering / success?

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Some thoughts on how we should best handle this, thinking ahead to what the feedback will be if someone gets into this situation.

PostHog/PostHogSDK.swift Show resolved Hide resolved
PostHog/PostHogSDK.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

This looks great functionally but feels like something that 100% needs tests

@marandaneto marandaneto merged commit 67c923e into v3.0.0 Nov 2, 2023
1 of 2 checks passed
@marandaneto marandaneto deleted the chore/sanitize-dictionary branch November 2, 2023 09:15
@marandaneto marandaneto mentioned this pull request Nov 2, 2023
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