Skip to content
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

Remove fromJust usage in URI.Extra.QueryPairs #68

Open
jbrechtel opened this issue Feb 4, 2022 · 2 comments
Open

Remove fromJust usage in URI.Extra.QueryPairs #68

jbrechtel opened this issue Feb 4, 2022 · 2 comments
Labels
status: accepted This issue is now ready to be implemented via a PR. type: breaking change A change that requires a major version bump. type: bug Something that should function correctly isn't.

Comments

@jbrechtel
Copy link

Is your change request related to a problem? Please describe.
We ran into a crashing bug in our production front-end where using print from URI.Extra.QueryPairs would result in an error which we traced down to fromJust against encodeURIComponent's result.

Examples:

Both keyToString and valueToString in URI.Extra.QueryPairs will error if their usage of encodeURIComponent results in a Nothing. This can happen if unprintable characters. For example a Dooli\U001001a9leinside of a string given toencodeURIComponentresults inNothing`.

I'm not sure exactly what in the string is causing encodeURIComponent to return a Nothing there.

Describe the solution you'd like
Broadly it'd be nice if usages of fromJust here were eliminated entirely. The Maybeness of the entire API could be represented in its external interface.

Alternatively the API could be split into safe and unsafe versions. This is a little tricky since there's already some "unsafe" functions here which are labeled and documented as such for different kinds of safety reasons.

I'm happy to work on a PR but would wanted to get some insight into the appetite for the various solutions to this.

Additional context
In the meantime we're going to mitigate this issue on our end locally by testing our input to the problematic functions against encodeURIComponent first - and possibly by additional input filtering elsewhere in our app.

@thomashoneyman thomashoneyman added status: accepted This issue is now ready to be implemented via a PR. type: breaking change A change that requires a major version bump. type: bug Something that should function correctly isn't. labels Feb 4, 2022
@thomashoneyman
Copy link
Contributor

I think there is appetite for this -- this library is meant to be safe. There are a few different things we can do, some of which are in this repository and some of which are upstream in js-uri, which provides the encode / decode functions.

Upstream, we can start by adding failing tests that demonstrate cases where encodeURIComponent and decodeURIComponent return Nothing:

https://github.com/purescript-contrib/purescript-js-uri/blob/b809ef6af53b880e0682bba8a9291f855607d2f4/test/Main.purs#L10-L18

For example, we could pull tests from the examples on MDN of when a URIError gets thrown. It's easier to think of strings that will fail decoding rather than encoding, but it's possible for example when encoding an unmatched surrogate pair, or in your example of an unprintable character.

Second, it may be possible to write some sanitizing code so that you can safely use encodeURIComponent and decodeURIComponent. We would need property tests via quickcheck to help prove that we're effectively sanitizing the input string before encoding. These safe versions could be written and used in this library, or they could possibly be added upstream to js-uri.

Finally, if that's not possible, then I'm happy to consider ways to adjust this package so that it never crashes in the case where encoding or decoding results in Nothing.

@ysangkok
Copy link
Contributor

ysangkok commented Feb 4, 2022

Here is the error I think we hit:

module Main where                                                                   
                                                                                      
import URI.Extra.QueryPairs (valueFromString)                                       
x = valueFromString "\xdc00"

Which crashes with

Error: Failed pattern match at Data.Maybe (line 281, column 1 - line 281, column 46): Nothing
    at /tmp/psctest/output/Data.Maybe/index.js:151:15
    at /tmp/psctest/output/URI.Common/index.js:60:24
    at /tmp/psctest/output/Data.Either/index.js:85:34
    at /tmp/psctest/output/Control.Monad.State.Trans/index.js:72:53
    at /tmp/psctest/output/Data.Identity/index.js:46:20
    at /tmp/psctest/output/Control.Monad.State.Trans/index.js:73:23
    at /tmp/psctest/output/Text.Parsing.Parser/index.js:216:137
    at /tmp/psctest/output/Data.Identity/index.js:112:20
    at /tmp/psctest/output/Text.Parsing.Parser/index.js:214:210
    at /tmp/psctest/output/Control.Monad.State.Trans/index.js:73:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted This issue is now ready to be implemented via a PR. type: breaking change A change that requires a major version bump. type: bug Something that should function correctly isn't.
Development

No branches or pull requests

3 participants