Skip to content

fix(schematics): escape values interpolated into generated Data Connect provider code - #3727

Open
herdiyana256 wants to merge 3 commits into
angular:mainfrom
herdiyana256:fix-dataconnect-codegen-injection
Open

fix(schematics): escape values interpolated into generated Data Connect provider code#3727
herdiyana256 wants to merge 3 commits into
angular:mainfrom
herdiyana256:fix-dataconnect-codegen-injection

Conversation

@herdiyana256

Copy link
Copy Markdown

`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.

…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 armando-navarro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 connectorConfig is now escaped by JSON.stringify and lands as inert string data, and a quote-bearing package is rejected and falls back to the object form.
  • I also confirmed the else if (config.connectorConfig) guard removes the Object.keys(undefined) crash on a connector with no generate.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: 123 in the yaml previously generated service: "123" (a string, compiles)
  • with this change it generates service: 123 (a number, which no longer satisfies ConnectorConfig)

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.

@armando-navarro armando-navarro added bump: patch comp: data-connect Data Connect (src/data-connect). comp: schematics ng add / deploy schematics (src/schematics). type: bug Defect: expected behavior doesn't happen. labels Aug 3, 2026
… 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.
@herdiyana256

herdiyana256 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review pushed a follow-up commit addressing all three points:

  • Type regression: wrapped each connectorConfig value in String(...) before JSON.stringify, so escaping and the string type are both preserved (e.g. service: 123 from an unquoted yaml scalar still generates service: "123").
  • Tests: added src/schematics/utils.jasmine.ts covering the injection-escaping fix (a quote-bearing value round-trips as inert string data via new Function), the type-coercion fix, isValidPackageSpecifier's rejection of quote/backslash/newline characters, and the no-javascriptSdk fallback (resolveDataConnectProviderConfig returns { kind: 'literal', literal: '{}' } without throwing). To make these testable without spinning up a full Angular workspace tree for addRootProvider, I extracted the config-string-building logic out of the DataConnect case into connectorConfigObjectLiteral, isValidPackageSpecifier, and resolveDataConnectProviderConfig.
  • Comment wording: reworded the comment above isValidPackageSpecifier to describe what the pattern actually enforces (blocking the string-literal breakout characters) rather than implying full module-specifier validation.

One incidental fix needed to get the new test file running at all under test:node-esm: utils.ts wasn't reachable from any existing jasmine test before, and two of its imports (@angular-devkit/schematics/tasks and ./common) were missing the explicit .js/index.js suffixes Node's ESM resolver needs — update/v21/index.ts already carries this same explicit-suffix convention for the same reason, so I aligned utils.ts with it. npm run build:jasmine && npm run test:node-esm now passes (67 specs, 0 failures), and tsc --noEmit / eslint are clean on both files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bump: patch comp: data-connect Data Connect (src/data-connect). comp: schematics ng add / deploy schematics (src/schematics). type: bug Defect: expected behavior doesn't happen.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants