-
Notifications
You must be signed in to change notification settings - Fork 1
fix(console): reattach backup dropdown options from CRD annotations #40
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| import { describe, it, expect } from "vitest" | ||
| import { graftOptionSources } from "./crd-option-sources.ts" | ||
|
|
||
| interface SchemaNode { | ||
| type?: string | ||
| properties?: Record<string, SchemaNode> | ||
| "x-cozystack-options"?: { source: string } | ||
| } | ||
|
|
||
| describe("graftOptionSources", () => { | ||
| it("grafts a source onto a nested field (applicationRef.kind)", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { | ||
| applicationRef: { | ||
| type: "object", | ||
| properties: { | ||
| kind: { type: "string" }, | ||
| name: { type: "string" }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| const out = graftOptionSources(spec, { | ||
| "options.cozystack.io/source.applicationRef.kind": "appkind", | ||
| }) as SchemaNode | ||
|
|
||
| expect(out.properties?.applicationRef?.properties?.kind?.["x-cozystack-options"]).toEqual({ | ||
| source: "appkind", | ||
| }) | ||
| // Sibling left untouched. | ||
| expect( | ||
| out.properties?.applicationRef?.properties?.name?.["x-cozystack-options"], | ||
| ).toBeUndefined() | ||
| }) | ||
|
|
||
| it("grafts a source onto a top-level spec field (backupClassName)", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { backupClassName: { type: "string" } }, | ||
| } | ||
|
|
||
| const out = graftOptionSources(spec, { | ||
| "options.cozystack.io/source.backupClassName": "backupclass", | ||
| }) as SchemaNode | ||
|
|
||
| expect(out.properties?.backupClassName?.["x-cozystack-options"]).toEqual({ | ||
| source: "backupclass", | ||
| }) | ||
| }) | ||
|
|
||
| it("applies every source annotation on a CRD (BackupJob shape)", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { | ||
| applicationRef: { type: "object", properties: { kind: { type: "string" } } }, | ||
| planRef: { type: "object", properties: { name: { type: "string" } } }, | ||
| backupClassName: { type: "string" }, | ||
| }, | ||
| } | ||
|
|
||
| const out = graftOptionSources(spec, { | ||
| "controller-gen.kubebuilder.io/version": "v0.16.4", | ||
| "options.cozystack.io/source.applicationRef.kind": "appkind", | ||
| "options.cozystack.io/source.planRef.name": "plan", | ||
| "options.cozystack.io/source.backupClassName": "backupclass", | ||
| }) as SchemaNode | ||
|
|
||
| expect(out.properties?.applicationRef?.properties?.kind?.["x-cozystack-options"]).toEqual({ | ||
| source: "appkind", | ||
| }) | ||
| expect(out.properties?.planRef?.properties?.name?.["x-cozystack-options"]).toEqual({ | ||
| source: "plan", | ||
| }) | ||
| expect(out.properties?.backupClassName?.["x-cozystack-options"]).toEqual({ | ||
| source: "backupclass", | ||
| }) | ||
| }) | ||
|
|
||
| it("ignores annotations without the option-source prefix", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { backupClassName: { type: "string" } }, | ||
| } | ||
|
|
||
| const out = graftOptionSources(spec, { | ||
| "controller-gen.kubebuilder.io/version": "v0.16.4", | ||
| "options.cozystack.io/other": "noise", | ||
| }) as SchemaNode | ||
|
|
||
| expect(out.properties?.backupClassName?.["x-cozystack-options"]).toBeUndefined() | ||
| }) | ||
|
|
||
| it("is a no-op for a path that does not exist in the schema", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { backupClassName: { type: "string" } }, | ||
| } | ||
|
|
||
| expect(() => | ||
| graftOptionSources(spec, { | ||
| "options.cozystack.io/source.missing.field": "appkind", | ||
| }), | ||
| ).not.toThrow() | ||
| }) | ||
|
|
||
| it("does not mutate the input schema, including nested nodes", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { | ||
| backupClassName: { type: "string" }, | ||
| applicationRef: { type: "object", properties: { kind: { type: "string" } } }, | ||
| }, | ||
| } | ||
|
|
||
| graftOptionSources(spec, { | ||
| "options.cozystack.io/source.backupClassName": "backupclass", | ||
| "options.cozystack.io/source.applicationRef.kind": "appkind", | ||
| }) | ||
|
|
||
| // Both a top-level field and a nested one must be left untouched, so a | ||
| // future shallow/partial clone that corrupts deep nodes can't slip through. | ||
| expect(spec.properties?.backupClassName?.["x-cozystack-options"]).toBeUndefined() | ||
| expect(spec.properties?.applicationRef?.properties?.kind?.["x-cozystack-options"]).toBeUndefined() | ||
| }) | ||
|
|
||
| it("returns the schema unchanged when there are no annotations", () => { | ||
| const spec: SchemaNode = { | ||
| type: "object", | ||
| properties: { backupClassName: { type: "string" } }, | ||
| } | ||
|
|
||
| expect(graftOptionSources(spec, undefined)).toBe(spec) | ||
| expect(graftOptionSources(spec, {})).toEqual(spec) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /** | ||
| * Graft the `x-cozystack-options` vendor keyword back onto a CRD's spec schema | ||
| * from its metadata annotations. | ||
| * | ||
| * The backups.cozystack.io CRDs cannot carry `x-cozystack-options` in their | ||
| * OpenAPI schema: apiextensions `JSONSchemaProps` is a closed struct that only | ||
| * preserves `x-kubernetes-*` extensions, and server-side apply rejects any | ||
| * other `x-` key outright. The field→source mapping the dropdowns need is | ||
| * therefore stored in CRD metadata annotations (which the apiserver does | ||
| * preserve) and reattached client-side here, so DynamicOptionsWidget — which | ||
| * reads `x-cozystack-options.source` off the schema node — keeps working. | ||
| * | ||
| * Annotation contract (emitted by kubebuilder markers on the Go types): | ||
| * options.cozystack.io/source.<dotted spec-relative path> = <option source> | ||
| * e.g. `options.cozystack.io/source.applicationRef.kind: appkind`. | ||
| */ | ||
|
|
||
| const SOURCE_ANNOTATION_PREFIX = "options.cozystack.io/source." | ||
|
|
||
| /** | ||
| * Return a copy of `specSchema` with `x-cozystack-options: { source }` set on | ||
| * every field named by a matching annotation. The input is not mutated. Paths | ||
| * that do not resolve in the schema are skipped silently. | ||
| */ | ||
| export function graftOptionSources( | ||
| specSchema: unknown, | ||
| annotations: Record<string, string> | undefined | null, | ||
| ): unknown { | ||
| if (!specSchema || typeof specSchema !== "object" || !annotations) { | ||
| return specSchema | ||
| } | ||
|
|
||
| const cloned = structuredClone(specSchema) | ||
|
|
||
| for (const [key, source] of Object.entries(annotations)) { | ||
| if (!source || !key.startsWith(SOURCE_ANNOTATION_PREFIX)) continue | ||
| const path = key.slice(SOURCE_ANNOTATION_PREFIX.length).split(".") | ||
| applySource(cloned, path, source) | ||
| } | ||
|
|
||
| return cloned | ||
| } | ||
|
|
||
| function applySource(specSchema: unknown, path: string[], source: string): void { | ||
| let node = specSchema | ||
| for (const segment of path) { | ||
| const properties = (node as { properties?: Record<string, unknown> })?.properties | ||
| if (!properties || typeof properties !== "object" || !(segment in properties)) { | ||
| return | ||
| } | ||
| node = properties[segment] | ||
| } | ||
|
Comment on lines
+46
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the To prevent this, we should explicitly block sensitive keys like for (const segment of path) {
if (segment === "__proto__" || segment === "constructor") {
return
}
const properties = (node as { properties?: Record<string, unknown> })?.properties
if (
!properties ||
typeof properties !== "object" ||
!Object.prototype.hasOwnProperty.call(properties, segment)
) {
return
}
node = properties[segment]
} |
||
| if (node && typeof node === "object") { | ||
| ;(node as Record<string, unknown>)["x-cozystack-options"] = { source } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, doeskey in objinclude prototype-chain properties likeprotoandconstructor, and isObject.hasOwn(obj, key)the recommended safe check to prevent prototype-pollution-style key traversal?💡 Result:
Yes, the
inoperator in JavaScript and TypeScript checks for the existence of a property both on the object itself and throughout its entire prototype chain [1][2]. Consequently,key in objwill returntruefor inherited properties such asproto,constructor, or any other properties defined on the object's prototypes [3][4][2].Object.hasOwn(obj, key)is the recommended, modern, and safe way to check if a property is defined directly on an object (as an "own" property) without traversing its prototype chain [3][4][5]. UsingObject.hasOwn()helps mitigate risks related to prototype pollution and avoids common pitfalls associated with older methods [4][6][5]: - It works correctly on objects created withObject.create(null)(which do not inherit fromObject.prototypeand therefore lack thehasOwnPropertymethod) [4][7]. - It is immune to issues where an object might have a shadowed or overriddenhasOwnPropertyproperty [4][7]. For codebases that need to support older environments, the traditional workaround isObject.prototype.hasOwnProperty.call(obj, key), butObject.hasOwn()is preferred wherever ES2022+ is supported [3][5][8]. When iterating over object keys, you should also prefer methods that only access own properties, such asObject.keys(),Object.values(),Object.entries(), orfor...ofloops, over thefor...inloop, which also traverses the prototype chain [9][6].Citations:
Avoid prototype-chain key traversal in annotation-path resolution
Line 48’s
segment in propertiesmatches inherited keys via the prototype chain (e.g.__proto__,constructor). That can cause the traversal to setnodeto a prototype object and then mutate it at lines 54-55. Switching to an own-property check prevents this.Proposed fix
function applySource(specSchema: unknown, path: string[], source: string): void { let node = specSchema for (const segment of path) { const properties = (node as { properties?: Record<string, unknown> })?.properties - if (!properties || typeof properties !== "object" || !(segment in properties)) { + if ( + !properties || + typeof properties !== "object" || + segment === "__proto__" || + segment === "prototype" || + segment === "constructor" || + !Object.hasOwn(properties, segment) + ) { return } node = properties[segment] } if (node && typeof node === "object") { ;(node as Record<string, unknown>)["x-cozystack-options"] = { source } } }Add a regression test to ensure
options.cozystack.io/source.__proto__is ignored.📝 Committable suggestion
🤖 Prompt for AI Agents