feat(#603): json_file setup fields — upload the key, do not transcribe it - #682
Merged
Conversation
…e it OM-17, structural fix. The reported near-miss: a tester typed their real Google account password into `gw_sa_private_key`. Nothing careless about it — the form asked them to hand-transcribe two values out of a service-account key file into an email field and a masked field stacked directly beneath it, which is the visual pattern of a login. #599 made that mistake DETECTABLE (caution copy + `pattern` validation). Uploading the file removes the opportunity to make it. The tester proposed exactly this, and they were right: it is structural, and it removes a step rather than adding one. ## The type is mirrored in SIX places, not three The issue named three (`admin-v1.ts`'s union, `manifestLoader.isSetupFieldType`, `agentSpec`'s `.strict()` z.enum). Two more only surface when you follow the value: `SUPPORTED_TYPES` in `installService.ts` — where a missing member does NOT error, `isSupportedType` just skips the field and the upload vanishes from the install wizard with no diagnostic — and `InstallSetupField`'s own shape, which is what `extractSetupSchema` returns and therefore what the new route reads. A sixth lives in web-ui (`storeTypes.ts`). All six carry `json_file` now, each with a comment naming the others. ## Server-side extraction `src/plugins/setupJsonFile.ts` — the client posts the file's TEXT, the server parses it, validates it, explodes it into the keys named in `extracts`, and stores only those. The browser is never trusted to decide which bytes become `gw_sa_private_key`. Guard rails, each pinned by a test: - the size cap is applied to the RAW text, before `JSON.parse` runs; - `expect` rejects the wrong file BEFORE any value is extracted (an OAuth client secret and a service-account key look alike at a glance); - a missing/empty/non-string value is a readable error, never a silently empty secret — that outcome would look like a successful setup and fail later, far from the cause; - `Object.hasOwn` on every path segment, so `$.constructor` cannot reach a prototype property; - the raw document is never returned, logged or persisted. Paths are a deliberate subset (`$.a.b`), not JSONPath: service-account files are flat, and the full grammar would add a dependency plus an evaluation surface for no case anyone has. An unsupported path fails loudly instead of matching nothing. ## The write path is SHARED, not copied `POST /installed/:id/secrets/from-json` extracts, then hands the derived values to the same `applySetupValues` the typed `PATCH …/secrets` now uses. The security claim of #603 is that an extracted value is treated exactly like a typed one — same `pattern` validation, same vault/config split, same reactivation, same response (key NAMES only, never a value). Two implementations of that would be two implementations that can drift, and the drift would be invisible until a `json_file` field quietly skipped a check the typed path applies. The extraction map comes from the MANIFEST, never the request: a caller-supplied `extracts` would let anyone write any vault key from any file. `coerce()` refuses a value submitted under a `json_file` key rather than ignoring it — silently dropping it would let a client believe it had stored a credential. The install form skips the field for the same reason, so the two sides agree. ## UI The install wizard renders a file picker. Falling through to the text input would be worse than not shipping the type: it would ask the operator to paste a raw key into a field, which is the transcription step this removes. The post-install `CredentialsEditor` omits `json_file` fields — every row there is a text input, and the DERIVED fields are ordinary secrets listed as usual, so post-install editing still works. Offering the upload there too is a follow-up. ## The errorHelp guard earned its keep The new codes were first built with a template (`runtime.json_file_${failure.code}`), which the coverage guard cannot see — the codes would have shipped with no operator copy while the suite stayed green. Now a `JSON_FILE_ERROR_CODES` table holds literals, exhaustive over `JsonFileFailureCode` so a new failure kind cannot ship without a wire code, and the forwarder is registered in the guard. 11 new help entries in both locales; the two spec-level ones say plainly that the fault is in the plugin, not the operator's file — that distinction is the difference between "try another file" and "report this". Verification: `test/setupJsonFile.test.ts` 12/12, related middleware suites 45/45, web-ui 693/693, `tsc --noEmit` clean on both sides, `typecheck:test` ratchet 406 = baseline, eslint 0 errors. `TemplateInstantiateForm.test.tsx` failed once in a full-suite run and passes both in isolation (22/22) and on a full-suite re-run (693/693) — the repo's known cross-file pollution, not this change. Not included: the Google Workspace manifest itself lives in a separate repo, and post-install upload in the credentials editor. Closes #603
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OM-17, structural fix. The reported near-miss: a tester typed their real Google account password into
gw_sa_private_key. Nothing careless about it — the form asked them to hand-transcribe two values out of a service-account key file into an email field and a masked field stacked directly beneath it, which is the visual pattern of a login.#599 made that mistake detectable (caution copy +
patternvalidation). Uploading the file removes the opportunity to make it. The tester proposed exactly this, and they were right: it is structural, and it removes a step rather than adding one.The type is mirrored in six places, not three
The issue named three. Two more only surface when you follow the value, and a sixth lives in web-ui:
admin-v1.ts— theSetupFieldTypeunionmanifestLoader.isSetupFieldTypeagentSpec's.strict()z.enumSUPPORTED_TYPESininstallService.tsisSupportedTypeskips the field and the upload vanishes from the install wizard silentlyInstallSetupField's shapeextractSetupSchemareturns, i.e. what the new route readsweb-ui/app/_lib/storeTypes.tsAll six carry
json_filenow, each commented with a pointer to the others.Server-side extraction
middleware/src/plugins/setupJsonFile.ts. The client posts the file's text; the server parses, validates, explodes it into the keys named inextracts, and stores only those. The browser is never trusted to decide which bytes becomegw_sa_private_key.Guard rails, each pinned by a test:
JSON.parse— the fixture is valid JSON on purpose, so the test would catch a cap that ran after parsing;expectrejects the wrong file before any value is extracted — an OAuth client secret and a service-account key look alike at a glance;Object.hasOwnon every path segment, so$.constructorcannot reach a prototype property;Paths are a deliberate subset (
$.a.b), not JSONPath: service-account files are flat, and the full grammar would add a dependency plus an evaluation surface for no case anyone has. An unsupported path fails loudly instead of matching nothing.The write path is shared, not copied
POST /installed/:id/secrets/from-jsonextracts, then hands the derived values to the sameapplySetupValuesthat the typedPATCH …/secretsnow uses.The security claim of #603 is that an extracted value is treated exactly like a typed one — same
patternvalidation, same vault/config split, same reactivation, same response (key names only, never a value). Two implementations of that would be two implementations that can drift, and the drift would be invisible until ajson_filefield quietly skipped a check the typed path applies.The extraction map comes from the manifest, never the request: a caller-supplied
extractswould let anyone write any vault key from any file.coerce()refuses a value submitted under ajson_filekey rather than ignoring it — silently dropping it would let a client believe it had stored a credential. The install form skips the field for the same reason, so both sides agree.UI
The install wizard renders a file picker. Falling through to the text input would be worse than not shipping the type: it would ask the operator to paste a raw key into a field, which is the transcription step this removes.
The post-install
CredentialsEditoromitsjson_filefields — every row there is a text input, and the derived fields are ordinary secrets listed as usual, so post-install editing still works. Offering the upload there too is a follow-up, not a gap in the flow.The errorHelp guard earned its keep
The new codes were first built with a template (
runtime.json_file_${failure.code}), which the coverage guard cannot see — they would have shipped with no operator copy while the suite stayed green.Now a
JSON_FILE_ERROR_CODEStable holds literals, typed exhaustively overJsonFileFailureCodeso a new failure kind cannot ship without a wire code, and the forwarder is registered in the guard. 11 new help entries per locale; the two spec-level ones say plainly that the fault is in the plugin, not the operator's file — that distinction is the difference between "try another file" and "report this".(Amusingly, the guard then caught my own explanatory comment, because it cannot tell a comment from code. Reworded.)
Verification
test/setupJsonFile.test.ts: 12/12tsc --noEmit: clean on both sidestypecheck:testratchet: 406 = baselineTemplateInstantiateForm.test.tsxfailed once in a full-suite run and passes both in isolation (22/22) and on a full-suite re-run (693/693) — the repo's known cross-file pollution, not this change.Not included
Closes #603
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.