Skip to content

fix: Changes to resolve #2688#3090

Merged
ozh merged 3 commits intoYOURLS:masterfrom
dan-r:master
Oct 30, 2021
Merged

fix: Changes to resolve #2688#3090
ozh merged 3 commits intoYOURLS:masterfrom
dan-r:master

Conversation

@dan-r
Copy link
Copy Markdown
Contributor

@dan-r dan-r commented Oct 18, 2021

This resolves the issues caused in the issue #2688.

The changes to resolve this issue are minor as follows:

  • Set the $user variable passed to the yourls_verify_nonce method to '-1' (unauthorised). There seems to be an issue where the original nonce is created prior to the user logging in, and then verified after the fact. This change ensures that throughout the script execution, the same nonce is being verified
  • Add an exit() after a redirect in yourls_is_valid_user(). Without this, the redirect causes the URL to be added to the database twice (or fail the second time).

I've tested these changes on my local instance and see no issues.

@ozh
Copy link
Copy Markdown
Member

ozh commented Oct 19, 2021

Thanks for tackling this problem !

I'm a bit worried about the need to add an exit() statement within the function call. Wouldn't a return work ? The thing is, exit() (and die()) are bitching when it comes to unit testing.

@dan-r
Copy link
Copy Markdown
Contributor Author

dan-r commented Oct 19, 2021

My pleasure - I've been using YOURLS on and off for many years so glad to be able to contribute!

I've replaced the exit() with a return and the bug still appears fixed.

I haven't had a chance to get the testing setup in my local env yet, but will (hopefully) get them working later on.

@dan-r
Copy link
Copy Markdown
Contributor Author

dan-r commented Oct 29, 2021

@ozh sorry to bump but was there anything else that needed changing with this, or any tests that needed updating as a result? Thanks!

@ozh
Copy link
Copy Markdown
Member

ozh commented Oct 30, 2021

Hey, no offense, you're right bumping this, it was sitting on my todo list. Thanks for this PR!

@ozh ozh merged commit a52df70 into YOURLS:master Oct 30, 2021
This was referenced Jan 9, 2022
@ozh
Copy link
Copy Markdown
Member

ozh commented Jan 9, 2022

Reverted for now, the fix was breaking the tests and after a couple hours on this I decided I prefered a minor UI bug than a whole broken test suite. I'm reopening #2688

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.

2 participants