@luismsgomes opened this Pull Request on June 11th 2020

Please see https://github.com/matomo-org/matomo/issues/16057 for details on what this commit is fixing.

@tsteur commented on June 16th 2020 Member

Looks generally good to me. @Findus23 @sgiehl @diosmosis any other thoughts?

Generally thinking because there are some minor changes in there that could break something we might prefer to have this in Matomo 4 and not Matomo 3 where we now only fix security issues and bigger bugs.

@luismsgomes commented on June 16th 2020

Well, this is big for me since without this patch I couldn't login ;-)
That's why I took the effort to fix it.

On Tue, Jun 16, 2020, 21:32 Thomas Steur <notifications@github.com> wrote:

Looks generally good to me. @Findus23 https://github.com/Findus23
@sgiehl https://github.com/sgiehl @diosmosis
https://github.com/diosmosis any other thoughts?

Generally thinking because there are some minor changes in there that
could break something we might prefer to have this in Matomo 4 and not
Matomo 3 where we now only fix security issues and bigger bugs.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/matomo-org/matomo/pull/16058#issuecomment-644996229,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ACFVCGJIPYOOGV56ZPZDODLRW7JD7ANCNFSM4N3YAU6A
.

@tsteur commented on June 16th 2020 Member

@luismsgomes sorry about that. There will be only very few releases for 3.X in the future so it might be worth patching it in the worst case. Maybe we could think of a less "risky" version for 3.X where it doesn't change as much. Eg for the case where a port is defined like in https://github.com/matomo-org/matomo/pull/16058/files#diff-e3d1b64ec2cd9e138ad8d5c1a0d09a0eR140 we are now no longer returning the URLs without port. This could be causing trouble to other users maybe (as it's always hard to tell with these things)

@tsteur commented on June 24th 2020 Member

@Findus23 can you think of any objections or do you reckon it should be fine?

@Findus23 commented on June 24th 2020 Member

I know little about how this part works, so I can't comment on it.

@diosmosis commented on June 24th 2020 Member

@tsteur missed this issue, looks ok to me, Nonce::getAcceptableOrigins() is only used in Nonce so it won't affect anything else. I can't think of a problem w/ treating http://somesite.com & http://somesite.com:80 as equivalent. Might be good to add some tests to NonceTest.

This Pull Request was closed on June 24th 2020
Powered by GitHub Issue Mirror