fix(schematics): escape values interpolated into generated Data Connect provider code - #3727
fix(schematics): escape values interpolated into generated Data Connect provider code#3727herdiyana256 wants to merge 3 commits into
Conversation
…ct provider code featureToRules's DataConnect case builds the generated provideDataConnect(...) call from two values read out of the project's own dataconnect.yaml/ connector.yaml: config.package (passed as a module specifier to addRootProvider's external()) and the connectorConfig object's location/connector/service strings (interpolated directly into a double-quoted object literal). Neither was validated or escaped, so a value containing a quote character breaks out and lets arbitrary source land in the project's generated provider file. config.package is now checked against a conservative allow-list pattern before being used as a module specifier, falling back to the connectorConfig object form otherwise. The connectorConfig values are now serialized with JSON.stringify instead of raw string interpolation.
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for this, and for the clear write-up and repro in the description. I worked through the change against the compiled schematic and it does close the injection you set out to fix:
- A quote-bearing value in
connectorConfigis now escaped byJSON.stringifyand lands as inert string data, and a quote-bearingpackageis rejected and falls back to the object form. - I also confirmed the
else if (config.connectorConfig)guard removes theObject.keys(undefined)crash on a connector with nogenerate.javascriptSdk. Nice catch on that second one.
A few things I ran into that I think are worth a look before this goes in. I may be missing context about how these configs get generated, so please push back where I have this wrong.
One regression I'd suggest fixing
The move from "${value}" to JSON.stringify(value) also preserves the value's type, not just its escaping. getDataConnect's ConnectorConfig requires location, connector, and service to be strings, but the values come straight from yaml.parse, which turns an unquoted 123 or true into a number or boolean.
So for those inputs the generated code changes like this:
service: 123in the yaml previously generatedservice: "123"(a string, compiles)- with this change it generates
service: 123(a number, which no longer satisfiesConnectorConfig)
All-string values (the normal case) are unaffected. It only bites when an identifier is all-digits or a yaml magic token, which is uncommon, but it is a case that used to compile and now would not. Wrapping the value in String(...) before JSON.stringify keeps both properties (the escaping and the string type):
(key) => `${key}: ${JSON.stringify(String((config.connectorConfig as ConnectorConfig)[key]))}`A test would make this much stronger
featureToRules doesn't have a schematics spec today, and since this is a codegen-correctness fix I think one would carry a lot of weight here: one case asserting a quote-bearing value round-trips as escaped data, and one asserting the no-javascriptSdk config doesn't throw.
Happy to point at the existing common.jasmine.ts harness as a starting shape if that helps.
One smaller note
The regex /^[^'"\\\n\r]+$/ correctly blocks the breakout characters, but it still admits things like spaces and ../…, so the comment "reject anything that isn't a plausible package specifier" reads a little stronger than what it does.
I checked that those inputs stay inside the generated import string (they produce a broken-but-not-injecting specifier), so it's not a correctness problem, just worth aligning the comment with what the pattern actually enforces.
None of this takes away from the core fix, which I'm glad you found. If any of the above is off because of something I'm not seeing in the config flow, let me know and I'll take another pass.
… provider config codegen JSON.stringify alone changes a yaml scalar's type along with escaping it, so an unquoted numeric/boolean connectorConfig value (e.g. service: 123) no longer satisfied ConnectorConfig's string fields after the previous escaping fix. Wrap each value in String(...) before JSON.stringify to keep both the escaping and the string type. Extract the config-string-building logic into connectorConfigObjectLiteral, isValidPackageSpecifier, and resolveDataConnectProviderConfig so it's unit-testable independent of the addRootProvider schematic machinery, and add coverage for the injection-escaping fix, the type-coercion regression, and the no-javascriptSdk fallback path.
|
Thanks for the thorough review pushed a follow-up commit addressing all three points:
One incidental fix needed to get the new test file running at all under |
`featureToRules`'s `DataConnect` case builds the generated `provideDataConnect(...)` call from two values read out of the project's own `dataconnect.yaml`/`connector.yaml`: `config.package` (passed as a module specifier to `addRootProvider`'s `external()`) and the `connectorConfig` object's `location`/`connector`/`service` strings, interpolated directly into a double-quoted object literal (```${key}: "${value}"```). Neither was validated or escaped, so a value containing a quote character breaks out of its string literal and lands arbitrary source in the project's generated provider file.
Confirmed with the exact two expressions from `utils.ts`: a `connectorConfig.location` of ``us-central1"; console.log("INJECTED"); const _z="x`` produced `getDataConnect({location: "us-central1"; console.log("INJECTED"); const _z="x",...})`` — a live statement, not an unusual string value.
`config.package` is now checked against a conservative allow-list pattern (rejects quotes, backslashes, newlines) before being used as a module specifier, falling back to the `connectorConfig` object form otherwise. The `connectorConfig` values are now serialized with `JSON.stringify` instead of raw string interpolation, which also fixes a pre-existing `as ConnectorConfig` cast that assumed `connectorConfig` was always defined whenever `package` was falsy (it isn't — both are set together or neither is, per `parseDataConnectConfig`).
Related but separate PR for a different sink in the same general area: #3726 (the `deploy` schematic's Cloud Run `gcloud` invocation).
`npx tsc --noEmit` and `npx eslint src/schematics/utils.ts` both pass clean.