Skip to content

Check for trailing content when parsing JSON#5048

Closed
findepi wants to merge 1 commit into
apache:masterfrom
findepi:findepi/reject-trailing-json-garbage
Closed

Check for trailing content when parsing JSON#5048
findepi wants to merge 1 commit into
apache:masterfrom
findepi:findepi/reject-trailing-json-garbage

Conversation

@findepi

@findepi findepi commented Jun 15, 2022

Copy link
Copy Markdown
Member

Before the change, various JSON parsing routines would ignore content
after a } matching to the opening { is found. This commit changes
the common parsing pattern to use a newly introduced utility method
which checks for trailing payload and fails if any is found.

Follows #4537 (comment)

@findepi

findepi commented Jun 15, 2022

Copy link
Copy Markdown
Member Author

Currently, based on #4537, since it fixes also code being added there.

@nastra nastra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one question

Comment thread core/src/test/java/org/apache/iceberg/TestSnapshotRefParser.java
@findepi findepi force-pushed the findepi/reject-trailing-json-garbage branch from 314e6fb to 3661c85 Compare June 15, 2022 13:49
@findepi findepi force-pushed the findepi/reject-trailing-json-garbage branch from 3661c85 to 3e684b7 Compare June 23, 2022 12:15
Before the change, various JSON parsing routines would ignore content
after a `}` matching to the opening `{` is found. This commit changes
the common parsing pattern to use a newly introduced utility method
which checks for trailing payload and fails if any is found.
@findepi findepi force-pushed the findepi/reject-trailing-json-garbage branch from 3e684b7 to b40c777 Compare June 23, 2022 12:57
@findepi

findepi commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

I think I don't know how to fix the iceberg-mr classpath to include jackson databind, with the right version.

@github-actions github-actions Bot added the MR label Jun 23, 2022
@rdblue

rdblue commented Jun 28, 2022

Copy link
Copy Markdown
Contributor

@findepi, what problem is this fixing? Did you see this in practice with a corrupt file?

@findepi

findepi commented Jun 28, 2022

Copy link
Copy Markdown
Member Author

@rdblue thank for spending time and reviewing my PR.

Did you see this in practice with a corrupt file?

I haven't.
Iceberg library doesn't write such invalid content.

i also don't see currently that this could be exploited for security reasons

what problem is this fixing?

we incorrectly accept an incorrect input which ought to be rejected

@findepi

findepi commented Jul 22, 2022

Copy link
Copy Markdown
Member Author

we have a bug in the library where we accept invalid JSON payloads (files) just because we stop parsing at some point.
i think this should be fixed.

@findepi

findepi commented Jul 29, 2022

Copy link
Copy Markdown
Member Author

@danielcweeks do you have opinion about this PR?

@findepi findepi closed this Aug 23, 2022
@findepi findepi deleted the findepi/reject-trailing-json-garbage branch August 23, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants