-
Notifications
You must be signed in to change notification settings - Fork 96
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
Jackson databind vulnerability #824
Conversation
e85c3fc
to
270ac68
Compare
@@ -739,7 +739,7 @@ private Query getFilterQuery(Set<ApiFilter> filters) { | |||
} finally { | |||
lock.readLock().unlock(); | |||
} | |||
return new SinglePagePagination<>( | |||
return new SinglePagePagination<DimensionRow>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diamond operator seems to be fine (I see method signature already specified type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this and then accidentally reverted it.
@@ -43,7 +43,7 @@ public AllPagesPagination(Collection<T> entireCollection, PaginationParameters p | |||
this.collectionSize = entireCollection.size(); | |||
this.pageToFetch = paginationParameters.getPage(entireCollection.size()); | |||
this.countPerPage = paginationParameters.getPerPage(); | |||
this.lastPage = (collectionSize > countPerPage) ? (collectionSize - 1) / countPerPage + 1 : 1; | |||
this.lastPage = (collectionSize > countPerPage) ? (int) ((collectionSize - 1) / countPerPage + 1) : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make lastPage
long
as well. What happens if someone asks for a lot of dimension rows, with a page size of 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If last page is long, then pageToFetch must also be long. I will pull this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting to a error if we get greater than maxint since that doesn't seem to be compatible with our scoredoc collector that we're using.
* Fixed bad format in error message | ||
* Moved tests off of serialization of `SimplifiedIntervalList`. That's turning out to be hard to solve. | ||
|
||
- [Bump lucene version to patch vulnerability](https://github.com/yahoo/fili/issues/819) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should explicitly call out that this forces a breaking change on Pagination, since anybody who has their own search providers has to work with them. In particular that the number of results, and the last page are now (or should be) longs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting the breaking change in favor of an error on largeints and no interface changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github doesn't let me request changes without something in this goofy box, or adding my comments in their "review mode" that means nobody can see my comments until I'm done reviewing, which I don't like.
9b15fa5
to
28d5ece
Compare
@archolewa I've reverted all the interface impacting changes. |
String message = String.format(TOO_MANY_DOCUMENTS, hitDocs.totalHits); | ||
IllegalStateException illegalStateException = new IllegalStateException(message); | ||
LOG.error(illegalStateException.getMessage(), illegalStateException); | ||
throw illegalStateException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the same exception for when perPage
exceeds maxResults
: a RowLimitReachedException
? Seems to me like this is a similar kind of problem (we're getting back more results than we can handle). For reference: https://github.com/yahoo/fili/pull/824/files#diff-0e7dd2ec8a518d234151c865cd2b6dacR764
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the RowLimitReachedException
rather than an IllegalStateException
, since this is a case where we've reached a row limit. We might also want to include in the commit messages the rationale for why we're exploding rather than just making everything a long. I could see somebody crawling through the history a year from now to figure out why this particular quirk is what it is.
@archolewa Done |
Stabilized field ordering to deal with serialization changes from unconstrained orderings.
Resolved spring core security vulnerability patch Bumping lucene forced us to implicitly support long return values. However the Lucene search predicated don't all support long per-page values and some of our pagination implementations need to be able to handle all results in one page. Since we don't anticipate larger than Integer.MAX_VALUE documents from lucene, we're simply going to validate that the returning hit count is less than that and throw an exception if it isn't. (+1 squashed commit)
488fcbb
to
18be7ca
Compare
No description provided.