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

Authorization header parsing #4799

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

@tommasodotNET
Copy link

@tommasodotNET tommasodotNET commented Dec 15, 2020

Changes

Issues
Adresses #4739

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Dec 18, 2020

If all you are trying to do is to parse a comma separated string regardless of if it's quoted or not, then you don't need to create a list and append each character to it.

You can just access the index of the string and use a range to copy the substring - something like the code below.

Please note - that this isn't compilable code - just an alternative that doesn't need the List.

        public static string[] GetParts(string quotedString)
        {
            var result = new List<string>();
            var escapeChars = new[] { '"', ',' };
            var escaped = false;
            int start = 0;
            int i = 0;
            while (i < quotedString.Length)
            {
                var token = quotedString[i];
                if (token == '"' || token == ',')
                {
                    start = i + 1;
                }
                else
                {
                    // Applying a XOR logic to evaluate whether it is opening or closing a value
                    escaped = (!escaped) == (token == '"');
                    if (token == ',' && !escaped)
                    {
                        // Meeting a comma after a closing escape char means the value is complete
                        if (start-i > 1)
                        {
                            result.Add(WebUtility.UrlDecode(quotedString[start..(i - 1)]));
                        }

                        start = i + 1;
                    }
                }

                i++;
            }

            // Add last value
            if (start < i)
            {
                result.Add(WebUtility.UrlDecode(quotedString[start..(i - 1)]));
            }

            return result.ToArray();
        }
    }
    ```
@tommasodotNET
Copy link
Author

@tommasodotNET tommasodotNET commented Jan 2, 2021

Nice idea @BaronGreenback, thank you! I've pushed a new version of the method.

@dkanada dkanada requested a review from BaronGreenback Feb 9, 2021
@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Feb 26, 2021

@tommasodotNET: Apologies for the delay in checking the PR.

hope you don't mind, but I've made a suggestion of optimization by getting the GetParts to parse both the key / values. (The commit can be rolled back if you don't like - it was just easier in VS2019 to do it this way)

Based on the code, I've assumed the input format is key=value, key=value. Is this format correct?

I've also added a test method for the function, so it can be tested outside of JF, which should help in getting the PR accepted.
I've included some tests based on the original bug report - are these correct, and are there any more that it would be worth adding.

@tommasodotNET
Copy link
Author

@tommasodotNET tommasodotNET commented Feb 26, 2021

Hi @BaronGreenback.
No problem! Thank you for your help.
Yes the correct input was key=value, key=value. Also, we had to consider the possibility of key="val,ue" if I recall right.

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Feb 26, 2021

I've included the following tests

[InlineData("x=\"123,123\",y=\"123\"", "x", "123,123")]
[InlineData("x=\"ab\"", "x", "ab")]
[InlineData("param=Hörbücher", "param", "Hörbücher")]
[InlineData("param=%22%Hörbücher", "param", "\"%Hörbücher")]

the 1st param is the input string, the 2nd param the key, and the 3rd the value
so param=%22%Hörbücher should parse as a key of param and a value of "%Hörbücher

Is there anymore conditions that need adding?

@tommasodotNET
Copy link
Author

@tommasodotNET tommasodotNET commented Feb 26, 2021

No this should be fine, thanks!

@BaronGreenback BaronGreenback requested a review from crobibero Feb 26, 2021
@tommasodotNET
Copy link
Author

@tommasodotNET tommasodotNET commented Jun 3, 2021

Any hint on why some tests are failing? The tests related to the authorization header look fine...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants