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

DateTime mapping is not very intuitive while it is a common type #240

Open
GregOriol opened this issue Sep 16, 2024 · 3 comments
Open

DateTime mapping is not very intuitive while it is a common type #240

GregOriol opened this issue Sep 16, 2024 · 3 comments

Comments

@GregOriol
Copy link

Hi,

In the current state, the mapping of DateTime type is not very intuitive: it's a common json/php type but it's not clear how to make use of it in the mapping.

The documentation for example has the "Simple type mapping" paragraph, but says "disabled by default for security reasons" => so if we should not use this boolean for security reasons to map DateTime, what should we use instead?

The next documentation paragraphe "Class map" also has an example with DateTime in it, but it doesn't work to replace the boolean, $mapper->classMap['DateTime'] = function ($class, $jvalue) { is not taken into account when the boolean is true.

Note that my type mapping on the class is correct and detected, it works with the boolean set back to false.

Any ideas on this?

@SvenRtbg
Copy link
Contributor

My perspective is that the security aspect is targeting the general case where an arbitrary value is passed as the first and only parameter into an object. As usual, a dumb implementation will avoid any and all validation inside the constructor, fully trusting the value to be valid, which may lead to all imaginable issues.

In case of DateTime, I would believe there is quite some parsing logic in place within PHP that should be able to handle evil strings, however it depends on the use case and expectation of the application. You can pass almost any string into DateTime and still get back a valid result, but not all strings would be considered expected (i.e. you can pass an ISO datetime string, or a string resembling a unix timestamp, or "+1month", or "now", and may get a DateTime object created).

So obviously, a validation step seems to be applicable, either in the form of a schema validation using an additional library, or some code of your own that is used to create a better validated DateTime object.

For example, you could create a call to DateTime::createFromFormat, allowing to further validate that an arbitrary string isn't leading to an unexpected DateTime object.

In terms of using DateTime as an example in the docs: That definitely is a legacy of prior versions, as this is the obvious example for an object where a scalar value is passed as the constructor argument that everyone can look up and understand.

Can you illustrate what does or doesn't work with a bit more code beyond a reference to a partial excerpt from the docs?

@GregOriol
Copy link
Author

I don't understand what you are talking about exactly...

What I wanted to point out is that DateTime, while a common json data, isn't mapped if one is following the documentation's recommandation of setting the "Simple type mapping" bStrictObjectTypeChecking boolean to true (which is the default). And there is no alternative provided in the docs.

So the user is left with either setting the boolean back to false as was previously, with the added security risk as stated by the documentation (which is worrisome to a user not knowing the exact impacts of this security information), or what else? the documentation doesn't seem to describe another way of having DateTime work...

@cweiske
Copy link
Owner

cweiske commented Sep 22, 2024

The "exact impacts" are documented in the README in the "Simple types instead of objects" section: https://github.com/cweiske/jsonmapper?tab=readme-ov-file#simple-types-instead-of-objects

I'll happily accept a patch that adds a whitelist of classes allowed for automatic constructor mapping, even if bStrictObjectTypeChecking is disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants