-
Notifications
You must be signed in to change notification settings - Fork 76
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
base: master
Are you sure you want to change the base?
Conversation
I think I'm 👍 on this approach. Tests would make me more confident that it works as expected. |
Hi! I added a support for relative paths. Now we can pass |
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 For reference just in case this type of system path munging is new to you, be sure that any paths are normalized using |
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. |
@pipermerriam @Lookyan We would like to use this feature, are you still planning to fix remaining issues? |
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. |
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. |
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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__)), '../../../../../') |
There was a problem hiding this comment.
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.
#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?