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

#60 $ref pointing to a external file #125

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Lookyan
Copy link

@Lookyan Lookyan commented Sep 23, 2016

#60 ($ref pointing to a external file)

I added a support of absolute path resolving in $ref.
I also want to add support for relative paths using special optional param base_path.
I will write tests for these cases.

What do you think about this?

@pipermerriam
Copy link
Owner

I think I'm 👍 on this approach. Tests would make me more confident that it works as expected.

@Lookyan Lookyan changed the title [WIP] #60 $ref pointing to a external file #60 $ref pointing to a external file Oct 12, 2016
@Lookyan
Copy link
Author

Lookyan commented Oct 12, 2016

Hi!

I added a support for relative paths. Now we can pass base_path which would be the base directory for $ref schemas. I think it is very useful. I'm not sure that it is a cool solution for relative paths, but now only using kwargs we can get base_path on any level in validation (I was thinking about thread local, but I think it is not ok here).
Also I added tests for these cases.

@pipermerriam
Copy link
Owner

Ok, I've got a security concern that I'm curious to hear your thoughts on. This feature will enable loading any file on the filesystem which makes me a little uncomfortable. I'm haven't thought of this enough to really have a good picture of what the actual attack vector is but it still makes me uncomfortable.

Can you look into restricting any path to have a common base as the primary schema that is being loaded?

For example, if the schema being loaded was /var/schemas/example.com/some-schema.json then only files who's path begins with /var/schemas/example.com/ should be allowed.

For reference just in case this type of system path munging is new to you, be sure that any paths are normalized using os.path.abspath so that paths like /var/schemas/example.com/../../../etc/super-secret-passwords.txt do not pass the test.

@Lookyan
Copy link
Author

Lookyan commented Oct 14, 2016

Of course, public schemas is prone to have a vulnerability in this case. But it can be a good feature to load a file from for example parent directory. And from the other side security is very important, so I think we should avoid dot-dot-slash attack. I will patch it to resolve schemas only in subdirectories of base_path.

@danopz
Copy link
Contributor

danopz commented Feb 7, 2017

@pipermerriam @Lookyan We would like to use this feature, are you still planning to fix remaining issues?

@Lookyan
Copy link
Author

Lookyan commented Feb 7, 2017

Hi, copynpaste!

Yes, I will fix it. But I've found one more issue with this feature. I didn't cover cases where we have multilevel nested schemas. My approach is broken when we use $ref on $ref on $ref. If you can fix it, it would be great. If not, I will come back to this issue approximately in next week.

Sorry for long pause.

@pipermerriam
Copy link
Owner

I'm setting some time aside tomorrow to go through open issues. @copynpaste always feel free to pester me to get attention on stuff like this.

Copy link
Owner

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I may have missed it but I didn't see the protection against loading files outside of the the base directory. I'd like to see that added before merging this.

if parts.path.startswith('/'):
context = load_source(parts.path)
elif 'base_path' in kwargs:
context = load_source(os.path.join(kwargs['base_path'], parts.path))
Copy link
Owner

Choose a reason for hiding this comment

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

This same block of code is repeated quite a few times. Can you abstract it into a reusable function.

@@ -10,6 +12,9 @@
)


DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), '../../../../../')
Copy link
Owner

Choose a reason for hiding this comment

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

An easier to read way to do this is to do the following.

import flex
BASE_DIR = os.path.dirname(os.path.dirname(flex.__file__))

This will give you the root folder of the project.

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.

3 participants