Read workflow_settings.yaml #1580
Conversation
core/index.ts
Outdated
| @@ -19,4 +20,4 @@ const supportedFeatures = [dataform.SupportedFeatures.ARRAY_BUFFER_IPC]; | |||
|
|
|||
| // These exports constitute the public API of @dataform/core. | |||
| // Changes to these will break @dataform/api, so take care! | |||
| export { compiler, main, session, supportedFeatures, version }; | |||
| export { compiler, main, readWorkflowSettings, session, supportedFeatures, version }; | |||
There was a problem hiding this comment.
if you are exposing this to call it from the CLI and/or closed-source... i would strongly recommend not doing that.
it exposes the caller to hacked versions of @dataform/core which could do anything in readWorkflowSettings. yes, it can still be run in a sandbox, but why expose yourself to a hacked version when the caller could just read the YAML directly and trust that it reads what is actually there?
There was a problem hiding this comment.
I don't think there's really security implications here:
- CLI callers are always potentially exposed to hacked versions of dependencies in their
node_modules, I don't see why this makes it any more or less secure. - Closed source will run it using the same sandbox as
mainis invoked from.
the caller could just read the YAML directly
I don't want to do this in two places. It means reading the YAML, converting it to JSON, and then validating that the proto is correct. Doing it in one place makes it a lot more consistent and less work.
There was a problem hiding this comment.
Yes, agreed, there are no (immediate) security implications here - I didn't say that.
The point is that it is useful to be able to read files and trust what you read. If a caller (outside of @dataform/core) wants to read workflow_settings.yaml, presumably it wants to read what is actually there, not (potentially) something else entirely.
I feel pretty strongly we should not be using this function outside of @dataform/core, for that reason alone.
There was a problem hiding this comment.
I can sort of buy that, but it would be weird for a user to call main from a different Node context to readWorkflowSettings.
I've removed the export for now anyway, and will see if I can find a workaround for restricting the warehouse, that doesn't require calling that function first or changing the request to the compiler.
* Add base dir structure for reading workflow settings, some cleanup * Add a basic workflow_settings reader * Progress with loading YAML via require * Working YAML reader for workflow settings * Tidying * Cleanup bits * Fix relative path for requiring workflow settings * Fix import orders and lint errors * Stop exporting getWorkflowSettings * Move TSLint comment locations * Add explicit include for new compilation_sql BUILD file to cli/api * Fix compilation sql visibility
This means:
require.main, as that's where this loading and conversion happens.main, a Node VM has to be used to apply the override forrequire.