feat: Add dynamic JSON progress bar.#205
Conversation
|
@nimble-turtle is attempting to deploy a commit to the guibranco's projects Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideImplements a new /dynamic/json/ endpoint that safely fetches remote JSON, extracts a numeric progress value via JSONPath, and renders the existing SVG progress bar, along with SSRF/content-type safeguards, caching controls, tests, and documentation updates. Sequence diagram for dynamic JSON progress bar endpointsequenceDiagram
actor User
participant Browser
participant FlaskApp
participant FetchJsonOpener
participant RemoteJsonServer
participant JsonPathNg
participant Jinja2
User->>Browser: Request /dynamic/json/?url=...&query=...
Browser->>FlaskApp: HTTP GET /dynamic/json/
FlaskApp->>FlaskApp: Validate url and query params
alt Missing or disallowed url or query
FlaskApp->>Jinja2: Render dynamic_error.svg
Jinja2-->>FlaskApp: SVG error content
FlaskApp-->>Browser: 400/502/422 SVG error
Browser-->>User: Error progress bar image
else Allowed url and query
FlaskApp->>FetchJsonOpener: fetch_json_document(url)
FetchJsonOpener->>RemoteJsonServer: HTTP GET JSON
RemoteJsonServer-->>FetchJsonOpener: JSON response
FetchJsonOpener-->>FlaskApp: Parsed JSON document
FlaskApp->>JsonPathNg: extract_progress_with_jsonpath(doc, query)
JsonPathNg-->>FlaskApp: Numeric progress value
FlaskApp->>FlaskApp: get_template_fields(progress)
FlaskApp->>Jinja2: Render progress.svg
Jinja2-->>FlaskApp: SVG progress bar
FlaskApp->>FlaskApp: Apply _finalize_svg_response and cache headers
FlaskApp-->>Browser: 200 SVG response
Browser-->>User: Dynamic JSON progress bar image
end
Class diagram for no-redirect HTTP handler used in JSON fetchingclassDiagram
class HTTPRedirectHandler {
}
class _NoHttpRedirectHandler {
+redirect_request(req, fp, code, msg, headers, newurl)
}
_NoHttpRedirectHandler --|> HTTPRedirectHandler
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
gstraccini
Bot
added
enhancement
github-actions
Bot
added
the
size/XL
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
actor Client
participant App as App Server
participant HTTP as HTTP Fetcher
participant Extractor as JSONPath Extractor
participant Renderer as SVG Renderer
Client->>App: GET /dynamic/json/?url=...&query=...
App->>App: Validate & normalize URL (SSRF check)
App->>App: Normalize JSONPath/dot-notation query
App->>HTTP: Fetch JSON document (with timeout/size limits)
HTTP-->>App: JSON document (with Content-Type check)
App->>Extractor: Extract numeric value using selector
Extractor-->>App: Parsed numeric progress value
App->>Renderer: Render progress.svg with extracted value
Renderer-->>App: SVG response with Content-Type headers
App-->>Client: SVG image + cache headers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
 |
