fix(console): reattach backup dropdown options from CRD annotations#40
Conversation
github-actions
Bot
added
area/console
|
Warning Review limit reached
More reviews will be available in 53 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds a utility to restore option-source metadata lost when CRD specs are fetched from the API server. Annotations with the ChangesOption source grafting from CRD annotations
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a utility to graft x-cozystack-options vendor keywords back onto a CRD's spec schema from its metadata annotations, bypassing API server limitations that strip non-kubernetes extensions. This utility is integrated into the useCRDSchema hook. A security review identified a potential prototype pollution vulnerability in the path traversal logic of applySource when using the in operator, and recommended explicitly blocking sensitive keys like __proto__ and constructor while using hasOwnProperty for safe property checking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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] | ||
| } |
There was a problem hiding this comment.
Using the in operator to traverse the schema path is vulnerable to Prototype Pollution. If an annotation key contains __proto__ or constructor, it can traverse to and mutate Object.prototype, affecting all objects in the application.
To prevent this, we should explicitly block sensitive keys like __proto__ and constructor, and use Object.prototype.hasOwnProperty.call instead of the in operator to ensure we only traverse own properties of the schema.
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]
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/console/src/lib/crd-option-sources.ts`:
- Around line 47-49: The current check uses "segment in properties" which
matches inherited prototype keys; change it to an own-property check (e.g.
Object.prototype.hasOwnProperty.call(properties, segment) or Object.hasOwn) so
only direct keys are used when resolving the annotation path and reassigning
node (the variables: properties, segment, node in crd-option-sources.ts). Update
the traversal logic that sets/mutates node to only proceed when the key is an
own property, and add a regression test asserting that an annotation like
"options.cozystack.io/source.__proto__" is ignored (i.e. does not create or
mutate prototype properties).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a00b7d5-21cd-4f17-9010-03ec9e8ee294
📒 Files selected for processing (3)
apps/console/src/lib/crd-option-sources.test.tsapps/console/src/lib/crd-option-sources.tsapps/console/src/lib/use-crd-schema.ts
| const properties = (node as { properties?: Record<string, unknown> })?.properties | ||
| if (!properties || typeof properties !== "object" || !(segment in properties)) { | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, does key in objinclude prototype-chain properties likeprotoandconstructor, and is Object.hasOwn(obj, key) the recommended safe check to prevent prototype-pollution-style key traversal?
💡 Result:
Yes, the in operator 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 obj will return true for inherited properties such as proto, 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]. Using Object.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 with Object.create(null) (which do not inherit from Object.prototype and therefore lack the hasOwnProperty method) [4][7]. - It is immune to issues where an object might have a shadowed or overridden hasOwnProperty property [4][7]. For codebases that need to support older environments, the traditional workaround is Object.prototype.hasOwnProperty.call(obj, key), but Object.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 as Object.keys(), Object.values(), Object.entries(), or for...of loops, over the for...in loop, which also traverses the prototype chain [9][6].
Citations:
- 1: https://stackoverflow.com/questions/13632999/ifkey-in-object-or-ifobject-hasownpropertykey
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in
- 3: https://skillstuff.com/understanding-hasownproperty-and-object-hasown-no-pitfalls/
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty
- 6: https://github.com/mdn/content/blob/main/files/en-us/web/security/attacks/prototype_pollution/index.md
- 7: https://2ality.com/2021/06/object-hasown.html
- 8: https://eslint.org/docs/latest/rules/prefer-object-has-own
- 9: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
Avoid prototype-chain key traversal in annotation-path resolution
Line 48’s segment in properties matches inherited keys via the prototype chain (e.g. __proto__, constructor). That can cause the traversal to set node to 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const properties = (node as { properties?: Record<string, unknown> })?.properties | |
| if (!properties || typeof properties !== "object" || !(segment in properties)) { | |
| return | |
| 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 === "__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 } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/src/lib/crd-option-sources.ts` around lines 47 - 49, The current
check uses "segment in properties" which matches inherited prototype keys;
change it to an own-property check (e.g.
Object.prototype.hasOwnProperty.call(properties, segment) or Object.hasOwn) so
only direct keys are used when resolving the annotation path and reassigning
node (the variables: properties, segment, node in crd-option-sources.ts). Update
the traversal logic that sets/mutates node to only proceed when the key is an
own property, and add a regression test asserting that an annotation like
"options.cozystack.io/source.__proto__" is ignored (i.e. does not create or
mutate prototype properties).
The backup forms drive the backupClassName, planRef.name and applicationRef.kind dropdowns from the CRD's x-cozystack-options keyword via DynamicOptionsWidget. apiextensions strips that vendor extension from the live CRD schema, so the keyword never reached the form and the dropdowns degraded to plain text inputs. The backups.cozystack.io CRDs now carry the field-to-source mapping in metadata.annotations. Graft the x-cozystack-options keyword back onto the spec schema from those annotations in useCRDSchema, so DynamicOptionsWidget binds again. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
fix/backup-dropdowns-from-crd-annotations
branch
from
46cfd90 to
80fcfb2
Compare
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Cross-checked the contract against cozystack/cozystack#2823: prefix, dotted spec-relative paths, and source values all match, and every path the four CRDs declare resolves through plain object properties, which is exactly what the walker handles. The rollout-order independence holds up — absent annotations and unresolvable paths are both no-ops, so the UI can ship before or after the CRD change.
Two non-blocking notes:
- The walker only descends through
properties, so a future annotation path crossingitemsoradditionalPropertieswould silently no-op. Worth one line in the contract doc comment so the constraint is explicit for whoever adds the next annotation. - The annotations are CRD-metadata-level while the schema is per-version (storage version picked); a single mapping can't distinguish versions if a field ever moves. Theoretical today with single-version CRDs.
…chema (#2823) ## What this PR does Two changes that together fix the v1.5.0 release blocker where the `cozy-backup-controller` HelmRelease failed on every reconcile. **Root cause** — the four `backups.cozystack.io` CRDs (`backupjobs`, `backups`, `plans`, `restorejobs`) embedded the `x-cozystack-options` vendor extension directly in their OpenAPI validation schema. `apiextensions` `JSONSchemaProps` only preserves the documented `x-kubernetes-*` vendor extensions — every other `x-` key is rejected by helm-controller's server-side apply (`field not declared in schema`) and dropped by the apiserver on decode. So the CRDs never applied and the controller crash-looped. This moves the field→source mapping the dashboard needs out of the schema and into CRD `metadata.annotations` (which the apiserver preserves), emitted declaratively via `+kubebuilder:metadata:annotations` markers on the Go types, and removes the post-processing awk injector (`hack/inject-cozystack-options.awk`). A helm-unittest suite pins the contract: each dropdown field carries its source in annotations and must not carry `x-cozystack-options` in the schema. The dashboard reattaches the extension client-side from these annotations. Annotation contract: `options.cozystack.io/source.<spec-relative-path>: <option-source>`, e.g. `options.cozystack.io/source.applicationRef.kind: appkind`. **Detection gap** — the install e2e let this ship green: the fanned-out `kubectl wait` discarded child exit codes (a bare POSIX `wait` returns 0) and the readiness check printed offending HelmReleases without ever exiting non-zero. The gate is now restored — a single `kubectl wait hr --all -A` for trace visibility, an outcome-based re-list that also gates late-created HelmReleases (with a short retry to absorb momentary drift-reconcile flaps) and `exit 1`s, plus a full Ready-condition dump per non-ready HelmRelease so the real failure reason is visible. This commit is by @myasnikovdaniil, consolidated here from #2824 so the root cause and the CI gap land in one PR. With the gate restored, CI on `main` stays red until the backups fix in this PR merges — intended. Closes #2822 · Dashboard counterpart: cozystack/cozystack-ui#40 ### Release note ```release-note fix(backups): fix backup-controller CRDs failing to apply — the dashboard dropdown hint was placed in the CRD schema where apiextensions rejects it; it now lives in CRD annotations ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a CRD option-source test suite and a package test target to validate annotation-based option sources. * **Bug Fixes** * Improved end-to-end install checks with consolidated readiness waits and richer failure diagnostics for releases. * **Refactor** * Migrated backup-controller CRD option metadata from legacy vendor extensions to annotation-based keys for cleaner schema metadata. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
What this PR does
The backup create forms drive the
backupClassName,planRef.nameandapplicationRef.kinddropdowns from the CRD'sx-cozystack-optionskeyword viaDynamicOptionsWidget.apiextensionsstrips that vendor extension from the live CRD schema, so the keyword never reached the form and those fields degraded to plain text inputs.The
backups.cozystack.ioCRDs now carry the field→source mapping inmetadata.annotations(options.cozystack.io/source.<path>). This PR grafts thex-cozystack-optionskeyword back onto the parsed spec schema from those annotations inuseCRDSchema, soDynamicOptionsWidgetbinds again. New pure helpergraftOptionSourceswith unit tests; no new dependencies.Requires the CRD annotations from cozystack/cozystack#2823 to be present in the cluster; the graft is a safe no-op when they are absent.
Summary by CodeRabbit
New Features
Tests