Skip to content

Fix invalid params value#97

Closed
Tom910 wants to merge 4 commits intosindresorhus:masterfrom
Tom910:fix-no-valid-value
Closed

Fix invalid params value#97
Tom910 wants to merge 4 commits intosindresorhus:masterfrom
Tom910:fix-no-valid-value

Conversation

@Tom910
Copy link
Copy Markdown

@Tom910 Tom910 commented May 13, 2017

Hey, parsing params ?utm_source=%% failed and generating the error

URIError: URI malformed
at decodeURIComponent ()
at /query-string/index.js:148:36

As a result, the server falls -_-

Copy link
Copy Markdown

@pavelpower pavelpower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Tom910
Copy link
Copy Markdown
Author

Tom910 commented May 15, 2017

// @sindresorhus

@sindresorhus
Copy link
Copy Markdown
Owner

I would prefer to follow the behavior of URLSearchParams:

x = new URLSearchParams('?utm_source=%%');
x.get('utm_source');
//=> "%%"

It just works there.

@Tom910
Copy link
Copy Markdown
Author

Tom910 commented May 15, 2017

@sindresorhus

this pr -
fn.parse('?utm_source=%%') // => { utm_source: '%%' }

index.js Outdated
}

str = str.trim().replace(/^(\?|#|&)/, '');
str = str.trim().replace(/^(\?|#|&|%)/, '');
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs to be removed. This replace is meant to remove ? from the start of a query string.

@sindresorhus
Copy link
Copy Markdown
Owner

The behavior needs to be documented, that if decoding fails it will use the original key/value.

@Tom910
Copy link
Copy Markdown
Author

Tom910 commented May 17, 2017

@sindresorhus fixed

@SamVerschueren
Copy link
Copy Markdown
Contributor

I have my thoughts about this method. For instance, what if we want to decode the following %%S%C3%B8rhus%%? It will end up on the server like that which is not what you expect.

So I thought about an alternative solution by building a custom decoder. The custom decoder is only used if the regular decoder is not able to decode it.

const regex = /(%[a-f0-9]{2})+/gi;
function customDecoder(input) {
    let match;
    
    while (match = regex.exec(input)) {
        try {
        	input = input.replace(new RegExp(match[0], 'g'), decodeURIComponent(match[0]));
        } catch (err) {
        	// Do nothing
        }
    }
    
    return input;
}

function decode(input) {
    try {
        return decodeURIComponent(input);
    } catch (err) {
        return customDecoder(input);
    }
}

Running decode('%%S%C3%B8rhus%%') will now end up with %%Sørhus%% which is what we expected.

There might still be edge cases in my solution so don't think this is "production" ready. I think it's just worth exploring.

@Tom910
Copy link
Copy Markdown
Author

Tom910 commented May 19, 2017

@SamVerschueren Yes, this case is a problem. I can take an implementation from https://github.com/nodejs/node/blob/master/lib/querystring.js

@Tom910
Copy link
Copy Markdown
Author

Tom910 commented May 19, 2017

And what is surprising, other libraries and polyfills do not support this behavior. Including https://github.com/ljharb/qs

@SamVerschueren
Copy link
Copy Markdown
Contributor

SamVerschueren commented May 19, 2017

I created a library with an improved version of my draft from above right here. It seems to work as expected with even more complex input. I didn't publish it yet as I'm awaiting some feedback first. The algorithm behind it might not be optimal (yet) and I believe it can be improved to make it faster. Want to explore some alternative approaches somewhere in the coming days. But most of the time the regular decodeURIComponent will work so I don't think someone will even notice a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants