Fix: avoid embedding the user input into the error response. #2207

CNLHC · 2022-03-04T02:48:50Z

Pull Request

What does this PR do?

The problem is meilisearch embeds the user input to the error message.

The reason for this problem is milli throws a serde_json: Error whose Display implementation will do this embedding.

I tried to solve this problem in this PR by manually implementing the Display trait for DocumentFormatError instead of deriving automatically.

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Kerollmops

Thank you very much for your PR!
Apart from some esthetic changes, it's good for me.

meilisearch-lib/src/document_formats.rs

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Kerollmops

I had some thoughts about your PR and think it removes too much information about the error, users will be lost if an error occurs and only the line and column are specified.

What do you think about ellipsing the message? By using something like 50 chars at the start and 50 chars at the end:

The `json` payload provided is malformed. `Couldn't serialize document value: invalid type: string \"my very ver...very very long body here\", expected a documents, or a sequence of documents. at line 1 column 7`.

meilisearch-lib/src/document_formats.rs

curquiza · 2022-03-24T15:33:45Z

Hello @CNLHC, thanks again for starting this PR. Do you need any precision or help regarding the @Kerollmops review? 🙂 The whole core team is available to help you do your contribution.

Also, we would totally understand you don't have the time to apply the changes. In this case, let me know so that we can let someone else work on it and integrate the bug fix to the next release 🙂

Hope everything is doing well on your side!

CNLHC · 2022-03-24T21:07:39Z

Sorry for the late response. I will submit a patch to follow the suggestion of @Kerollmops today.

…change_json_error_message

Co-authored-by: Clément Renault <renault.cle@gmail.com>

curquiza · 2022-03-28T13:42:02Z

@CNLHC thanks a lot for these changes 🙏
Looks like there is a tiny issue to fix with our clippy CI before Kero reviews it :)

Kerollmops

Thank you very much for your contribution ❤️
However, could you modify the tests according to the changes I proposed with the ellipsing algorithm? And could you stash everything into one final commit when we are done, please?

meilisearch-http/tests/documents/add_documents.rs

Kerollmops · 2022-03-29T15:53:19Z

meilisearch-lib/src/document_formats.rs

+                    let mut serde_msg = se.to_string();
+                    let prefix = r#"invalid type: string ""#;
+                    if serde_msg.starts_with(prefix) {
+                        let start_idx = prefix.len();
+                        if let Some(end_idx) = serde_msg.rfind("\"") {
+                            if end_idx - start_idx > 100 {
+                                serde_msg.replace_range(start_idx + 50..end_idx - 50, " ... ");
+                            }
+                        } else {
+                            serde_msg = String::from("");
+                        }
+                    }
+
+                    write!(
+                    f,
+                    "The `{}` payload provided is malformed. `Couldn't serialize document value: {}`.",
+                    b,serde_msg
+                )


I think ze can simplify the algorithm by directly ellipsing the whole message. Also matching on the invalid type: string " isn't a good idea, it could happen with objects too. What do you think? Could you modify the tests accordingly, please?

Suggested change

let mut serde_msg = se.to_string();

let prefix = r#"invalid type: string ""#;

if serde_msg.starts_with(prefix) {

let start_idx = prefix.len();

if let Some(end_idx) = serde_msg.rfind("\"") {

if end_idx - start_idx > 100 {

serde_msg.replace_range(start_idx + 50..end_idx - 50, " ... ");

}

} else {

serde_msg = String::from("");

}

}

write!(

f,

"The `{}` payload provided is malformed. `Couldn't serialize document value: {}`.",

b,serde_msg

)

let mut serde_msg = se.to_string();

let ellipsis = "...";

if serde_msg.len() > 100 + ellipsis.len() {

serde_msg.replace_range(50..serde_msg.len() - 50, ellipsis);

}

write!(

f,

"The `{}` payload provided is malformed. `Couldn't serialize document value: {}`.",

b, serde_msg

)

In some cases, this method will lead to weird error messages. For example, if the raw message is

The `json` payload provided is malformed. `Couldn't serialize document value: invalid type: string "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789", expected a documents, or a sequence of documents. at line 1 column 102`.

Using your method, ellipsing the string without parsing the context will generate

The `json` payload provided is malformed. `Couldn't serialize document value: invalid type: string \"0123456789012345678901234567..., or a sequence of documents. at line 1 column 102`.

Some meaningful information(The expected a documents) is truncated.

The meaningful suffix length cannot be determined. But in most cases, its length is around 75.

", expected a documents, or a sequence of documents. at line <LINE_NUMBER> column <COLUMN_NUMBER>

So I suggest to change 50..serde_msg.len() - 50 to 50..serde_msg.len() - 85

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Kerollmops

Thank you, that's better and clever. Could you remove the dbg! in the test and I'll merge after that, please?
Once you have done so, could you squash everything in one commit and rebase on main, please?

meilisearch-http/tests/documents/add_documents.rs

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Kerollmops

Thank you very much for your work @CNLHC, merging now!
bors merge

bors · 2022-04-04T17:50:11Z

Build succeeded:

…between 100 and 135

2727: Don't panic when the error length is slightly over 100 r=Kerollmops a=onyxcherry # Pull Request ## What does this PR do? Fixes PR #2207 as [the last commit](7ece7a9) has changed number of the characters at the end to leave in place from `50` to `85` **but the lower limit of a string length wasn't changed**. Therefore, any data (e.g. example string from issue #2680) was causing `meilisearch` to **panic**. So I simply raised the minimum value from `100` to `135` (`50 + 85`) to ensure that `replace_range()` won't panic due to an inverted range. At the same time I am in favor of the `85` value which was changed in the `@CNLHC's` last commit. ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing ~issue~ pull request? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Tomasz Wiśniewski <tomasz@wisniewski.app>

CNLHC added 3 commits March 4, 2022 10:46

first try

58e2903

show detailed error message

b138b92

fix broken test

a356c83

CNLHC changed the title ~~Fix #2107~~ Fix #2107: avoid embedding the whole document in the error response. Mar 4, 2022

CNLHC changed the title ~~Fix #2107: avoid embedding the whole document in the error response.~~ Fix #2107: avoid embedding the user input into the error response. Mar 4, 2022

CNLHC marked this pull request as ready for review March 4, 2022 07:50

curquiza requested review from irevoire and Kerollmops March 7, 2022 09:28

Kerollmops requested changes Mar 7, 2022

View reviewed changes

meilisearch-lib/src/document_formats.rs Outdated Show resolved Hide resolved

irevoire added this to the v0.27.0 milestone Mar 7, 2022

LiuHanCheng and others added 2 commits March 8, 2022 12:03

Update meilisearch-lib/src/document_formats.rs

c8895ca

Co-authored-by: Clément Renault <renault.cle@gmail.com>

fix test

35bf7ee

Kerollmops requested changes Mar 8, 2022

View reviewed changes

meilisearch-lib/src/document_formats.rs Show resolved Hide resolved

meilisearch-lib/src/document_formats.rs Show resolved Hide resolved

CNLHC added 4 commits March 25, 2022 19:53

Merge branch 'main' of github.com:meilisearch/meilisearch into CNLHC/…

193c666

…change_json_error_message

add truncate logic

ce85981

fix test and add truncate test.

3c72f4d

add some comments

ce12000

CNLHC requested a review from Kerollmops March 25, 2022 13:34

CNLHC and others added 3 commits March 25, 2022 21:36

tidy

c7b489f

Update meilisearch-lib/src/document_formats.rs

80d8ac4

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Update meilisearch-lib/src/document_formats.rs

13a0e78

Co-authored-by: Clément Renault <renault.cle@gmail.com>

curquiza changed the title ~~Fix #2107: avoid embedding the user input into the error response.~~ Fix: avoid embedding the user input into the error response. Mar 28, 2022

Kerollmops requested changes Mar 29, 2022

View reviewed changes

LiuHanCheng and others added 3 commits March 31, 2022 10:14

Update meilisearch-lib/src/document_formats.rs

b28aa8e

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Update meilisearch-http/tests/documents/add_documents.rs

403f03c

Co-authored-by: Clément Renault <renault.cle@gmail.com>

change truncate strategy and coresponding test

7ece7a9

CNLHC requested a review from Kerollmops March 31, 2022 05:49

Kerollmops requested changes Mar 31, 2022

View reviewed changes

meilisearch-http/tests/documents/add_documents.rs Outdated Show resolved Hide resolved

meilisearch-http/tests/documents/add_documents.rs Outdated Show resolved Hide resolved

LiuHanCheng and others added 2 commits April 1, 2022 09:30

Update meilisearch-http/tests/documents/add_documents.rs

eee2cd5

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Update meilisearch-http/tests/documents/add_documents.rs

6fc6b83

Co-authored-by: Clément Renault <renault.cle@gmail.com>

Kerollmops approved these changes Apr 4, 2022

View reviewed changes

bors bot merged commit 9e344f6 into meilisearch:main Apr 4, 2022

curquiza added the v0.27.0 PRs/issues solved in v0.27.0 label Aug 24, 2022

onyxcherry pushed a commit to onyxcherry/meilisearch that referenced this pull request Aug 31, 2022

Fix meilisearch#2207 - do not panic when the error message length is …

60792ee

…between 100 and 135

onyxcherry mentioned this pull request Aug 31, 2022

Don't panic when the error length is slightly over 100 #2727

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: avoid embedding the user input into the error response. #2207

Fix: avoid embedding the user input into the error response. #2207

CNLHC commented Mar 4, 2022 •

edited

Kerollmops left a comment

Kerollmops left a comment

curquiza commented Mar 24, 2022

CNLHC commented Mar 24, 2022

curquiza commented Mar 28, 2022

Kerollmops left a comment

Kerollmops Mar 29, 2022

CNLHC Mar 31, 2022

CNLHC Mar 31, 2022 •

edited

Kerollmops left a comment •

edited

Kerollmops left a comment

bors bot commented Apr 4, 2022

Fix: avoid embedding the user input into the error response. #2207

Fix: avoid embedding the user input into the error response. #2207

Conversation

CNLHC commented Mar 4, 2022 • edited

Pull Request

What does this PR do?

PR checklist

Kerollmops left a comment

Choose a reason for hiding this comment

Kerollmops left a comment

Choose a reason for hiding this comment

curquiza commented Mar 24, 2022

CNLHC commented Mar 24, 2022

curquiza commented Mar 28, 2022

Kerollmops left a comment

Choose a reason for hiding this comment

Kerollmops Mar 29, 2022

Choose a reason for hiding this comment

CNLHC Mar 31, 2022

Choose a reason for hiding this comment

CNLHC Mar 31, 2022 • edited

Choose a reason for hiding this comment

Kerollmops left a comment • edited

Choose a reason for hiding this comment

Kerollmops left a comment

Choose a reason for hiding this comment

bors bot commented Apr 4, 2022

CNLHC commented Mar 4, 2022 •

edited

CNLHC Mar 31, 2022 •

edited

Kerollmops left a comment •

edited