Conversation
WalkthroughThe pull request modifies the Keycloak configuration template to introduce a new variable Changes
Sequence DiagramsequenceDiagram
participant Config as CozyConfig
participant Template as Keycloak Template
participant Client as Kubeapps Client
Config->>Template: Provide extra redirect URIs
Template->>Template: Split and process extra URIs
Template->>Client: Append additional redirect URIs
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/system/keycloak-configure/templates/configure-kk.yaml (2)
3-3: Consider enhancing the URI parsing and validation.While the implementation works, there are a few considerations:
- Using '|' as a separator might be problematic if URIs contain this character.
- No validation is performed on the URIs, which could lead to invalid redirect URIs being added.
- The new configuration option
extra-keycloak-redirect-uri-for-dashboardneeds documentation.Consider adding validation and documentation:
{{- /* Validate and split additional redirect URIs for the dashboard */}} {{- $rawUris := (index $cozyConfig.data "extra-keycloak-redirect-uri-for-dashboard") | default "" }} + {{- /* Validate URI format */}} + {{- if $rawUris }} + {{- if not (regexMatch "^(https?://[^|]+)+(\\|https?://[^|]+)*$" $rawUris) }} + {{- fail "extra-keycloak-redirect-uri-for-dashboard must contain valid HTTP(S) URIs separated by '|'" }} + {{- end }} + {{- end }} {{- $extraRedirectUris := splitList "|" $rawUris }}
Line range hint
1-196: Update PR title to be more descriptive.The current PR title "fix" does not provide any context about the changes being made. Consider updating it to something more descriptive like:
"feat(keycloak): Add support for additional dashboard redirect URIs"🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| {{- range $uri := $extraRedirectUris }} | ||
| - "{{ $uri }}" | ||
| {{- end }} |
There was a problem hiding this comment.
Security: Consider implementing safeguards for redirect URIs.
The current implementation allows arbitrary redirect URIs to be added without any restrictions, which could potentially lead to security vulnerabilities if not properly controlled.
Consider implementing the following safeguards:
- Add a limit to the number of additional redirect URIs.
- Implement URI pattern validation to ensure only allowed domains are added.
- Add deduplication to prevent duplicate entries.
- Consider using an allowlist approach for additional domains.
Example implementation:
{{- /* Add additional redirect URIs with safeguards */}}
+ {{- $maxExtraUris := 5 }}
+ {{- $allowedDomains := list (printf ".%s$" $host) }}
+ {{- $seen := dict }}
{{- range $uri := $extraRedirectUris }}
+ {{- if gt (len $extraRedirectUris) $maxExtraUris }}
+ {{- fail (printf "Number of extra redirect URIs exceeds maximum allowed (%d)" $maxExtraUris) }}
+ {{- end }}
+ {{- $valid := false }}
+ {{- range $domain := $allowedDomains }}
+ {{- if regexMatch $domain $uri }}
+ {{- $valid = true }}
+ {{- end }}
+ {{- end }}
+ {{- if not $valid }}
+ {{- fail (printf "URI %s is not from an allowed domain" $uri) }}
+ {{- end }}
+ {{- if not (hasKey $seen $uri) }}
- "{{ $uri }}"
+ {{- $_ := set $seen $uri true }}
+ {{- end }}
{{- end }}Committable suggestion skipped: line range outside the PR's diff.
Merge the AI-generated v1.5.0 changelog with a fuller pass: - Add an "Upgrade Notes and Required Actions" section (K8s 1.33+, removal of upgrade.force, GPU permittedHostDevices ownership, MetalLB FRR-K8s / HTTPS metrics, automatic TLS on externally-published services). - Expand "Platform Components" with per-component upstream highlights and CVE references (Flux v2.8 CVE-2026-23990, MetalLB v0.16 CVE-2025-22874), plus the new frr-k8s subchart and kuberture package. - Scope the storageClass-immutability entry (#2639) to UI-only enforcement, with apiserver enforcement tracked in #2657. - Broaden Development/Testing/CI coverage and add the v1.4.x website docs backports (#565, #566, #567, #580). - Correct the loop-device LVM-filter attribution to talm#215 (the prior ansible-cozystack#51 reference was a dependency bump, not this change). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Summary by CodeRabbit