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

fix searching in external article database (#772) #773

Merged
merged 1 commit into from
Oct 17, 2020

Conversation

JuliusR
Copy link
Contributor

@JuliusR JuliusR commented Oct 10, 2020

Can we use this simple fix for #772 ?

@wvengen
Copy link
Member

wvengen commented Oct 10, 2020

Thanks for finding the issue here! 💯
Could this introduce a security issue?

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 11, 2020

Could this introduce a security issue?

This is the correct question. I believe the answer is "no" because we are using the result for the following two purposes only:

I perfectly agree that safety should be obvious, so my first attempt with to_unsafe_h is bad practice.

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 11, 2020

I updated the code to avoid to_unsafe_h. Could you have a look again please?

Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

Thank you, this cleans up the existing code too. Assuming you've tested this well, 👍

@JuliusR JuliusR added this to the 4.7.1 milestone Oct 14, 2020
@JuliusR JuliusR mentioned this pull request Oct 14, 2020
24 tasks
@JuliusR JuliusR merged commit 4fa5c4e into foodcoops:master Oct 17, 2020
@paroga
Copy link
Member

paroga commented Oct 17, 2020

Please rebase your commits when pushing them to master to avoid the useless merge commits, which make the history a lot less readable. The change in the rewritten history is now 4043433.

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.

searching in external article database fails with "A problem has occured." alert
3 participants