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.

Sign up for GitHub

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

Jump to bottom

Quarkus REST Jackson: Improve detection of generic fields annotated with the @SecureField and allow to explicitly enable secure serialization #44669

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Quarkus REST Jackson: Improve detection of generic fields annotated with the @SecureField and allow to explicitly enable secure serialization #44669

merged 1 commit into from
Nov 25, 2024

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Nov 23, 2024 •
edited
Loading

  • closes: Failing to apply @SecureField to a generic with nested objects. #44646
  • fixes reproducer that requires to resolve type params to actual types
    • this is done only for the endpoint return type (AKA once, it shouldn't have impact on time required to detect secure field)
  • however this won't work for more complex scenarios and personally I think it was never aim; for that reason, this PR propose to always include SecurityCustomSerialization when secure serialization is explicitly enabled with the EnableSecureSerialization annotation, which gives users means to use @SecureField for non-trivial scenarios
  • docs is improved to so that users understand secure serialization is enabled implicitly on the best-effort bases and they may need to enable it explicitly

@michalvavrik michalvavrik requested a review from geoand November 23, 2024 21:45
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 23, 2024 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@michalvavrik michalvavrik force-pushed the feature/improve-secure-field-detection branch from e86c7f3 to e8fd90a Compare November 24, 2024 11:08
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 24, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit e8fd90a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 24, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit e8fd90a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

geoand
geoand approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks a lot @michalvavrik!

@geoand geoand merged commit fd27f9a into quarkusio:main Nov 25, 2024
35 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 25, 2024
@suchwerk
Copy link
Contributor

Thanks @michalvavrik for the quick fix / enhancement!

@michalvavrik michalvavrik deleted the feature/improve-secure-field-detection branch November 25, 2024 08:10
FroMage
FroMage reviewed Nov 25, 2024
View reviewed changes
...quarkus/resteasy/reactive/jackson/deployment/processor/ResteasyReactiveJacksonProcessor.java
return currentClassInfo
.fields()
.stream()
.filter(fieldInfo -> !fieldInfo.hasAnnotation(JSON_IGNORE))
.map(FieldInfo::type)
.map(fieldType -> {
Copy link
Member

Choose a reason for hiding this comment

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

We have better type mapping functions already in place that do better than just handling simple cases like the field type being a type variable. Probably this could be improved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific? Both in scenarios you have in mind and where these mapping functions are already in place, because I don't know what you mean.

Also, I didn't want to do better because last time I improved this detection there was a complain that it prolongs build-time execution. So if your proposal won't have negative effect, I'm happy to apply it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Few hints should do, I just want to better understand what do you mean and make sure it wouldn't lead to increased build-time execution.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of the logic we already do to resolve type parameters in resteasy reactive. I think via JandexUtil and TypeMapper, but I can only find usages of them to obtain method parameter or return type signatures or the list of type parameters. I can't find anywhere where we resolve an entire Type with type parameters substituted. Probably I dreamed this :(

Copy link
Member Author

Choose a reason for hiding this comment

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

np, if you run into this or @geoand knows, please let me know. it's not on the top of my list tbh, so no hurry

Copy link
Contributor

@geoand geoand Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can't find anywhere where we resolve an entire Type with type parameters substituted. Probably I dreamed this

AFAIR, the cases we've it for are method parameters and method return types

@gsmet gsmet modified the milestones: 3.18 - main, 3.17.1 Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@suchwerk suchwerk suchwerk left review comments

@FroMage FroMage FroMage left review comments

Assignees
No one assigned
Labels
area/documentation area/rest area/security kind/bugfix
Projects
None yet
Milestone
3.17.2
Development

Successfully merging this pull request may close these issues.

Failing to apply @SecureField to a generic with nested objects.
5 participants
@michalvavrik @suchwerk @FroMage @geoand @gsmet