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

API Rescue Master Branch PR: Remove isDev / isTest querystring arguments #10456

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli mentioned this pull request Aug 23, 2022
5 tasks
@GuySartorelli GuySartorelli changed the title API Remove isDev / isTest querystring arguments API Rescue Master Branch PR: Remove isDev / isTest querystring arguments Aug 23, 2022
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

There was a bunch of removed unit tests in the original commit - do we need to worry about those?

@GuySartorelli
Copy link
Member Author

Those tests have actually changed since then to explicitly check that you can't use the isDev and isTest get parameters to change the env type - which is exactly what this rescued commit is intending to do (by outright removing that functionality). That's also why the CI is green despite not removing those tests.
It could be argued that the tests aren't necessary now that the functionality is removed, but because this is related to a security issue I decided it was best to keep them.
Happy to remove if you think they're not needed anymore.

@emteknetnz emteknetnz merged commit 4a3b6d9 into silverstripe:5 Aug 24, 2022
@emteknetnz emteknetnz deleted the pulls/5/rescue-master-remove-unsafe-queryparams branch August 24, 2022 03:14
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