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

Protect against fallback: false for exists? #75

Closed
wants to merge 3 commits into from

Conversation

namolnad
Copy link
Contributor

I noticed that exists? with fallback: false is potentially still exposed to an enumeration vulnerability, which would allow attackers to determine things like the number of records in a table. E.g. If an attacker were to discover an endpoint that was backed simply by Record.exists? then this would allow them to pass in a non-prefixed id and potentially determine information like the number of records in a given table, even if fallback was set to false. I think this change makes the behavior a bit safer and more predictable, though I was unsure how to handle array find-conditions, so have treated those as unsupported for now.

Hopefully this is helpful. Let me know what you think :)

@excid3
Copy link
Owner

excid3 commented Oct 14, 2024

I'm tempted to just remove the exists? override and recommend using decode_prefix_id(params[:id]) before calling exists?.

We'll end up with more and more of workarounds the more we override Rails features which doesn't seem like a good long-term approach.

@namolnad
Copy link
Contributor Author

@excid3 I feel like that could be a good idea. I've been finding more and more edge cases to handle for the current state of the world, which already seems a bit shaky/nebulous. Not to mention these all seem to mess with fixture accessor setup from any Rails version before 7.1 (hence the test failures)

@excid3
Copy link
Owner

excid3 commented Oct 14, 2024

I think that's the route we'll take. The find override isn't too bad and is 99% of the usage I think.

@excid3 excid3 closed this in 33c7982 Oct 14, 2024
@namolnad
Copy link
Contributor Author

Makes sense! The removal via 33c7982 provides the protection I was thinking was needed, so 👍 all around. Thanks for working with me on it!

@excid3
Copy link
Owner

excid3 commented Oct 14, 2024

Thanks for your help on this too @namolnad! This feels like a good move all around.

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