Fix CVE details page 400 error by respecting no-tags patterns#1164
Fix CVE details page 400 error by respecting no-tags patterns#1164nofaralfasi wants to merge 1 commit intotheforeman:developfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts scoped request pattern selection for Insights API forwarding so that GET requests consider both tagged and non-tagged patterns and pick the most specific regex, fixing CVE detail requests that should not have tags applied. Sequence diagram for CVE details scoped request pattern selectionsequenceDiagram
actor User
participant UI as Vulnerability_UI
participant Server as Foreman_Server
participant Forwarder as InsightsApiForwarder
participant Insights as Insights_API
User->>UI: Click CVE in Vulnerabilities view
UI->>Server: GET /api/vulnerability/v1/cves/CVE-2024-1234
Server->>Forwarder: forward_request(original_request, path)
Forwarder->>Forwarder: scope_request?(original_request, path)
Note over Forwarder: original_request.get? == true
Forwarder->>Forwarder: Find matching_patterns where
Forwarder->>Forwarder: pattern.test.match?(path)
Forwarder->>Forwarder: and (pattern.tag_name OR permissions[GET])
Forwarder->>Forwarder: Evaluate CVE_detail_pattern
Note over Forwarder: test: /api\/vulnerability\/v1\/cves\/[^/]+$/
Note over Forwarder: tag_name: nil
Note over Forwarder: permissions: { GET: true }
Forwarder->>Forwarder: Evaluate general_pattern
Note over Forwarder: test: /api\/vulnerability\/v1\/.*
Note over Forwarder: tag_name: tags
Note over Forwarder: permissions: { GET: true }
Forwarder->>Forwarder: Select most specific pattern by
Forwarder->>Forwarder: max [regex_length, tag_support_flag]
Note over Forwarder: CVE_detail_pattern chosen
Forwarder-->>Forwarder: tag_name = nil
alt tag_name is nil
Forwarder->>Insights: GET /api/vulnerability/v1/cves/CVE-2024-1234
else tag_name present (previous behavior)
Forwarder->>Insights: GET /api/vulnerability/v1/cves/CVE-2024-1234?tags=...
end
Insights-->>Forwarder: 200 OK, CVE details JSON
Forwarder-->>Server: CVE details JSON
Server-->>UI: CVE details JSON
UI-->>User: Render CVE synopsis, description, severity, packages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using regex source length as a proxy for specificity may produce surprising precedence in some edge cases; consider making precedence explicit (e.g., with a priority field) instead of relying on pattern length.
- Now that patterns without
tag_namecan override those withtag_name, it may be worth validating that any permission-only patterns inSCOPED_REQUESTSare still behaving as intended and are not accidentally suppressing tags for endpoints where they should be added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using regex source length as a proxy for specificity may produce surprising precedence in some edge cases; consider making precedence explicit (e.g., with a priority field) instead of relying on pattern length.
- Now that patterns without `tag_name` can override those with `tag_name`, it may be worth validating that any permission-only patterns in `SCOPED_REQUESTS` are still behaving as intended and are not accidentally suppressing tags for endpoints where they should be added.
## Individual Comments
### Comment 1
<location path="app/services/foreman_rh_cloud/insights_api_forwarder.rb" line_range="231-234" />
<code_context>
- # Choose the most specific pattern by regex source length for consistency
- # with required_permission_for behavior
+ # Choose the most specific pattern by regex source length
+ # This ensures patterns explicitly defined without tag_name take precedence
+ # over general patterns that do have tag_name
request_pattern = matching_patterns.max_by { |pattern| pattern[:test].source.length }
+
+ # Return the tag_name (may be nil if the most specific pattern doesn't support tags)
</code_context>
<issue_to_address>
**suggestion:** Consider tie-breaking to prefer tag-supporting patterns when specificity is equal
Because both permission-only and tag-supporting patterns now participate in `max_by`, ties on `regex.source.length` are currently broken only by definition order in `SCOPED_REQUESTS`. To avoid this order dependency, consider using a composite key like `[pattern[:test].source.length, pattern[:tag_name] ? 1 : 0]` (or the inverse, depending on desired behavior) so that tie-breaking between tag-supporting and non-tag patterns is explicit and deterministic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
I tested this and it fixes the issue.
Will leave it to @vkrizan to see if he agrees with bot's comments here. also the test failure seems related :)
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
pattern.dig(:permissions, 'GET')check assumes permission keys are strings; if any entries use symbol keys (e.g.:GET), this will silently skip them—consider normalizing or supporting both. - Since both
scope_request?andrequired_permission_fornow rely on similar pattern-selection semantics (specificity, permissions), consider extracting the matching/selection logic into a shared helper to avoid divergence over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `pattern.dig(:permissions, 'GET')` check assumes permission keys are strings; if any entries use symbol keys (e.g. `:GET`), this will silently skip them—consider normalizing or supporting both.
- Since both `scope_request?` and `required_permission_for` now rely on similar pattern-selection semantics (specificity, permissions), consider extracting the matching/selection logic into a shared helper to avoid divergence over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
If |
vkrizan
left a comment
vkrizan
left a comment
There was a problem hiding this comment.
Thanks for this fix. I've left some comments for possible simplification of the matching criteria.
| # This ensures patterns like api/vulnerability/v1/cves/[^/]+$ (GET without tags) | ||
| # take precedence over general patterns, while POST-only patterns are ignored. | ||
| matching_patterns = SCOPED_REQUESTS.select do |pattern| | ||
| pattern[:test].match?(path) && (pattern[:tag_name] || pattern.dig(:permissions, 'GET')) |
There was a problem hiding this comment.
I see that this combines two unrelated things: 1) that it needs to have a tag definition; or 2) it needs to have a GET permissions.
Could you elaborate on the requirement for testing the existence of a tag definition? Could there be just one requirement for GET permissions definition? Or is this kept for a backwards compatibility?
There was a problem hiding this comment.
Let me explain why I beleieve both conditions are needed.
Looking at the SCOPED_REQUESTS patterns, there are different categories:
Category A: Patterns with tag_name but NO permissions defined:
These patterns exist purely for tagging - they don't enforce permissions but do add tags to GET requests. Without the tag_name check, these would be excluded.
Category B: Patterns with GET permissions but NO tag_name:
These patterns explicitly say "this GET endpoint does NOT support tags". Without the GET permissions check, these would be excluded (the bug we're fixing).
If we only used pattern.dig(:permissions, 'GET'):
api/inventory/.* and api/tasks/.*would be excluded (breaking change)- GET requests to those endpoints would no longer get tags
So yes, it combines two concepts, but both are required to:
- Fix the CVE details bug (needs GET check)
- Maintain backwards compatibility for tagging-only patterns (needs
tag_namecheck)
There was a problem hiding this comment.
Thanks for the explanation!
| # Tie-break by preferring tag-supporting patterns when specificity is equal, | ||
| # to avoid order-dependent behavior in SCOPED_REQUESTS. | ||
| request_pattern = matching_patterns.max_by do |pattern| | ||
| [pattern[:test].source.length, pattern[:tag_name] ? 1 : 0] |
There was a problem hiding this comment.
This matching condition for preferring matches by tag definition could be simplified just to the rule of longest match on path. Could you elaborate on the need for that tag preference over simplification?
There was a problem hiding this comment.
You're right - the tie-breaking logic for preferring tag-supporting patterns adds unnecessary complexity.
The simple "longest match wins" rule is sufficient.
I checked and all permission entries in SCOPED_REQUESTS currently use string keys ('GET', 'POST', etc.), so |
The scope_request? method was incorrectly filtering to only consider patterns with tag_name defined, which caused the CVE details endpoint pattern (api/vulnerability/v1/cves/[^/]+$) to be excluded. This resulted in the more general pattern (api/vulnerability/v1/.*) being selected instead, which adds tags to the request. Since the CVE details API endpoint does not support tags per the OpenAPI spec, this caused a 400 Bad Request error: "Extra query parameter(s) tags not in spec" The fix removes the tag_name filter so all matching patterns are considered, then the most specific pattern (longest regex) is selected. For the CVE details endpoint, this correctly returns nil for tag_name, meaning no tags are added to the request. Fixes the "Cannot read properties of undefined (reading 'synopsis')" error that occurred when clicking on any CVE from the Vulnerabilities page or host details Vulnerabilities tab.
If it works as-is I'm fine without it. |
3 participants
The scope_request? method was incorrectly filtering to only consider patterns with tag_name defined, which caused the CVE details endpoint pattern (api/vulnerability/v1/cves/[^/]+$) to be excluded.
This resulted in the more general pattern (api/vulnerability/v1/.*) being selected instead, which adds tags to the request. Since the CVE details API endpoint does not support tags per the OpenAPI spec, this caused a 400 Bad Request error:
"Extra query parameter(s) tags not in spec"
What are the changes introduced in this pull request?
The fix removes the tag_name filter so all matching patterns are considered, then the most specific pattern (longest regex) is selected. For the CVE details endpoint, this correctly returns nil for tag_name, meaning no tags are added to the request.
Fixes the "Cannot read properties of undefined (reading 'synopsis')" error that occurred when clicking on any CVE from the Vulnerabilities page or host details Vulnerabilities tab.
What are the testing steps for this pull request?
Scenario 1:
Navigation Path:
Insights → Vulnerability → Click on any CVE
Scenario 2:
Navigation Path:
Hosts → All Hosts → client.example.com → Vulnerabilities → Click on any CVE
Expected Result:
Clicking on any CVE should open the Vulnerability details page and display full CVE information, including synopsis, description, severity, affected packages, etc.
Summary by Sourcery
Bug Fixes: