#60 $ref pointing to a external file#125
#60 $ref pointing to a external file#125Lookyan wants to merge 6 commits intopipermerriam:masterfrom
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. |
pipermerriam
left a comment
There was a problem hiding this comment.
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.
This same block of code is repeated quite a few times. Can you abstract it into a reusable function.
| ) | ||
|
|
||
|
|
||
| DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), '../../../../../') |
There was a problem hiding this comment.
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?