|
Overall Grade |
Security  Reliability Complexity Hygiene  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Secrets |  |
Apr 13, 2026 8:36p.m. | Review ↗ |
| Python |  |
Apr 13, 2026 8:36p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Failed to generate code suggestions for PR |
![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 6 issues, and left some high level feedback:
- In
get_progress_svg_dynamic_json,_finalize_svg_response(response)is called without using its return value, which is a bit confusing given that the function returns the response; consider either returning its result directly (as inget_progress_svg) or making_finalize_svg_responsemutate in place and returnNonefor consistency. - In
fetch_json_document, the response body is always decoded as UTF-8 regardless of any charset in theContent-Typeheader; if you expect external APIs with other encodings, you may want to respect the charset parameter or at least fail fast when a different charset is declared.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_progress_svg_dynamic_json`, `_finalize_svg_response(response)` is called without using its return value, which is a bit confusing given that the function returns the response; consider either returning its result directly (as in `get_progress_svg`) or making `_finalize_svg_response` mutate in place and return `None` for consistency.
- In `fetch_json_document`, the response body is always decoded as UTF-8 regardless of any charset in the `Content-Type` header; if you expect external APIs with other encodings, you may want to respect the charset parameter or at least fail fast when a different charset is declared.
## Individual Comments
### Comment 1
<location path="app.py" line_range="414-415" />
<code_context>
+ template_fields = get_template_fields(progress)
+ template = render_template("progress.svg", **template_fields)
+ response = make_response(template)
+ _finalize_svg_response(response)
+ cache_raw = request.args.get("cache") or request.args.get("cacheSeconds")
+ if cache_raw is not None:
</code_context>
<issue_to_address>
**suggestion:** Use the return value of `_finalize_svg_response` to avoid coupling to its mutating behavior.
Currently `_finalize_svg_response` mutates and returns the same response, so ignoring the return value happens to work. To avoid depending on that implementation detail and stay consistent with `get_progress_svg`, please assign the result:
```python
esponse = _finalize_svg_response(response)
```
This prevents future bugs if `_finalize_svg_response` is changed to return a new response object.
```suggestion
response = make_response(template)
response = _finalize_svg_response(response)
```
</issue_to_address>
### Comment 2
<location path="app.py" line_range="136" />
<code_context>
+ except ValueError:
+ pass
+ try:
+ for res in socket.getaddrinfo(host, None):
+ addr = res[4][0]
+ try:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider also blocking multicast addresses in the DNS resolution branch for consistency with the literal-IP checks.
Allowing hostnames that resolve to multicast addresses (e.g. 224.x.x.x or ff00::/8) while blocking them for literal IPs creates an inconsistent policy and may open an SSRF vector in some environments.
You can mirror the literal-IP check here by including `ip.is_multicast`:
```python
if (
ip.is_private
or ip.is_loopback
or ip.is_link_local
or ip.is_reserved
or ip.is_multicast
):
return False
```
Suggested implementation:
```python
if (
ip.is_private
or ip.is_loopback
or ip.is_link_local
or ip.is_reserved
or ip.is_multicast
):
return False
```
If the literal-IP branch already includes `ip.is_multicast`, no further changes are needed there. Ensure the `SEARCH` block above only matches the DNS-resolution branch (i.e., the one where `ip` is derived from `socket.getaddrinfo` results, such as `addr`), not the literal-IP parsing branch which should already contain `or ip.is_multicast`.
</issue_to_address>
### Comment 3
<location path="test_api.py" line_range="21" />
<code_context>
+def test_svg_response(client):
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that the dynamic JSON endpoint also sets `X-Content-Type-Options` to `nosniff`.
Since `/dynamic/json/` also returns SVG via `_finalize_svg_response`, please add a dedicated test (e.g. `test_dynamic_json_sets_nosniff_header`) that hits `/dynamic/json/` with a mocked `fetch_json_document` and asserts `X-Content-Type-Options == 'nosniff'` to guard against regressions.
</issue_to_address>
### Comment 4
<location path="test_api.py" line_range="52-61" />
<code_context>
+ assert b"73" in response.data
+
+
+@patch("app.fetch_json_document")
+def test_dynamic_json_jsonpath_dollar(mock_fetch, client):
+ mock_fetch.return_value = {"items": [{"n": 42}]}
+ response = client.get(
+ "/dynamic/json/",
+ query_string={
+ "url": "https://example.com/x.json",
+ "query": "$.items[0].n",
+ },
+ )
+ assert response.status_code == 200
+ assert b"42" in response.data
+
+
+@patch("app.fetch_json_document")
+def test_dynamic_json_percent_suffix_string(mock_fetch, client):
+ mock_fetch.return_value = {"approvalProgress": "73%"}
+ response = client.get(
</code_context>
<issue_to_address>
**suggestion (testing):** Cover additional `extract_progress_with_jsonpath` string-parsing edge cases (empty and non-numeric strings).
Please also add tests where the JSONPath result is (1) an empty or whitespace-only string (expect 422 with an "empty" error) and (2) a non-numeric string like "foo" (expect 422 with a "not numeric" error), so the branches raising "matched string is empty" and "matched string is not numeric" are covered.
</issue_to_address>
### Comment 5
<location path="test_api.py" line_range="217-36" />
<code_context>
+ assert response.status_code == 400
+
+
+def test_dynamic_json_nonstandard_port_blocked(client):
+ response = client.get(
+ "/dynamic/json/",
+ query_string={
+ "url": "https://example.com:8080/data.json",
+ "query": "$.x",
+ },
+ )
+ assert response.status_code == 400
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend URL-allowlist tests to cover long URLs and cloud-metadata hosts that are explicitly blocked.
Current tests nicely cover localhost, schemes, userinfo, and nonstandard ports. Since `url_is_allowed_for_fetch` also enforces a max URL length and blocks cloud-metadata hosts (e.g., `metadata.google.internal`, `169.254.169.254`), please add tests asserting that: (1) a URL longer than `JSON_URL_MAX_LENGTH` returns 400, and (2) requests to those metadata endpoints also return 400, so these protections are explicitly verified.
Suggested implementation:
```python
import pytest
from app import app, fetch_json_document, JSON_URL_MAX_LENGTH
```
```python
def test_dynamic_json_nonstandard_port_blocked(client):
response = client.get(
"/dynamic/json/",
query_string={
"url": "https://example.com:8080/data.json",
"query": "$.x",
},
)
assert response.status_code == 400
def test_dynamic_json_url_too_long_blocked(client):
# Construct a URL longer than JSON_URL_MAX_LENGTH
excessive_length = JSON_URL_MAX_LENGTH + 10
long_path = "a" * excessive_length
long_url = f"https://example.com/{long_path}.json"
response = client.get(
"/dynamic/json/",
query_string={
"url": long_url,
"query": "$.x",
},
)
assert response.status_code == 400
@pytest.mark.parametrize(
"metadata_url",
[
"http://169.254.169.254/latest/meta-data",
"http://metadata.google.internal/computeMetadata/v1",
],
)
def test_dynamic_json_cloud_metadata_hosts_blocked(client, metadata_url):
response = client.get(
"/dynamic/json/",
query_string={
"url": metadata_url,
"query": "$.x",
},
)
assert response.status_code == 400
```
</issue_to_address>
### Comment 6
<location path="test_api.py" line_range="228-237" />
<code_context>
+ assert response.status_code == 400
+
+
+@patch("app._fetch_json_opener.open")
+def test_fetch_json_rejects_html_content_type(mock_open):
+ mock_resp = MagicMock()
+ mock_resp.headers.get.return_value = "text/html; charset=utf-8"
+ mock_resp.read.return_value = b"{}"
+ ctx = MagicMock()
+ ctx.__enter__.return_value = mock_resp
+ ctx.__exit__.return_value = None
+ mock_open.return_value = ctx
+ with pytest.raises(ValueError, match="unsupported content type"):
+ fetch_json_document("https://example.com/x.json")
+
+
+@patch("app._fetch_json_opener.open")
+def test_fetch_json_allows_application_json(mock_open):
+ mock_resp = MagicMock()
+ mock_resp.headers.get.return_value = "application/json; charset=utf-8"
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `fetch_json_document` error branches: oversized responses, invalid UTF-8, and malformed JSON.
Currently only content-type rejection and happy-path JSON are tested. Please also cover the remaining error paths by mocking `read` to: (1) return `JSON_FETCH_MAX_BYTES + 1` bytes and assert `ValueError('response too large')`; (2) return invalid UTF-8 bytes and (3) syntactically invalid JSON (e.g. `b'{"n": }'`) and assert these are surfaced by the dynamic JSON view as 422 responses. This will ensure the size limit and parse failures are exercised end-to-end.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert response.headers.get("X-Content-Type-Options") == "nosniff" | ||
|
|
||
| assert b"<svg" in response.data | ||
| assert b"</svg>" in response.data |
There was a problem hiding this comment.
suggestion (testing): Add an assertion that the dynamic JSON endpoint also sets X-Content-Type-Options to nosniff.
Since /dynamic/json/ also returns SVG via _finalize_svg_response, please add a dedicated test (e.g. test_dynamic_json_sets_nosniff_header) that hits /dynamic/json/ with a mocked fetch_json_document and asserts X-Content-Type-Options == 'nosniff' to guard against regressions.
| @patch("app.fetch_json_document") | ||
| def test_dynamic_json_query_param(mock_fetch, client): | ||
| mock_fetch.return_value = {"v": 9} | ||
| response = client.get( | ||
| "/dynamic/json/", | ||
| query_string={ | ||
| "url": "https://example.com/x.json", | ||
| "query": "$.v", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Cover additional extract_progress_with_jsonpath string-parsing edge cases (empty and non-numeric strings).
Please also add tests where the JSONPath result is (1) an empty or whitespace-only string (expect 422 with an "empty" error) and (2) a non-numeric string like "foo" (expect 422 with a "not numeric" error), so the branches raising "matched string is empty" and "matched string is not numeric" are covered.
|
|
||
| def test_dynamic_json_missing_params(client): | ||
| response = client.get("/dynamic/json/") | ||
| assert response.status_code == 400 |
There was a problem hiding this comment.
suggestion (testing): Extend URL-allowlist tests to cover long URLs and cloud-metadata hosts that are explicitly blocked.
Current tests nicely cover localhost, schemes, userinfo, and nonstandard ports. Since url_is_allowed_for_fetch also enforces a max URL length and blocks cloud-metadata hosts (e.g., metadata.google.internal, 169.254.169.254), please add tests asserting that: (1) a URL longer than JSON_URL_MAX_LENGTH returns 400, and (2) requests to those metadata endpoints also return 400, so these protections are explicitly verified.
Suggested implementation:
import pytest
from app import app, fetch_json_document, JSON_URL_MAX_LENGTHdef test_dynamic_json_nonstandard_port_blocked(client):
response = client.get(
"/dynamic/json/",
query_string={
"url": "https://example.com:8080/data.json",
"query": "$.x",
},
)
assert response.status_code == 400
def test_dynamic_json_url_too_long_blocked(client):
# Construct a URL longer than JSON_URL_MAX_LENGTH
excessive_length = JSON_URL_MAX_LENGTH + 10
long_path = "a" * excessive_length
long_url = f"https://example.com/{long_path}.json"
response = client.get(
"/dynamic/json/",
query_string={
"url": long_url,
"query": "$.x",
},
)
assert response.status_code == 400
@pytest.mark.parametrize(
"metadata_url",
[
"http://169.254.169.254/latest/meta-data",
"http://metadata.google.internal/computeMetadata/v1",
],
)
def test_dynamic_json_cloud_metadata_hosts_blocked(client, metadata_url):
response = client.get(
"/dynamic/json/",
query_string={
"url": metadata_url,
"query": "$.x",
},
)
assert response.status_code == 400| @patch("app._fetch_json_opener.open") | ||
| def test_fetch_json_rejects_html_content_type(mock_open): | ||
| mock_resp = MagicMock() | ||
| mock_resp.headers.get.return_value = "text/html; charset=utf-8" | ||
| mock_resp.read.return_value = b"{}" | ||
| ctx = MagicMock() | ||
| ctx.__enter__.return_value = mock_resp | ||
| ctx.__exit__.return_value = None | ||
| mock_open.return_value = ctx | ||
| with pytest.raises(ValueError, match="unsupported content type"): |
There was a problem hiding this comment.
suggestion (testing): Add tests for fetch_json_document error branches: oversized responses, invalid UTF-8, and malformed JSON.
Currently only content-type rejection and happy-path JSON are tested. Please also cover the remaining error paths by mocking read to: (1) return JSON_FETCH_MAX_BYTES + 1 bytes and assert ValueError('response too large'); (2) return invalid UTF-8 bytes and (3) syntactically invalid JSON (e.g. b'{"n": }') and assert these are surfaced by the dynamic JSON view as 422 responses. This will ensure the size limit and parse failures are exercised end-to-end.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 2 minor |
🟢 Metrics 7 duplication
Metric Results Duplication 7
TIP This summary will be updated as you push new changes. Give us feedback
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
|
That is an impressive number of AI review bots! 😂 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app.py (2)
154-166: URL scheme validation relies on caller; consider defensive check.Static analysis (S310) flags
_fetch_json_opener.open(req, ...)sinceurllibcan openfile://URLs. Whileurl_is_allowed_for_fetch()validates schemes before this function is called,fetch_json_document()itself doesn't enforce the constraint. A defensive scheme check here would prevent misuse if called from other code paths in the future.Proposed defensive check
def fetch_json_document(url): + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + raise ValueError("only http/https URLs are supported") req = Request( url, headers={"User-Agent": JSON_FETCH_USER_AGENT}, method="GET", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.py` around lines 154 - 166, Add a defensive URL-scheme check at the start of fetch_json_document to ensure callers cannot pass disallowed schemes (e.g., file://); call the existing url_is_allowed_for_fetch(url) (or validate urllib.parse.urlparse(url).scheme against the allowed list) and raise ValueError("unsupported URL scheme") if it returns false before constructing the Request and calling _fetch_json_opener.open, so fetch_json_document enforces the same scheme restrictions as url_is_allowed_for_fetch.
95-151: SSRF protections are comprehensive but note DNS rebinding TOCTOU window.The URL validation is thorough: scheme restrictions, blocked hosts, IP range checks, port limits, userinfo rejection, and DNS resolution checks. However, there's a time-of-check-to-time-of-use (TOCTOU) window between
socket.getaddrinfo()here and the actual HTTP connection infetch_json_document(). An attacker with control over DNS could return a public IP during validation, then a private IP during the actual fetch (DNS rebinding).The risk is mitigated by:
- The
_NoHttpRedirectHandlerblocking redirects- The short time window between check and fetch
For additional hardening, consider binding the resolved IP directly when making the request, though this adds complexity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.py` around lines 95 - 151, The current SSRF check in url_is_allowed_for_fetch has a TOCTOU gap between socket.getaddrinfo() and the actual HTTP request in fetch_json_document(); fix by moving to a model where fetch_json_document resolves the host once and connects directly to the validated IP (rather than allowing the HTTP client to re-resolve), i.e., have fetch_json_document call the same _ip_for_ssrf_check/socket.getaddrinfo logic used in url_is_allowed_for_fetch to obtain a concrete IP, verify that IP remains allowed, open the TCP connection to that IP (using the numeric address) while setting the HTTP Host header to the original hostname to preserve virtual host semantics, and ensure redirects are still blocked (keep _NoHttpRedirectHandler). This prevents DNS rebinding between validation and use without changing url_is_allowed_for_fetch semantics.test_api.py (1)
165-177: Consider testing edge cases for cache parameter.The implementation has
if sec > 0to skip setting Cache-Control for zero values, and clamps to 86400 max. Consider adding tests for these edge cases.Suggested additional tests
`@patch`("app.fetch_json_document") def test_dynamic_json_cache_zero_no_header(mock_fetch, client): """cache=0 should not set Cache-Control header.""" mock_fetch.return_value = {"n": 5} response = client.get( "/dynamic/json/", query_string={ "url": "https://example.com/x.json", "query": "$.n", "cache": "0", }, ) assert response.status_code == 200 assert "Cache-Control" not in response.headers `@patch`("app.fetch_json_document") def test_dynamic_json_cache_clamped_to_max(mock_fetch, client): """cache values exceeding 86400 should be clamped.""" mock_fetch.return_value = {"n": 5} response = client.get( "/dynamic/json/", query_string={ "url": "https://example.com/x.json", "query": "$.n", "cache": "999999", }, ) assert response.status_code == 200 assert response.headers.get("Cache-Control") == "public, max-age=86400"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_api.py` around lines 165 - 177, Add two edge-case tests for the dynamic JSON cache behavior: create test_dynamic_json_cache_zero_no_header and test_dynamic_json_cache_clamped_to_max (both patching app.fetch_json_document like the existing test_dynamic_json_cache_sets_cache_control) that call client.get("/dynamic/json/") with query params "url", "query" and "cache" set to "0" and a very large value (e.g. "999999") respectively; assert the zero case returns status 200 and does NOT include "Cache-Control" in response.headers, and assert the large-value case returns status 200 and includes "Cache-Control" equal to "public, max-age=86400" to verify the if sec > 0 behavior and the 86400 clamp in the code handling cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 208: Replace the incorrect opening HTML tag "<tbody>" at the reported
location with the closing tag "</tbody>" so the table markup is properly closed;
locate the stray "<tbody>" in README.md (the same incorrect tag referenced in
the comment) and change it to "</tbody>" to fix the HTML typo.
- Line 162: Replace the incorrect opening HTML tag at the end of the table:
change the stray `<tbody>` token to the closing tag `</tbody>` so the table body
is properly terminated; locate the fragment containing `<tbody>` in README.md
and update it to `</tbody>` (ensure matching with surrounding `<table>` /
`<thead>` tags).
---
Nitpick comments:
In `@app.py`:
- Around line 154-166: Add a defensive URL-scheme check at the start of
fetch_json_document to ensure callers cannot pass disallowed schemes (e.g.,
file://); call the existing url_is_allowed_for_fetch(url) (or validate
urllib.parse.urlparse(url).scheme against the allowed list) and raise
ValueError("unsupported URL scheme") if it returns false before constructing the
Request and calling _fetch_json_opener.open, so fetch_json_document enforces the
same scheme restrictions as url_is_allowed_for_fetch.
- Around line 95-151: The current SSRF check in url_is_allowed_for_fetch has a
TOCTOU gap between socket.getaddrinfo() and the actual HTTP request in
fetch_json_document(); fix by moving to a model where fetch_json_document
resolves the host once and connects directly to the validated IP (rather than
allowing the HTTP client to re-resolve), i.e., have fetch_json_document call the
same _ip_for_ssrf_check/socket.getaddrinfo logic used in
url_is_allowed_for_fetch to obtain a concrete IP, verify that IP remains
allowed, open the TCP connection to that IP (using the numeric address) while
setting the HTTP Host header to the original hostname to preserve virtual host
semantics, and ensure redirects are still blocked (keep _NoHttpRedirectHandler).
This prevents DNS rebinding between validation and use without changing
url_is_allowed_for_fetch semantics.
In `@test_api.py`:
- Around line 165-177: Add two edge-case tests for the dynamic JSON cache
behavior: create test_dynamic_json_cache_zero_no_header and
test_dynamic_json_cache_clamped_to_max (both patching app.fetch_json_document
like the existing test_dynamic_json_cache_sets_cache_control) that call
client.get("/dynamic/json/") with query params "url", "query" and "cache" set to
"0" and a very large value (e.g. "999999") respectively; assert the zero case
returns status 200 and does NOT include "Cache-Control" in response.headers, and
assert the large-value case returns status 200 and includes "Cache-Control"
equal to "public, max-age=86400" to verify the if sec > 0 behavior and the 86400
clamp in the code handling cache.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb1e247-4a9b-45d8-b6fd-da16a87981c4
⛔ Files ignored due to path filters (1)
templates/dynamic_error.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
README.mdapp.pydocs/dynamic-json-sample.jsonrequirements.txttest_api.py
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
gstraccini
Bot
added
hacktoberfest
|
Hi @nimble-turtle, Thank you so much for your pull request! 🙌 I appreciate the time and effort you put into this contribution. Thanks again for your valuable contribution! 🚀 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Closes #204
📑 Description
Adds the ability to have progress bars that retrieve dynamic JSON.
It adds quite a few lines of code, so I understand if this feels like too much of a change. I also added quite a number of tests because pulling in remote files increases the attack surface both for your Vercel server and the end user.
Updated the
README.mdto reflect the new API usage info.It adds one new dependency for JSON parsing.
Let me know if you'd like any edits.
✅ Checks
☢️ Does this introduce a breaking change?
Summary by Sourcery
Add support for rendering progress bar SVGs from values loaded dynamically from remote JSON documents, with robust validation and security safeguards.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Tests