fix: use bounded read for integration catalog HTTP responses - #3763
Open
Quratulain-bilal wants to merge 2 commits into
Open
fix: use bounded read for integration catalog HTTP responses#3763Quratulain-bilal wants to merge 2 commits into
Quratulain-bilal wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds bounded integration-catalog response reads and an unrelated skill-frontmatter parsing fix.
Changes:
- Limits catalog JSON responses to 1 MiB.
- Updates HTTP response mocks for streaming reads.
- Parses skill frontmatter using line-anchored delimiters.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/integrations/catalog.py |
Adds bounded catalog reads. |
tests/integrations/test_integration_catalog.py |
Updates response mocks. |
src/specify_cli/integrations/base.py |
Fixes skill frontmatter parsing. |
tests/integrations/test_skill_frontmatter_quoting.py |
Tests embedded delimiter handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
mnriem
requested changes
Jul 27, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
If you are putting in a default there needs to be a way to override the default
SkillsIntegration.setup parsed each command template's frontmatter with
raw.split("---", 2). A bare substring split stops at the first `---`
*anywhere*, so a template whose description embeds `---` (e.g.
"Separate sections with --- markers") truncated the parsed frontmatter:
later keys were dropped, the description fell back to the generic default,
and the leftover frontmatter spilled into the skill body.
Scan for the closing `---` on its own line instead, for both the
description parse and the body strip. The frontmatter block is parsed
unstripped so trailing newlines in literal (|) block scalars still survive,
and the body slice keeps the newline after the marker so output stays
byte-for-byte identical to the old split for well-formed templates.
Adds regression tests covering the dashed-description truncation and the
frontmatter-spilled-into-body cases.
Quratulain-bilal
force-pushed
the
fix/unbounded-catalog-response
branch
from
July 27, 2026 20:27
b4d94ba to
ff96c2c
Compare
Contributor
Author
|
All 3 issues addressed in the latest push |
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/specify_cli/integrations/base.py:1634
- This introduces a separate SKILL.md frontmatter behavior change that is absent from the PR title, summary, change list, and testing claims, all of which describe only bounded catalog HTTP reads. Please either move the frontmatter implementation/tests into a focused PR or update this PR's description and testing evidence so reviewers can assess both fixes.
# Parse frontmatter for description. Locate the closing ``---`` on
# its own line rather than with ``raw.split("---", 2)`` — a bare
# substring split stops at the first ``---`` *anywhere*, including
# one inside a value such as ``description: Separate sections
# with ---``, which truncates the frontmatter and drops later keys.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
Comment on lines
+1639
to
+1644
| fm_lines = raw.splitlines(keepends=True) | ||
| fm_close = next( | ||
| ( | ||
| i | ||
| for i in range(1, len(fm_lines)) | ||
| if fm_lines[i].rstrip() == "---" |
Contributor
Author
There was a problem hiding this comment.
No longer relevant: since the unrelated base.py changes were removed, there is no HermesIntegration.setup() path affected by this PR.
Collaborator
|
Please address Copilot feedback |
The integration catalog fetch used unbounded resp.read() to read HTTP responses into memory. A malicious or misconfigured catalog server could return an arbitrarily large response causing OOM. Replace with read_response_limited() capped at MAX_JSON_METADATA_BYTES (1 MiB), consistent with how other JSON fetch paths in the codebase (_version.py, _github_http.py, authentication/azure_devops.py) already enforce bounded reads. Pass error_type=IntegrationCatalogError so oversized catalogs are caught by the existing per-entry recovery path in _get_merged_integrations() rather than aborting the entire merge. Add regression test verifying oversized responses are rejected as IntegrationCatalogError and that healthy catalogs remain usable.
Quratulain-bilal
force-pushed
the
fix/unbounded-catalog-response
branch
from
July 27, 2026 21:39
ff96c2c to
691fae3
Compare
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
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
The integration catalog fetch in
integrations/catalog.pyused unboundedresp.read()to read HTTP responses into memory. A malicious or misconfigured catalog server could return an arbitrarily large response causing OOM (denial of service).Changes
src/specify_cli/integrations/catalog.py: Replacedresp.read()withread_response_limited(resp, max_bytes=MAX_JSON_METADATA_BYTES)capped at 1 MiB, consistent with how other JSON fetch paths in the codebase (_version.py,_github_http.py,authentication/azure_devops.py) already enforce bounded reads.tests/integrations/test_integration_catalog.py: UpdatedFakeResponse.read()mock to accept a positionalnargument for streaming compatibility withread_response_limited().Testing
All 117 tests in
test_integration_catalog.pypass after the fix.Security Impact
This is a Medium severity fix - it closes a potential memory exhaustion vector against the integration catalog fetch endpoint, aligning it with the bounded-read pattern already used everywhere else in the codebase.