Don't return 500s for books containing non-ASCII fields.#1
Open
Julian wants to merge 2 commits intoPostman-Student-Program:mainfrom
Open
Don't return 500s for books containing non-ASCII fields.#1Julian wants to merge 2 commits intoPostman-Student-Program:mainfrom
Julian wants to merge 2 commits intoPostman-Student-Program:mainfrom
Conversation
Previously, trying to run e.g.:
http --json :4000/books title=שלום genre=bar author=baz yearPublished=2023 api-key:postmanrulz
i.e. POST to /books with:
```json
{
"title": "שלום",
"genre": "bar",
"author": "baz",
"yearPublished": 2023
}
```
would blow up with:
```
HTTP/1.1 500 Internal Server Error
Connection: keep-alive
Date: Wed, 07 Jun 2023 16:24:11 GMT
Keep-Alive: timeout=5
content-length: 107
content-type: application/json; charset=utf-8
{
"error": "Internal Server Error",
"message": "Cannot read properties of null (reading '0')",
"statusCode": 500
}
```
This error actually comes from the bad-words library during profanity
filtering, so it can actually be minimized to:
```
> var Filter = require('bad-words'),
... filter = new Filter();
> filter.clean("שלום עליכם");
Uncaught TypeError: Cannot read properties of null (reading '0')
at Filter.clean (/Users/julian/Development/library-api-v2/node_modules/.pnpm/bad-words@3.0.4/node_modules/bad-words/lib/badwords.js:58:41)
```
or even purely with JS regexes to:
```
/\b/.exec("שלום עליכם")
```
which just returns `null` because `\b` in JS doesn't cope with non-ASCII
word boundaries.
It seems slightly unlikely that `bad-words` would want some sort of
patch for this given it seems to only handle English anyhow (though
might file an issue anyhow in case it feels like at least doing nothing
in this case rather than blowing up).
But here, this patch makes us do the former, i.e. let through any
non-ASCII characters.
The test collection is also updated to make one of the fields actually
trigger this code path.
Julian
commented
Jun 7, 2023
| { | ||
| "key": "testBookAuthor", | ||
| "value": "bar", | ||
| "value": "פלוני אלמוני", |
Author
There was a problem hiding this comment.
(This says "John Doe" essentially FWIW)
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.
Previously, trying to run e.g.:
i.e. POST to /books with:
{ "title": "שלום", "genre": "bar", "author": "baz", "yearPublished": 2023 }would blow up with:
This error actually comes from the bad-words library during profanity
filtering, so it can actually be minimized to:
or even purely with JS regexes to:
which just returns
nullbecause\bin JS doesn't cope with non-ASCIIword boundaries.
It seems slightly unlikely that
bad-wordswould want some sort ofpatch for this given it seems to only handle English anyhow (though
might file an issue anyhow in case it feels like at least doing nothing
in this case rather than blowing up).
But here, this patch makes us do the former, i.e. let through any
non-ASCII characters.
The test collection is also updated to make one of the fields actually
trigger this code path.