OSAC-3609: restrict public filters to public object fields - #167
Open
wgordon17 wants to merge 17 commits into
Open
OSAC-3609: restrict public filters to public object fields#167wgordon17 wants to merge 17 commits into
wgordon17 wants to merge 17 commits into
Conversation
|
@wgordon17: This pull request references OSAC-3609 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@hack_full_comment.txt |
wgordon17
force-pushed
the
fix/OSAC-3609-public-filter-oracle
branch
from
August 6, 2026 14:27
7afe40a to
6c51e1c
Compare
wgordon17
force-pushed
the
fix/OSAC-3609-public-filter-oracle
branch
from
August 7, 2026 16:00
6c51e1c to
fa4c77f
Compare
wgordon17
marked this pull request as ready for review
August 7, 2026 16:01
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.go (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting this function into per-domain registration helpers.
The function is about 860 lines and carries a
//nolint:gocyclosuppression. Each block repeats the same builder shape. Grouping registrations into small helpers, for exampleregisterComputeServers,registerNetworkingServers, andregisterIdentityServers, keeps the same behavior and removes the suppression. This is not a blocker for this PR.Also applies to: 911-915
🤖 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 `@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.go` around lines 52 - 56, Split RegisterResourceServers into per-domain registration helpers such as registerComputeServers, registerNetworkingServers, and registerIdentityServers, grouping related public/private server registrations while preserving their order, dependencies, and behavior. Have the helpers use the existing context, grpc.ServiceRegistrar, and ResourceServerDeps, propagate registration errors, and remove the //nolint:gocyclo suppression once the top-level function is simplified.fulfillment-service/internal/servers/clusters_server.go (1)
181-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
objectDescalready computed in this function.Line 150 binds
objectDescto the same publicClusterdescriptor.network_classes_server.goreusesncDescriptorfor exactly this call. Reuse keeps one source of truth.♻️ Proposed change
- SetFilterDesc((*publicv1.Cluster)(nil).ProtoReflect().Descriptor()). + SetFilterDesc(objectDesc).🤖 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 `@fulfillment-service/internal/servers/clusters_server.go` at line 181, Update the SetFilterDesc call in the cluster server function to reuse the existing objectDesc variable computed earlier, instead of recomputing the public Cluster descriptor via ProtoReflect. Keep objectDesc as the single source of truth for this descriptor.fulfillment-service/internal/servers/private_baremetal_instance_templates_server.go (1)
76-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the doc comment reference to "the O generic parameter".
PrivateBareMetalInstanceTemplatesServerBuilderis not generic. The wording comes from the generic DAO builder and does not apply here. State the concrete fallback instead. The same text is repeated in the other private server builders, so apply the wording change consistently.♻️ Proposed wording
-// SetFilterDesc sets the protobuf message descriptor used to validate and translate CEL filter -// expressions. This is optional. When unset, the descriptor of the O generic parameter is used. +// SetFilterDesc sets the protobuf message descriptor used to validate and translate CEL filter +// expressions. This is optional. When unset, the descriptor of the private BareMetalInstanceTemplate +// message is used.🤖 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 `@fulfillment-service/internal/servers/private_baremetal_instance_templates_server.go` around lines 76 - 81, Update the SetFilterDesc doc comments on PrivateBareMetalInstanceTemplatesServerBuilder and the other private server builders to remove the invalid “O generic parameter” reference and describe the concrete descriptor fallback used by these non-generic builders. Apply the same corrected wording consistently across all repeated comments.fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go (1)
358-372: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a positive control for each discovered
Listrequest.All discovered list item messages contain
id, and theirfilterfields are optional. Invoke the same RPC withthis.id != this.idand expect no error. Also assert a filter-specific diagnostic in the rejection message, such as the private-only field name, to distinguishErrInvalidFilterfrom anotherInvalidArgumenterror.🤖 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 `@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go` around lines 358 - 372, Extend the table-driven tests around the list RPC cases to invoke each request with the valid filter expression “this.id != this.id” and assert the call succeeds. In the existing private-field rejection assertions, also verify the InvalidArgument status message includes the referenced private-only field name, distinguishing filter validation from unrelated errors.
🤖 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
`@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.go`:
- Around line 698-713: In RegisterResourceServers, replace the unchecked
deps.Notifier assertions with a checked type assertion to *database.Notifier and
return a descriptive error when it fails. Store the validated notifier in a
local dbNotifier variable and reuse it for the ExternalIPPools, Externals, and
related server builders at the other assertion sites.
In
`@fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 589-603: Remove or deprecate the Helm chart logic that emits the
three --vault-* arguments when .Values.vault.endpoint is configured. Update the
chart’s start grpc-server argument generation near the deployment command,
leaving unrelated arguments unchanged, so the command matches the flags
supported by start grpc-server and Vault-enabled deployments start successfully.
In `@fulfillment-service/internal/database/dao/filter_translator.go`:
- Around line 1014-1020: The map-field translation in the IsMap branch of
filterTranslator must stop embedding fieldName via fmt.Sprintf. Carry the JSON
key as a bound query argument, emit the appropriate placeholder through the DAO
query builder, and preserve filterTranslatorMapKind and max precedence so the
generated parameterized SQL still accesses the intended JSON object.
---
Nitpick comments:
In
`@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go`:
- Around line 358-372: Extend the table-driven tests around the list RPC cases
to invoke each request with the valid filter expression “this.id != this.id” and
assert the call succeeds. In the existing private-field rejection assertions,
also verify the InvalidArgument status message includes the referenced
private-only field name, distinguishing filter validation from unrelated errors.
In
`@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.go`:
- Around line 52-56: Split RegisterResourceServers into per-domain registration
helpers such as registerComputeServers, registerNetworkingServers, and
registerIdentityServers, grouping related public/private server registrations
while preserving their order, dependencies, and behavior. Have the helpers use
the existing context, grpc.ServiceRegistrar, and ResourceServerDeps, propagate
registration errors, and remove the //nolint:gocyclo suppression once the
top-level function is simplified.
In `@fulfillment-service/internal/servers/clusters_server.go`:
- Line 181: Update the SetFilterDesc call in the cluster server function to
reuse the existing objectDesc variable computed earlier, instead of recomputing
the public Cluster descriptor via ProtoReflect. Keep objectDesc as the single
source of truth for this descriptor.
In
`@fulfillment-service/internal/servers/private_baremetal_instance_templates_server.go`:
- Around line 76-81: Update the SetFilterDesc doc comments on
PrivateBareMetalInstanceTemplatesServerBuilder and the other private server
builders to remove the invalid “O generic parameter” reference and describe the
concrete descriptor fallback used by these non-generic builders. Apply the same
corrected wording consistently across all repeated comments.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35cd2db3-6491-4132-857f-cf55d452a373
📒 Files selected for processing (76)
fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.gofulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/database/dao/dao_errors.gofulfillment-service/internal/database/dao/dao_errors_test.gofulfillment-service/internal/database/dao/filter_translator.gofulfillment-service/internal/database/dao/filter_translator_test.gofulfillment-service/internal/database/dao/generic_dao.gofulfillment-service/internal/database/dao/generic_dao_list.gofulfillment-service/internal/database/dao/generic_dao_test.gofulfillment-service/internal/servers/bare_metal_instance_types_server.gofulfillment-service/internal/servers/baremetal_instance_catalog_items_server.gofulfillment-service/internal/servers/baremetal_instance_templates_server.gofulfillment-service/internal/servers/baremetal_instances_server.gofulfillment-service/internal/servers/cluster_catalog_items_server.gofulfillment-service/internal/servers/cluster_templates_server.gofulfillment-service/internal/servers/cluster_versions_server.gofulfillment-service/internal/servers/clusters_server.gofulfillment-service/internal/servers/compute_instance_catalog_items_server.gofulfillment-service/internal/servers/compute_instance_templates_server.gofulfillment-service/internal/servers/compute_instances_server.gofulfillment-service/internal/servers/events_server_test.gofulfillment-service/internal/servers/external_ip_attachments_server.gofulfillment-service/internal/servers/external_ip_pools_server.gofulfillment-service/internal/servers/external_ips_server.gofulfillment-service/internal/servers/generic_server.gofulfillment-service/internal/servers/generic_server_test.gofulfillment-service/internal/servers/host_types_server.gofulfillment-service/internal/servers/identity_providers_server.gofulfillment-service/internal/servers/instance_types_server.gofulfillment-service/internal/servers/nat_gateways_server.gofulfillment-service/internal/servers/network_classes_server.gofulfillment-service/internal/servers/private_baremetal_instance_catalog_items_server.gofulfillment-service/internal/servers/private_baremetal_instance_templates_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instances_server.gofulfillment-service/internal/servers/private_cluster_catalog_items_server.gofulfillment-service/internal/servers/private_cluster_templates_server.gofulfillment-service/internal/servers/private_cluster_versions_server.gofulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_compute_instance_catalog_items_server.gofulfillment-service/internal/servers/private_compute_instance_templates_server.gofulfillment-service/internal/servers/private_compute_instances_server.gofulfillment-service/internal/servers/private_external_ip_attachments_server.gofulfillment-service/internal/servers/private_external_ip_pools_server.gofulfillment-service/internal/servers/private_external_ips_server.gofulfillment-service/internal/servers/private_host_types_server.gofulfillment-service/internal/servers/private_hubs_server.gofulfillment-service/internal/servers/private_identity_providers_server.gofulfillment-service/internal/servers/private_instance_types_server.gofulfillment-service/internal/servers/private_nat_gateways_server.gofulfillment-service/internal/servers/private_network_classes_server.gofulfillment-service/internal/servers/private_project_memberships_server.gofulfillment-service/internal/servers/private_projects_server.gofulfillment-service/internal/servers/private_role_bindings_server.gofulfillment-service/internal/servers/private_roles_server.gofulfillment-service/internal/servers/private_secrets_server.gofulfillment-service/internal/servers/private_security_groups_server.gofulfillment-service/internal/servers/private_storage_backends_server.gofulfillment-service/internal/servers/private_storage_tiers_server.gofulfillment-service/internal/servers/private_subnets_server.gofulfillment-service/internal/servers/private_tenants_server.gofulfillment-service/internal/servers/private_users_server.gofulfillment-service/internal/servers/private_virtual_networks_server.gofulfillment-service/internal/servers/project_memberships_server.gofulfillment-service/internal/servers/projects_server.gofulfillment-service/internal/servers/role_bindings_server.gofulfillment-service/internal/servers/roles_server.gofulfillment-service/internal/servers/secrets_server.gofulfillment-service/internal/servers/security_groups_server.gofulfillment-service/internal/servers/subnets_server.gofulfillment-service/internal/servers/tenants_server.gofulfillment-service/internal/servers/users_server.gofulfillment-service/internal/servers/users_server_test.gofulfillment-service/internal/servers/virtual_networks_server.gofulfillment-service/internal/testing/testing_server.go
wgordon17
force-pushed
the
fix/OSAC-3609-public-filter-oracle
branch
from
August 7, 2026 20:17
fa4c77f to
3610ccc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go (2)
378-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the positive-control field from the descriptor instead of hardcoding
id.Line 382 assumes every discovered public resource message has an
idfield. The rest of this file is deliberately self-updating through protoreflect. A future public resource withoutidfails this positive control withInvalidArgument, and the failure reads like a filter-oracle regression rather than a missing field. Pick the first public scalar field fromtestCaseand build the expression from it, or assert the presence ofidexplicitly so the failure names the real cause.🤖 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 `@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go` around lines 378 - 389, Update the positive-control test in the dedupeByResource loop to avoid assuming every resource descriptor contains an id field. Derive the filter field name from testCase.requestDesc by selecting the first public scalar field, then build the valid filter expression with that field; alternatively, explicitly validate id exists before constructing the request so failures identify the missing descriptor field.
166-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass an isolated Prometheus registerer to the test.
RegisterResourceServersforwardsMetricsRegistererto each builder, but a nil value disables metric creation instead of causing a panic. SetMetricsRegisterer: prometheus.NewRegistry()to exercise the production metric-registration path without sharing the default registry.🤖 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 `@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go` around lines 166 - 173, Update the RegisterResourceServers test setup to pass an isolated Prometheus registerer via MetricsRegisterer: prometheus.NewRegistry() in ResourceServerDeps. Keep the existing dependencies unchanged and use the new registry to exercise metric registration without relying on the default registry.
🤖 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.
Nitpick comments:
In
`@fulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.go`:
- Around line 378-389: Update the positive-control test in the dedupeByResource
loop to avoid assuming every resource descriptor contains an id field. Derive
the filter field name from testCase.requestDesc by selecting the first public
scalar field, then build the valid filter expression with that field;
alternatively, explicitly validate id exists before constructing the request so
failures identify the missing descriptor field.
- Around line 166-173: Update the RegisterResourceServers test setup to pass an
isolated Prometheus registerer via MetricsRegisterer: prometheus.NewRegistry()
in ResourceServerDeps. Keep the existing dependencies unchanged and use the new
registry to exercise metric registration without relying on the default
registry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e20c995-4840-482d-bf3a-7b22d32622f8
📒 Files selected for processing (36)
fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.gofulfillment-service/internal/cmd/service/start/grpcserver/register_servers_test.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/clusters_server.gofulfillment-service/internal/servers/private_baremetal_instance_catalog_items_server.gofulfillment-service/internal/servers/private_baremetal_instance_templates_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instances_server.gofulfillment-service/internal/servers/private_cluster_catalog_items_server.gofulfillment-service/internal/servers/private_cluster_templates_server.gofulfillment-service/internal/servers/private_cluster_versions_server.gofulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_compute_instance_catalog_items_server.gofulfillment-service/internal/servers/private_compute_instance_templates_server.gofulfillment-service/internal/servers/private_compute_instances_server.gofulfillment-service/internal/servers/private_external_ip_attachments_server.gofulfillment-service/internal/servers/private_external_ip_pools_server.gofulfillment-service/internal/servers/private_external_ips_server.gofulfillment-service/internal/servers/private_host_types_server.gofulfillment-service/internal/servers/private_hubs_server.gofulfillment-service/internal/servers/private_identity_providers_server.gofulfillment-service/internal/servers/private_instance_types_server.gofulfillment-service/internal/servers/private_nat_gateways_server.gofulfillment-service/internal/servers/private_network_classes_server.gofulfillment-service/internal/servers/private_project_memberships_server.gofulfillment-service/internal/servers/private_projects_server.gofulfillment-service/internal/servers/private_role_bindings_server.gofulfillment-service/internal/servers/private_roles_server.gofulfillment-service/internal/servers/private_secrets_server.gofulfillment-service/internal/servers/private_security_groups_server.gofulfillment-service/internal/servers/private_storage_backends_server.gofulfillment-service/internal/servers/private_storage_tiers_server.gofulfillment-service/internal/servers/private_subnets_server.gofulfillment-service/internal/servers/private_tenants_server.gofulfillment-service/internal/servers/private_users_server.gofulfillment-service/internal/servers/private_virtual_networks_server.go
🚧 Files skipped from review as they are similar to previous changes (34)
- fulfillment-service/internal/servers/private_secrets_server.go
- fulfillment-service/internal/servers/clusters_server.go
- fulfillment-service/internal/servers/private_roles_server.go
- fulfillment-service/internal/servers/private_subnets_server.go
- fulfillment-service/internal/servers/private_external_ip_pools_server.go
- fulfillment-service/internal/servers/private_network_classes_server.go
- fulfillment-service/internal/servers/private_project_memberships_server.go
- fulfillment-service/internal/servers/private_baremetal_instances_server.go
- fulfillment-service/internal/servers/private_compute_instance_catalog_items_server.go
- fulfillment-service/internal/servers/private_identity_providers_server.go
- fulfillment-service/internal/servers/private_users_server.go
- fulfillment-service/internal/servers/private_clusters_server.go
- fulfillment-service/internal/servers/private_virtual_networks_server.go
- fulfillment-service/internal/servers/private_hubs_server.go
- fulfillment-service/internal/servers/private_host_types_server.go
- fulfillment-service/internal/servers/private_compute_instance_templates_server.go
- fulfillment-service/internal/servers/private_role_bindings_server.go
- fulfillment-service/internal/servers/private_compute_instances_server.go
- fulfillment-service/internal/servers/private_nat_gateways_server.go
- fulfillment-service/internal/servers/private_tenants_server.go
- fulfillment-service/internal/servers/private_baremetal_instance_templates_server.go
- fulfillment-service/internal/servers/private_security_groups_server.go
- fulfillment-service/internal/servers/private_cluster_catalog_items_server.go
- fulfillment-service/internal/servers/private_cluster_templates_server.go
- fulfillment-service/internal/servers/private_baremetal_instance_types_server.go
- fulfillment-service/internal/servers/private_cluster_versions_server.go
- fulfillment-service/internal/servers/private_external_ips_server.go
- fulfillment-service/internal/servers/private_external_ip_attachments_server.go
- fulfillment-service/internal/cmd/service/start/grpcserver/register_servers.go
- fulfillment-service/internal/servers/private_instance_types_server.go
- fulfillment-service/internal/servers/private_storage_backends_server.go
- fulfillment-service/internal/servers/private_baremetal_instance_catalog_items_server.go
- fulfillment-service/internal/servers/private_projects_server.go
- fulfillment-service/internal/servers/private_storage_tiers_server.go
omer-vishlitzky
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
August 12, 2026 09:18
Auto-dismissed: only Prow labels gate merging
omer-vishlitzky
disabled auto-merge
August 12, 2026 09:18
7 tasks
… over O Assisted-by: Claude Code <noreply@anthropic.com>
…classify filter errors Assisted-by: Claude Code <noreply@anthropic.com>
…translation Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
…urces Assisted-by: Claude Code <noreply@anthropic.com>
…r API consistency Assisted-by: Claude Code <noreply@anthropic.com>
…etal-instance-types servers Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
…lter leaks Assisted-by: Claude Code <noreply@anthropic.com>
…field Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
…base Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
translateSelectJsonField built JSON key access via fmt.Sprintf("...'%s'...",
fieldName), embedding the field name straight into the SQL text. In practice
fieldName always comes from a CEL field selector, whose grammar already
restricts it to safe identifier characters, so this was never reachable with
attacker-controlled content. But the function had no local guarantee of that
and relied on it implicitly.
Escape fieldName through the existing translateString helper (the same one
translateIndex already uses for map bracket-index keys) and build the quoted
SQL literal explicitly, rather than trusting the caller.
Assisted-by: Claude Code <noreply@anthropic.com>
msgDesc.Fields().ByName returns nil when fieldName isn't a real field of msgDesc, and the code immediately called fieldDesc.IsMap() on the result, which panics on a nil descriptor. CEL compilation against the message descriptor already rejects unknown selectors before reaching this function, so this isn't reachable today, but the function shouldn't depend on that external guarantee to avoid a nil-pointer panic on its own filter-oracle path. Return an error the same way the sibling translateSelectThisMdField default case already does for an unknown metadata field, instead of panicking. Assisted-by: Claude Code <noreply@anthropic.com>
discoverFilterOracleCases built its pairs map by ranging over protoregistry.GlobalFiles, so the resulting cases came out in map-iteration order, which varies between runs. Sort by resourceName then fieldPath before returning so the generated spec order is stable. Assisted-by: Claude Code <noreply@anthropic.com>
wgordon17
force-pushed
the
fix/OSAC-3609-public-filter-oracle
branch
from
August 12, 2026 20:11
a0585b9 to
d6df36d
Compare
|
New changes are detected. LGTM label has been removed. |
|
🤖 Finished Review · ✅ Success · Started 8:12 PM UTC · Completed 8:33 PM UTC Commit: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -213,26 +232,24 @@ func (b *FilterTranslatorBuilder[O]) Build() (result *FilterTranslator[O], err e | |||
| } | |||
There was a problem hiding this comment.
[low] error-handling
When createCelEnv() fails, the original error is discarded and replaced with a generic 'failed to create CEL environment' message without wrapping the original error via %w. Diagnostic information from the CEL library is silently lost.
Suggested fix: Use err = fmt.Errorf("failed to create CEL environment: %w", err).
| // Create the filter translator: | ||
| filterTranslator, err := NewFilterTranslator[O](). | ||
| // Create the filter translator. The filter descriptor defaults to the object's own descriptor, but callers | ||
| // may override it via SetFilterDesc to restrict which fields are visible to filter expressions. |
There was a problem hiding this comment.
[low] fail-open
When GenericDAOBuilder.filterDesc is nil (the default), Build() falls back to the private object descriptor. A public server that forgets to call SetFilterDesc silently uses the private descriptor and exposes private fields through filters. Mitigated by the reflection-driven regression test in register_servers_test.go.
| SetLogger(logger). | ||
| SetPool(pool). | ||
| Build() | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
[low] incomplete-coverage
The Users resource is excluded from the auto-discovery regression test. A targeted test exists for keycloak_user_id, but future private-only User fields would require manual test additions rather than being auto-discovered.
Suggested fix: Extend the targeted test in users_server_test.go to use findPrivateOnlyPaths() discovery logic so all current and future private-only User fields are automatically covered.
| var filter string | ||
| filter, err = r.dao.filterTranslator.Translate(ctx, r.filter) | ||
| if err != nil { | ||
| err = &ErrInvalidFilter{Reason: err.Error()} |
There was a problem hiding this comment.
[low] error-wrapping-idiom
ErrInvalidFilter is constructed via string-flattening (err.Error()) rather than error-chain wrapping. This discards the original error chain and differs from the DAO's convention for other typed errors.
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
5 participants
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.
Summary
FilterTranslatoris now descriptor-driven instead of generic over the private proto type, so public-fronted resources configure it with the public descriptor and reject tenant filters referencing private-only fields at CEL compile time instead of leaking them through list-result timing/presencesecrets,cluster_versions,bare_metal_instance_types), and classified filter-translate failures asInvalidArgumentinstead ofInternalRelates to OSAC-2819
Summary by CodeRabbit
InvalidArgumenterrors instead of internal failures.