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

Phone doesn't work with safe_yaml #38

Open
konklone opened this issue Apr 30, 2013 · 3 comments · May be fixed by #39
Open

Phone doesn't work with safe_yaml #38

konklone opened this issue Apr 30, 2013 · 3 comments · May be fixed by #39
Assignees
Labels

Comments

@konklone
Copy link

Loading YAML without extra Ruby-specific processing, which is now a best practice following the wave of security vulnerabilities earlier this year, means that the phone gem's YAML data is broken, because it uses symbol keys.

So when using safe_yaml, for example, this causes the symbol keys to be read as string keys, yielding validation errors on valid phone numbers.

@konklone konklone linked a pull request Apr 30, 2013 that will close this issue
@Xylakant
Copy link

Xylakant commented May 6, 2013

This repo seems unmaintained. However, you are mistaken: Loading YAML without extra Ruby-specific processing is best practice for untrusted data, for example for input from clients. Your own config files or data your app serialized to storage can be regarded as trusted and can certainly use any ruby specific processing.

@konklone
Copy link
Author

konklone commented May 6, 2013

Sure, but in practice, the safest way guarantee that my code, and the code of all the libraries I depend on, is using trusted YAML is to use a blunt instrument like monkeypatching YAML. This is what safe_yaml does.

phone is a small gem, and I can pretty easily see what it's doing with its YAML. But I can't inspect all the gems I depend on, and all of their dependencies. The only YAML I'll consider trusted input is YAML that I wrote in my own application code. Library code, unless it's using Ruby-specific processing for a specific reason (not just cause symbol keys are/were conventional), should take a vanilla approach to YAML that doesn't require I extend it my trust.

@elskwid
Copy link
Collaborator

elskwid commented May 9, 2013

I'm going to be making a pass through issues soon. See #41.

@elskwid elskwid self-assigned this Jan 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants