Skip to content

Correct parsing of basic authentication to comply with RFC7617#477

Merged
vinoski merged 1 commit intoerlyaws:masterfrom
gnaeser:master
Nov 12, 2023
Merged

Correct parsing of basic authentication to comply with RFC7617#477
vinoski merged 1 commit intoerlyaws:masterfrom
gnaeser:master

Conversation

@gnaeser
Copy link

@gnaeser gnaeser commented Nov 7, 2023

According to RFC7617 the password consists of everything following the first colon, including any colons.

Using string:tokens/2 effectively removes any leading and or trailing colons, and compacts all multiple colons in the password into single ones: Auth = "foo:::bar:::frob::::" gives {"foo", "bar:frob"}, which obviously is incorrect. The PR changes this to returning {"foo", "::bar:::frob::::"}.

@vinoski
Copy link
Collaborator

vinoski commented Nov 8, 2023

Thanks, I'll have a look at this soon.

@vinoski vinoski self-assigned this Nov 8, 2023
@avtobiff
Copy link
Collaborator

avtobiff commented Nov 8, 2023

The basic_auth tests doesn't seem to work

2023-11-08 09:55:49.576

Error: {{assertMatch,
            [{module,auth_SUITE},
             {line,60},
             {expression,"yaws : parse_auth ( Auth0 )"},
             {pattern,"{ User0 , Password0 , Auth0 }"},
             {value,
                 {undefined,undefined,
                     {"Authorization",
                      "Basic Zm9vOjo6YmFyOjo6ZnJvYjo6Ojo="}}}]},
        [{auth_SUITE,basic_auth,1,[{file,"auth_SUITE.erl"},{line,60}]},
         {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
         {test_server,run_test_case_eval1,6,
             [{file,"test_server.erl"},{line,1291}]},
         {test_server,run_test_case_eval,9,
             [{file,"test_server.erl"},{line,1223}]}]}


			RESULT: Failed

@avtobiff
Copy link
Collaborator

avtobiff commented Nov 8, 2023 •
edited
Loading

Shouldn't an empty password be supported?
I.e. he following should return

yaws:parse_auth(auth_header("user","")).
{"user", undefined,{"Authorization","Basic dXNlcjo="}}

Currently this is returned

yaws:parse_auth(auth_header("user","")).
{undefined, undefined,{"Authorization","Basic dXNlcjo="}}

@gnaeser
Copy link
Author

gnaeser commented Nov 8, 2023 •
edited
Loading

My fault! The line Auth0 = auth_header(User0, Password0), in the test should be {_, Auth0} = auth_header(User0, Password0),

Tests work now.

@gnaeser
Copy link
Author

gnaeser commented Nov 8, 2023

Got an out of band comment that I should add more tests.

@vinoski
Copy link
Collaborator

vinoski commented Nov 11, 2023

Got an out of band comment that I should add more tests.

Yes, we generally want tests for any new functionality.

vinoski
vinoski approved these changes Nov 12, 2023
View reviewed changes
Copy link
Collaborator

@vinoski vinoski left a comment

Choose a reason for hiding this comment

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

LGTM

@vinoski vinoski merged commit a57b2c1 into erlyaws:master Nov 12, 2023
@vinoski
Copy link
Collaborator

vinoski commented Nov 12, 2023

Thanks for fixing this!

@vinoski vinoski mentioned this pull request Nov 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vinoski vinoski vinoski approved these changes

Assignees

@vinoski vinoski

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gnaeser @vinoski @avtobiff