Merged
Conversation
xavierpinho
approved these changes
Jul 8, 2025
Contributor
xavierpinho
left a comment
There was a problem hiding this comment.
Thanks for the prompt discussion and clarifications!
Just a typo (double negation) I spotted:
nor should (not/it) cause
mpollmeier
approved these changes
Jul 8, 2025
README.md
Outdated
|
|
||
| If you need to handle untrusted `.fg` files, then we recommend some form of sandboxing in order to limit the DoS impact. | ||
|
|
||
| If you do decide against our recommendation to write your own code to "sanity check" potentially malicious `.fg` files before attempting to deserialize them, then we'd be happy for your feedback and PRs. (also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests) |
Contributor
There was a problem hiding this comment.
Suggested change
| If you do decide against our recommendation to write your own code to "sanity check" potentially malicious `.fg` files before attempting to deserialize them, then we'd be happy for your feedback and PRs. (also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests) | |
| You should always sanity check potentially malicious `.fg` files before attempting to deserialize them. If there's anything we can add in flatgraph's Deserializer, we'd be happy for your feedback and PRs. | |
| N.b. also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests. |
Contributor
Author
There was a problem hiding this comment.
No. Our recommendation is to not bother with sanity checks, and do sandboxing instead.
Contributor
There was a problem hiding this comment.
Your initial text says "our recommendation to write your own code to "sanity check" potentially malicious .fg files", which I interpret as 'we do recommend to write code to sanity check.
But yes, fair enough, will provide a new suggestion based on the current version.
Contributor
There was a problem hiding this comment.
ah, you already updated that part - thanks!
Co-authored-by: Michael Pollmeier <michael@michaelpollmeier.com>
Co-authored-by: Michael Pollmeier <michael@michaelpollmeier.com>
Co-authored-by: Michael Pollmeier <michael@michaelpollmeier.com>
Co-authored-by: Michael Pollmeier <michael@michaelpollmeier.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds some clarifications regarding security goals. Big thanks to https://github.com/xavierpinho ❤️ for mentioning that here https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh
Any improvement / rewording ideas?