mozilla / fxa-email-service

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
6 stars 6 forks source link

"Error: Couldn't parse JSON body" on email address containing single apostrophe #273

Closed jrgm closed 5 years ago

jrgm commented 5 years ago
{"Logger":"fxa_email_service-1.127.0","Type":"fxa_email_service:log","Pid":1,"Severity":6,"Timestamp":1549900230109276749,"Field":
{"msg":"Request started","remoteAddressChain":"127.0.0.1","path":"/send","method":"POST"}}
=> Error: Couldn't parse JSON body: 
Error("invalid value: string \"danny.o\\\'shea@example.com\", expected email address", line: 1, column: 49)
=> Outcome: Success
{"Logger":"fxa_email_service1.127.0","Type":"fxa_email_service:log","Pid":1,"Severity":3,
"Timestamp":1549900230117310610,"Fields":{"msg":"Request errored","code":400,"error":
"Bad Request","errno":102,"message":"Invalid payload: invalid value: string \"danny.o\\'shea@example.com\", 
expected email address at line 1 column 49","additional_fields":"{}"}}

Where that email address is completely made up, but has a similar form to the real one that triggered this. Seemed to have passed upstream parses of that address (in auth and frontend) to get to this point.

philbooth commented 5 years ago

This is caused by the exact same problem as mozilla/fxa-content-server#6702. We have a regex that contains RIGHT SINGLE QUOTATION MARK (U+2019) instead of an apostrophe:

https://github.com/mozilla/fxa-email-service/blob/8527be4de3b87d6c195345ca7d58663be3a6c3d8/src/types/validate/mod.rs#L29

Probably helpfully put there by someone's text editor, I presume. I'll fix this in the current train.

shane-tomlinson commented 5 years ago

I'll fix this in the current train.

Train-130 goes out today, is that the target?

shane-tomlinson commented 5 years ago

Probably helpfully put there by someone's text editor, I presume. I'll fix this in the current train.

I wonder if we could have some lint rule to avoid having those changed.

philbooth commented 5 years ago

Train-130 goes out today, is that the target?

Oh good point, yeah, I'll cut a 130 tag if the PR gets merged.

I wonder if we could have some lint rule to avoid having those changed.

Well the PR has tests, which are even better than a lint rule I think!