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

feat(config): use yml.load to support custom js types #2788

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

JLHwung
Copy link
Collaborator

@JLHwung JLHwung commented Oct 7, 2017

The safeLoad method is recommended for untrusted data. However, hexo uses js-yaml to parse config files handwritten by SAs. These config files should be treated as trusted data and thus we can use load method for flexibility. The load method uses DEFAULT_FULL_SCHEMA to support !!js/regexp or !!js/function types.

This commit does not introduce breaking change as DEFAULT_FULL_SCHEMA is a superset of DEFAULT_SAFE_SCHEMA, which is defaults of safeLoad.

Thank you for creating a pull request to contribute to Hexo code! Before you open the request please review the following guidelines and tips to help it be more easily integrated:

  • Add test cases for the changes.
  • Passed the CI test.

The `safeLoad` method is recommended for untrusted data. Hexo use `js-yaml` to parse config files handwritten by SAs. These config files should be treated as trusted data and use load method for flexibility. The load method uses DEFAULT_FULL_SCHEMA to support !!js/regexp or !!js/function types.

This commit does not introduce breaking change as DEFAULT_FULL_SCHEMA is a superset of DEFAULT_SAFE_SCHEMA
@JLHwung JLHwung requested a review from NoahDragon October 7, 2017 16:09
@coveralls
Copy link

coveralls commented Oct 7, 2017

Coverage Status

Coverage remained the same at 97.219% when pulling 3d88929 on load-all-types-on-js-yaml into bccee83 on master.

@NoahDragon
Copy link
Member

Just not confident about this change. I saw many issues related to the wrong format of config file. What happened if the yaml file is corrupt? The load will continue parsing the correct parts?

@JLHwung
Copy link
Collaborator Author

JLHwung commented Oct 12, 2017

@NoahDragon I don't think the corruption behavior of safeLoad differs to the one of load.

As from the documentation and the code, the only difference between safeLoad and load is the schema support: The load has support of !!js/regexp, !!js/undefined and !!js/function type schema other than safeLoad. The safeLoad calls load under the hood but pass in restricted schema.

We can add corruption behavior test on js-yaml though.

@NoahDragon
Copy link
Member

Thank you. Then I think it is good for us.

Copy link
Member

@NoahDragon NoahDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@JLHwung JLHwung merged commit d5b0f82 into master Oct 20, 2017
@JLHwung JLHwung deleted the load-all-types-on-js-yaml branch October 20, 2017 07:26
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