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

Dissallow test/modification of some properties #33

Closed
AnderssonPeter opened this issue Oct 1, 2024 · 13 comments
Closed

Dissallow test/modification of some properties #33

AnderssonPeter opened this issue Oct 1, 2024 · 13 comments
Labels
enhancement New feature or request

Comments

@AnderssonPeter
Copy link
Contributor

Hi we have some Classes where we apply modifications using the Patch object, but there are some Properties we don't want to allow tests or changes.

They currently have a Attribute on them, is there someway we could throw a exception when SystemTextJsonPatch tries to access them?

@Havunen
Copy link
Owner

Havunen commented Oct 2, 2024

What about using ValidationAttribute?

@AnderssonPeter
Copy link
Contributor Author

@Havunen We could absolutly use ValidationAttribute, but where and how would we check if a property has the attibute when running ApplyTo?

@Havunen Havunen added the enhancement New feature or request label Oct 2, 2024
@Havunen
Copy link
Owner

Havunen commented Oct 2, 2024

This feature request #6 would combine modelstate validation and ApplyTo

until that is implemented, it needs to be done manually

@AnderssonPeter
Copy link
Contributor Author

After reading the PR's in the linked issue, i don't think that validation would suffice as i can't check if a value has changed using validation?

It seems to be more oriented to validating a value of a field, like An email must contain @ and a valid domain.

But in this case we want to deny the Json Patch access to some properties.

I can see 2 solutions to this issue:

  • To have some sort of Callback where you can return true or false after fetching a property, and if false throw an exception.
  • Add a custom Attribute, like DenyAccessToJsonPathAttribute (or some better name) and check if the property has that attribute, and then throw a exception, this check could be done in PropertyProxyCache.FindPropertyInfo

If the second option would be acceptable for you, then i could create a PR.

@Havunen
Copy link
Owner

Havunen commented Oct 3, 2024

One approach could be to have specific input model for the http patch routine where those properties are not allowed.

I started thinking that if we introduce DenyPatchAttribute is the next feature request to configure it based some logical operation?

Well maybe we could start with simple approach and see if it ever becomes a use case.

If you would like to see this implemented PR is welcome.

This could maybe go into next major version as I would like to refactor the exception type as well

@AnderssonPeter
Copy link
Contributor Author

@Havunen PR created :), One thing that might be nice but would require a bigger rewrite would be to include the full path in the exception that is thrown. But there is no need to do that for the first release in my opinion.

@AnderssonPeter
Copy link
Contributor Author

@Havunen Any idea when a new nuget can be released with this functionality? (We need it before we go into production)

@Havunen
Copy link
Owner

Havunen commented Oct 4, 2024

Soon

@Havunen
Copy link
Owner

Havunen commented Oct 5, 2024

Release 3.3.0 has been published https://github.com/Havunen/SystemTextJsonPatch/releases/tag/v3.3.0

@Havunen Havunen closed this as completed Oct 5, 2024
@AnderssonPeter
Copy link
Contributor Author

AnderssonPeter commented Oct 7, 2024

Thanks for the fast release, it works great :)

But i managed to find one way you actually can write to a property marked with DenyPatch, when the property is in the value field.
For our software this is fine, but it might be a security leak for someone else, maybe a line about it should be added to the readme?

Example:

[{"op":"add","path":"/safe/1","value":{"deny":"sneaky"}}]

In this case the deny property was marked with [DenyPath]

@Havunen
Copy link
Owner

Havunen commented Oct 8, 2024

Can you create separate issue for that please. It would be nice if you could patch that functionality

@AnderssonPeter
Copy link
Contributor Author

@Havunen To be honest i don't know how to fix this issue, my guess is that the SystemTextJsonPatch deserializes it using System.Text.Json, but i haven't found any way to block specific properties using it.

@Havunen
Copy link
Owner

Havunen commented Oct 9, 2024

Okay lets document it then, could you send the PR for the doc update please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants