Repository navigation
Conversation
prosemirror-go does not fully validate the steps sent by clients. A replaceAround step whose insert is outside its slice panics with an index out of range in model.replace, so PATCH /notes/:id answered with a 500. A panic while applying a step is now turned into a failed step: the client gets ErrCannotApply (409) and the note is not modified. Fixes #5130
This branch has not been deployed
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.
Fixes #5130
Problem
prosemirror-go does not fully validate the steps sent by clients. A
replaceAroundstep whoseinsertis outside its slice (e.g.insert: -1withopenStart: 1) panics with an index out of range inmodel.replace, soPATCH /notes/:idanswers with a 500. Any client that can edit a note can send it, including the recipient of a shared note.Solution
applynow calls each step throughapplyStep, which turns a panic into a failed step. The client getsErrCannotApply(409) and the note is not modified, as for any step that cannot be applied. The notes lock is still released by itsdefer.Validating the steps in prosemirror-go itself would be a larger change in another repository; the recover also guards against the other unchecked indexes of the library.
StepFromJSONwas probed with malformed steps (wrong types, missing fields, unknown step types): it returns errors and does not panic, so it is left as is.The Markdown import panic of the same fuzzing campaign (#5129) is fixed by #5152; this PR does not conflict with it.
Test plan
TestApplyStepThatPanicsInProsemirror: the step from Notes: PATCH /notes/:id with a replaceAround step whose insert is out of the slice panics in prosemirror-go (index out of range, 500) #5130 returnsErrCannotApplyand the version is unchanged (panics withindex out of range [9] with length 9without the fix)go test ./model/note/passes, alone and merged with fix(note): return an error for table markers outside a table #5152