flashers: support http headers#610
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds optional per-request HTTP headers and an optional Bearer token to the flasher driver and CLI; validates the token; propagates headers/bearer into operator selection, URL access checks, metadata HEAD fallbacks, curl download commands, and background transfer paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI
participant D as Driver.flash
participant OP as HTTP Operator / curl
participant M as Metadata
U->>CLI: jumpstarter flash --header "Key: Value" --bearer "TOKEN" <path>
CLI->>CLI: _parse_headers -> headers dict
CLI->>D: flash(path, headers=headers, bearer_token=TOKEN)
alt path is HTTP(S)
D->>D: _validate_bearer_token(TOKEN)
D->>D: _prepare_headers(headers, TOKEN) -> header_args
D->>OP: build HTTP operator (Bearer) / set scheme=http
D->>OP: _check_url_access(url, header_args)
OP-->>D: access result
else local path
D->>D: use provided/local operator
end
D->>M: _create_metadata_and_json(..., headers)
M-->>D: metadata
D->>OP: _flash_with_progress(..., header_args) => curl -H ... download
OP-->>D: progress/completion
D-->>CLI: result
CLI-->>U: exit/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
459-484: Header-aware metadata fallback isn’t wired; decide to pass or drop.
_create_metadata_and_json(..., headers: dict[str, str] | None = None)accepts headers and uses them forrequests.head(...), but callers don’t passheaders. Either:
- Pass headers selectively (only when HEADing the original remote URL, not the exporter), or
- Remove the parameter to avoid confusion.
Suggested selective pass (prevents leaking headers to exporter):
- metadata, metadata_json = self._create_metadata_and_json(src_operator, src_path, file_hash, original_url) + headers_for_head = None + if original_url and original_url.startswith(("http://", "https://")): + # Only forward headers if the HEAD target is not the exporter + try: + from urllib.parse import urlparse as _u + if _u(original_url).netloc != _u(self.http.get_url()).netloc: + headers_for_head = headers # requires threading arg; see below + except Exception: + pass + metadata, metadata_json = self._create_metadata_and_json( + src_operator, src_path, file_hash, original_url, headers=headers_for_head + )To make this compile, add
headersto_transfer_bg_thread(...)signature and pass it from theflash(...)call site:- args=(path, operator, operator_scheme, os_image_checksum, self.http.storage, error_queue, image_url), + args=(path, operator, operator_scheme, os_image_checksum, self.http.storage, error_queue, image_url, headers),and:
- def _transfer_bg_thread( + def _transfer_bg_thread( self, ... - original_url: str | None = None, + original_url: str | None = None, + headers: dict[str, str] | None = None,Also applies to: 436-439
🧹 Nitpick comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
658-684: Allow duplicate headers or document the limitation.Parsing into a dict collapses duplicates (e.g., multiple Cookie or Accept entries). If duplicates are needed, return a list[tuple[str, str]] or accept repeated keys in a MultiDict-like structure; otherwise, document that last-wins.
If you want duplicates:
- def _parse_headers(self, headers: list[str]) -> dict[str, str]: + def _parse_headers(self, headers: list[str]) -> list[tuple[str, str]]: ... - header_map = {} + parsed: list[tuple[str, str]] = [] ... - header_map[key] = value - return header_map + parsed.append((key, value)) + return parsedYou’d also need to accept both dict and list in
_curl_header_args.
704-709: CLI UX: option is clear; minor polish.
- Help text is good. Consider allowing
--header @filein the future if users have many headers.- Wiring from CLI to driver looks correct.
Also applies to: 721-722, 731-741
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
🔇 Additional comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
254-265: LGTM on header plumb-through to curl and access check.The flow and argument threading look correct once header leakage and quoting are fixed.
Also applies to: 283-291
83-84: Public API change — update docs & verify call-sitesAdded
headers: dict[str, str] | None = Nonetoflash(...). Update API docs / CLI help and verify all internal callers; I couldn't confirm call-sites from the sandbox run — run a repo-wide search for.flash((e.g.rg -n --hidden --no-ignore-vcs -S '\.flash\s*\(' --glob '!**/site-packages/**' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'). Location: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:83-84.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Outdated
Show resolved
Hide resolved
| header_args = self._curl_header_args(headers) | ||
| self._flash_with_progress( | ||
| console, | ||
| manifest, | ||
| path, | ||
| image_url, | ||
| target_device, | ||
| insecure_tls, | ||
| stored_cacert, | ||
| header_args, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Avoid leaking auth headers to the exporter’s HTTP server.
header_args is applied to curl even when we’re pulling from the exporter (should_download_to_httpd=True). This can leak Authorization or sensitive headers to the exporter and logs. Only pass headers to curl when downloading directly from the remote URL.
Apply:
- header_args = self._curl_header_args(headers)
+ header_args = self._curl_header_args(headers) if not should_download_to_httpd else ""And keep _check_url_access/flash pipeline unchanged; they will get empty headers when targeting the exporter.
Also applies to: 329-344
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 163 to 174, header_args is always computed and passed to curl even
when should_download_to_httpd=True, which can leak Authorization/sensitive
headers to the exporter; only compute and pass header_args when not using the
exporter (i.e., when should_download_to_httpd is False), otherwise set
header_args to an empty list and pass that; apply the same change to the other
occurrence around lines 329 to 344; leave the _check_url_access/flash pipeline
unchanged so they receive empty headers when targeting the exporter.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
98-108: Fix type issue: Convert parsed URL path to Path objectWhen using Bearer token authentication with HTTP(S) sources,
pathis set tourlparse(path).pathwhich returns a string. This causes issues later when code expects a Path object (e.g., accessingpath.nameat line 111).Apply this fix to ensure
pathremains a Path object:if path.startswith(("http://", "https://")) and bearer_token: parsed = urlparse(path) self.logger.info(f"Using Bearer token authentication for {parsed.netloc}") operator = Operator( "http", root="/", endpoint=f"{parsed.scheme}://{parsed.netloc}", token=bearer_token ) operator_scheme = "http" - path = Path(urlparse(path).path) + path = Path(parsed.path)
168-180: Security: Don't leak authentication headers to exporter's HTTP serverThe code always includes header arguments when calling curl, even when
should_download_to_httpd=True. This could leak sensitive authentication headers to the exporter's HTTP server and into logs.Apply this fix to only pass headers when downloading directly from the remote URL:
all_headers = headers.copy() if headers else {} if bearer_token: all_headers["Authorization"] = f"Bearer {bearer_token}" - header_args = self._curl_header_args(all_headers) + header_args = self._curl_header_args(all_headers) if not should_download_to_httpd else ""
249-260: Security: Harden shell escaping in curl header argumentsHeader values containing single quotes will break the shell command and could potentially lead to command injection. The current implementation doesn't properly escape single quotes.
Apply proper shell escaping:
def _curl_header_args(self, headers: dict[str, str] | None) -> str: """Generate header arguments for curl command""" if not headers: return "" parts: list[str] = [] + def _escape_single_quotes(s: str) -> str: + """Escape single quotes for shell: ' -> '\''""" + return s.replace("'", "'\"'\"'") for k, v in headers.items(): k = str(k).strip() - v = str(v) + v = str(v).strip() if not k: continue - parts.append(f"-H '{k}: {v}'") + parts.append(f"-H '{_escape_single_quotes(k)}: {_escape_single_quotes(v)}'") return " ".join(parts)
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
168-171: Avoid redundant header copying when no modifications neededThe code always creates a copy of headers even when no Bearer token is provided, which is unnecessary.
Optimize header handling:
- all_headers = headers.copy() if headers else {} - if bearer_token: - all_headers["Authorization"] = f"Bearer {bearer_token}" + if bearer_token: + all_headers = headers.copy() if headers else {} + all_headers["Authorization"] = f"Bearer {bearer_token}" + else: + all_headers = headers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
790-801: LGTM: Decompression command logic handles the new path types correctlyThe
_get_decompression_commandfunction properly handles both PosixPath objects and HTTP(S) URLs, extracting the filename correctly for compression detection.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
| if headers: | ||
| response = requests.head(original_url, headers=headers) | ||
| else: | ||
| response = requests.head(original_url) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider timeout and error handling for HEAD requests
The HEAD requests for metadata retrieval don't specify timeouts, which could cause the process to hang on unresponsive servers.
Add timeout and improve error handling:
if headers:
- response = requests.head(original_url, headers=headers)
+ response = requests.head(original_url, headers=headers, timeout=30)
else:
- response = requests.head(original_url)
+ response = requests.head(original_url, timeout=30)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if headers: | |
| response = requests.head(original_url, headers=headers) | |
| else: | |
| response = requests.head(original_url) | |
| if headers: | |
| response = requests.head(original_url, headers=headers, timeout=30) | |
| else: | |
| response = requests.head(original_url, timeout=30) |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 488-491, the requests.head call has no timeout and lacks exception
handling; update both HEAD branches to pass a sensible timeout (e.g., timeout=5
or configurable constant), wrap the call in a try/except catching
requests.exceptions.RequestException, handle timeouts and connection errors by
logging a warning/error and returning a safe fallback (e.g., None or an error
response), and avoid raising unhandled exceptions; if HEAD fails consider a
controlled fallback to a GET with the same timeout.
There was a problem hiding this comment.
This is unrelated to this patch.
There was a problem hiding this comment.
@mangelajo You're absolutely right! The timeout and error handling suggestion, while a good practice, is indeed outside the scope of this PR which is specifically focused on adding HTTP headers support. I apologize for the tangential suggestion.
The core functionality you're implementing—passing headers to the requests.head() calls—works correctly as written. Thanks for keeping the PR focused on its intended purpose.
🐰💭
✏️ Learnings added
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.549Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Outdated
Show resolved
Hide resolved
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
We should add some early checking to:
-
Headers (that conform to standards and don't include extra spaces which would break the cmdline later.
-
Bearer token, in the same way.
So that curl or download does not fail later down the road.
| return "" | ||
| parts: list[str] = [] | ||
| for k, v in headers.items(): | ||
| k = str(k).strip() |
There was a problem hiding this comment.
should we prevent/fail if k/v have spaces or something incompatible?, just to avoid the command call crashing and then giving a hard to understand message to the user.
| if headers: | ||
| response = requests.head(original_url, headers=headers) | ||
| else: | ||
| response = requests.head(original_url) |
There was a problem hiding this comment.
This is unrelated to this patch.
| key = key.strip() | ||
| value = value.strip() | ||
|
|
||
| if not key: |
There was a problem hiding this comment.
may be the additional checks should happen here?
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Outdated
Show resolved
Hide resolved
bennyz
force-pushed
the
flashers-headers
branch
2 times, most recently
from
c6facad to
9b951c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
168-178: Do not leak auth headers to the exporter’s HTTP server.Only pass headers to curl when downloading directly from the remote URL. When using the exporter (
should_download_to_httpd=True), setheader_args="". Also avoid sending headers in the access check. This was raised previously and still applies.- header_args = self._prepare_headers(headers, bearer_token) + header_args = self._prepare_headers(headers, bearer_token) if not should_download_to_httpd else "" @@ - self._check_url_access(console, prompt, image_url, tls_args, header_args) + self._check_url_access(console, prompt, image_url, tls_args, header_args) @@ - f'( curl -fsSL {tls_args} {header_args} "{image_url}" | ' + f'( curl -fsSL {tls_args} {header_args} "{image_url}" | 'Also applies to: 300-305, 346-361
🧹 Nitpick comments (5)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (5)
101-110: Nit: reuse parsed URL; avoid double urlparse.Minor clean-up and consistency with earlier lines.
- operator_scheme = "http" - path = Path(urlparse(path).path) + operator_scheme = "http" + path = Path(parsed.path)
246-251: Authorization precedence: don’t silently override user-provided Authorization.If the user supplied an Authorization header, prefer it and ignore the bearer token (warn). Prevents multiple/conflicting Authorization headers.
def _prepare_headers(self, headers: dict[str, str] | None, bearer_token: str | None) -> str: - all_headers = headers.copy() if headers else {} - if bearer_token: - all_headers["Authorization"] = f"Bearer {bearer_token}" + all_headers = headers.copy() if headers else {} + if bearer_token: + if any(k.lower() == "authorization" for k in all_headers.keys()): + self.logger.warning("Authorization header provided; ignoring bearer token") + else: + all_headers["Authorization"] = f"Bearer {bearer_token}" return self._curl_header_args(all_headers)
675-698: Harden header parsing: RFC‑compliant names, duplicates, and control chars.Validate header names as RFC7230 tokens, reject CR/LF in names/values, and detect duplicates case‑insensitively for clearer UX.
def _parse_headers(self, headers: list[str]) -> dict[str, str]: @@ - header_map = {} + token_re = re.compile(r"^[!#$%&'*+\-.^_`|~0-9A-Za-z]+$") + header_map: dict[str, str] = {} + seen: set[str] = set() for h in headers: if ":" not in h: raise click.ClickException(f"Invalid header format: {h!r}. Expected 'Key: Value'.") key, value = h.split(":", 1) key = key.strip() value = value.strip() if not key: raise click.ClickException(f"Invalid header key in: {h!r}") - - header_map[key] = value + if not token_re.match(key): + raise click.ClickException(f"Invalid header name '{key}': must be an HTTP token (RFC7230)") + if any(c in ("\r", "\n") for c in key) or any(c in ("\r", "\n") for c in value): + raise click.ClickException("Header names/values must not contain CR/LF") + kl = key.lower() + if kl in seen: + raise click.ClickException(f"Duplicate header '{key}'") + seen.add(kl) + header_map[key] = value return header_map
252-270: Defensive check for programmatic callers of _curl_header_args.If
headersbypass the CLI parser, add a minimal check to reject CR/LF in name/value to avoid shell breakage.def _curl_header_args(self, headers: dict[str, str] | None) -> str: @@ for k, v in headers.items(): k = str(k).strip() v = str(v).strip() if not k: continue + if "\r" in k or "\n" in k or "\r" in v or "\n" in v: + raise click.ClickException("Header names/values must not contain CR/LF") parts.append(f"-H '{_sq(k)}: {_sq(v)}'")
718-728: Optional: validate bearer at CLI edge for faster feedback.Validate
--bearerbefore invokingself.flash()so users get immediate errors.def flash( @@ - headers = self._parse_headers(header) if header else None + headers = self._parse_headers(header) if header else None + if bearer: + bearer = self._validate_bearer_token(bearer)Also applies to: 751-752
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T08:19:27.391Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.391Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-09-15T08:18:48.549Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.549Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (5)
operator_for_path(46-58)flash(516-525)flash(555-569)flash(624-651)flash(734-755)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
179-189: Do not send Authorization/headers to the exporter’s HTTP server.Headers (including Authorization) are always passed to curl, which leaks them to the exporter when should_download_to_httpd is True. Only include headers when fetching directly from the remote URL. This was flagged earlier; reiterating here.
- header_args = self._prepare_headers(headers, bearer_token) + header_args = self._prepare_headers(headers, bearer_token) if not should_download_to_httpd else "" self._flash_with_progress(No changes needed in _flash_with_progress/_check_url_access; passing an empty header_args string will naturally omit headers from those curls when using the exporter.
Also applies to: 314-321, 373-375
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
86-88: Validate bearer token when provided (even if empty string).Currently validation is skipped for empty strings because the condition checks truthiness. Validate whenever the option is present.
- if bearer_token: - bearer_token = self._validate_bearer_token(bearer_token) + if bearer_token is not None: + bearer_token = self._validate_bearer_token(bearer_token)
100-113: Preserve original HTTP URL for metadata HEAD fallback when only headers (no bearer) are supplied.If path is HTTP(S) but no bearer_token is given, original_http_url remains None and the requests HEAD fallback can’t use headers. Capture the original URL in that branch too.
if operator is None: - if path.startswith(("http://", "https://")) and bearer_token: + if path.startswith(("http://", "https://")) and bearer_token: parsed = urlparse(path) self.logger.info(f"Using Bearer token authentication for {parsed.netloc}") original_http_url = path operator = Operator( "http", root="/", endpoint=f"{parsed.scheme}://{parsed.netloc}", token=bearer_token ) operator_scheme = "http" path = Path(parsed.path) else: - path, operator, operator_scheme = operator_for_path(path) + # If a remote HTTP URL is used without bearer, keep the original for HEAD fallback with headers + if isinstance(path, str) and path.startswith(("http://", "https://")): + original_http_url = path + path, operator, operator_scheme = operator_for_path(path)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)
13-15: Make the mock logger tolerant to future call signatures.Use callables that accept *args, **kwargs so added parameters (e.g., exc_info) won’t break tests.
- self.logger = type( - "MockLogger", (), {"warning": lambda msg: None, "info": lambda msg: None, "error": lambda msg: None} - )() + self.logger = type( + "MockLogger", + (), + { + "warning": lambda *args, **kwargs: None, + "info": lambda *args, **kwargs: None, + "error": lambda *args, **kwargs: None, + }, + )()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(17 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T08:19:27.391Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.391Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-09-15T08:18:48.549Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.549Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
_validate_bearer_token(729-745)_curl_header_args(267-284)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (4)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
83-85: API surface extension looks good.Adding headers and bearer_token to flash(...) is consistent with the PR’s objective.
694-728: Header parsing validation is solid.Good RFC7230 token enforcement, duplicate detection, and CR/LF rejection. Matches the project’s intended robustness.
729-745: Bearer token validator looks correct and balanced.Reasonable character and length checks; messages match tests.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)
18-40: Targeted tests for bearer validation and header quoting look good.Covers empty/invalid tokens and the POSIX-safe single-quote escape pattern.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
179-189: Do not send headers to the exporter’s HTTP server.When
should_download_to_httpdis True,image_urlpoints at the exporter. Passing user headers (esp. Authorization) leaks them. Gate headers for exporter downloads.- header_args = self._prepare_headers(headers, bearer_token) + header_args = "" if should_download_to_httpd else self._prepare_headers(headers, bearer_token) self._flash_with_progress(
122-135: Metadata HEAD fallback misses Authorization; merge bearer into headers passed to thread.If the remote requires auth, the fallback HEAD runs without the Bearer header. Merge it before spawning the thread.
- storage_thread = threading.Thread( + # Build headers for controller-side HTTP HEAD (only matters when original_http_url is set) + _head_headers = None + if original_http_url: + _head_headers = dict(headers or {}) + if bearer_token and not any(k.lower() == "authorization" for k in _head_headers): + _head_headers["Authorization"] = f"Bearer {bearer_token}" + + storage_thread = threading.Thread( target=self._transfer_bg_thread, args=( path, operator, operator_scheme, os_image_checksum, self.http.storage, error_queue, - original_http_url, - headers, + original_http_url, + _head_headers, ), name="storage_transfer", )
🧹 Nitpick comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
83-89: Additions look good; validate bearer even when empty string is passed.Empty strings skip validation due to truthiness check. Validate when not None to give a clear error for
--bearer "".- if bearer_token: - bearer_token = self._validate_bearer_token(bearer_token) + if bearer_token is not None: + bearer_token = self._validate_bearer_token(bearer_token)
102-112: HTTP operator + Path fix LGTM; consider behavior when operator is provided alongside bearer.The Path conversion resolves the earlier
.name/decompression issues. If a caller supplies bothoperatorandbearer_token, the token is ignored; a warning could aid UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(17 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T08:19:27.391Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.391Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-09-15T08:18:48.549Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.549Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (5)
operator_for_path(46-58)flash(516-525)flash(555-569)flash(624-651)flash(734-755)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (10)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (10)
257-275: Shell-safe header quoting LGTM.Single-quote escaping is correct; validation is centralized in
_validate_header_dict.
276-286: Signature extension is consistent.
header_argsthreading into flashing path looks correct.
304-313: Ensure header gating applies to both access check and download.With the proposed gating,
{header_args}will be empty for exporter URLs, preventing leaks in both_check_url_accessand the curl pipeline.Also applies to: 351-365
424-435: Thread plumbing for headers/original URL is correct; pairs with HEAD auth fix above.Passing
original_urlandheadersinto_create_metadata_and_jsonis the right place; apply the merge diff suggested earlier.Also applies to: 460-462
484-486: HEAD with optional headers LGTM (scope‑aligned).Signature and conditional header use look good. Avoid adding timeout/error‑handling here per PR scope.
Also applies to: 506-509
684-701: Header validation helper LGTM.RFC7230 token check, CR/LF rejection, and case‑insensitive duplicate detection are solid.
703-713: CLI header parsing defers to shared validation — good consolidation.
714-724: Header preparation flow LGTM.Merging Authorization only when absent is correct.
725-741: Bearer validation helper LGTM; pairs with earlier call‑site fix.
762-772: CLI options and wiring look correct.Options are clear; headers parsed and both headers/bearer forwarded to
flash().Also applies to: 784-786, 795-795, 804-805
| self._manifest = FlasherBundleManifestV1Alpha1.from_string(yaml_str) | ||
| return self._manifest | ||
|
|
||
| def _validate_header_dict(self, header_map: dict[str, str]) -> dict[str, str]: |
There was a problem hiding this comment.
nice!
This is probably nitpicking, but a suggestion,
header validation should probably happen inside the flash command, at the start (early failure) if headers provided and return jumpstarter exceptions.
Then in the click commands we can convert those exceptions to click.
This would cover the programatic use of flash commands with python.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
flashers-headers
branch
from
66b0536 to
f381274
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
105-116: Optional: warn when bearer is provided alongside an explicit operator.To avoid surprises, emit a warning if
operatoris already set andbearer_tokenis provided (since the auto‑HTTP operator path won’t run).Example:
- if operator is None: + if operator is None: if path.startswith(("http://", "https://")) and bearer_token: ... else: path, operator, operator_scheme = operator_for_path(path) + elif bearer_token: + self.logger.warning("Bearer token provided but an Operator was supplied; ignoring bearer_token")
182-192: Do not leak Authorization/headers to the exporter HTTP server.When
should_download_to_httpdis True,image_urlpoints to the exporter. Passingheader_argswill leak auth headers (incl. Bearer) to the exporter and logs. Gate headers to only be used for direct remote downloads.Apply:
- header_args = self._prepare_headers(headers, bearer_token) + # Only send headers when downloading from the remote URL directly (not via exporter) + header_args = "" if should_download_to_httpd else self._prepare_headers(headers, bearer_token)Also applies to: 312-313, 367-368
126-137: HEAD fallback misses Authorization when only bearer_token is supplied.If the resource requires auth and the user passed only
bearer_token(no explicit Authorization header), the requests HEAD tooriginal_urlis unauthenticated. Build a header map for the HEAD fallback that includes Authorization when needed and pass it into the transfer thread.Apply:
# Start the storage write operation in the background + # Build headers for remote HEAD metadata (used only when original_http_url is set) + _head_headers = (headers or {}).copy() + if bearer_token and not any(k.lower() == "authorization" for k in _head_headers): + _head_headers["Authorization"] = f"Bearer {bearer_token}" storage_thread = threading.Thread( target=self._transfer_bg_thread, args=( path, operator, operator_scheme, os_image_checksum, self.http.storage, error_queue, - original_http_url, - headers, + original_http_url, + _head_headers, ), name="storage_transfer", )Also applies to: 487-513
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
720-733: Make the Authorization presence check case‑insensitive to avoid duplicates.If a caller provides
authorization(lowercase), the current check won’t see it and will add anotherAuthorization, leading to duplicate headers.Apply:
- if bearer_token and "Authorization" not in (headers or {}): + if bearer_token and not any(k.lower() == "authorization" for k in (headers or {})): auth_header = {"Authorization": all_headers["Authorization"]} self._validate_header_dict(auth_header)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(18 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.412Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
📚 Learning: 2025-09-15T08:19:27.412Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.412Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-09-15T08:18:48.571Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
260-278: LGTM: robust single‑quote escaping for curl header args.The POSIX‑safe escaping is correct; combined with upstream validation, this is safe to interpolate into the shell.
734-751: LGTM: bearer token validation is clear and defensive.Good checks for emptiness, printable ASCII, exclusion of whitespace/quotes, and a sane upper bound.
86-91: LGTM: early validation for bearer and headers.Validating inputs at the start of
flash()prevents late failures and aligns with the CLI parsing path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
182-192: Do not send auth headers to the exporter’s HTTP server.When
should_download_to_httpdis True, the curl runs against the exporter; passing user headers (especially Authorization) leaks secrets. Gateheader_argsaccordingly.- header_args = self._prepare_headers(headers, bearer_token) + header_args = ( + self._prepare_headers(headers, bearer_token) + if not should_download_to_httpd + else "" + ) self._flash_with_progress(
126-137: Metadata HEAD fallback misses Authorization when only bearer is provided.You pass
headersto the background thread unchanged; if the user supplied only--bearer, the HEAD fallback won’t carryAuthorization. Build a per-HEAD headers dict that includes the bearer-token Authorization when not already present.- storage_thread = threading.Thread( + # Build headers for controller-side HTTP HEAD (if we later use requests fallback) + _head_headers = dict(headers) if headers else {} + if bearer_token and not any(k.lower() == "authorization" for k in _head_headers): + _head_headers["Authorization"] = f"Bearer {bearer_token}" + + storage_thread = threading.Thread( target=self._transfer_bg_thread, args=( path, operator, operator_scheme, os_image_checksum, self.http.storage, error_queue, - original_http_url, - headers, + original_http_url, + _head_headers, ),
🧹 Nitpick comments (4)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (4)
83-91: Early validation in the right place; minor consistency nit.Good: validating bearer and headers up front. Minor: since
_validate_header_dictraisesArgumentErrorbut_validate_bearer_tokenraisesclick.ClickException, exception types are inconsistent at the driver layer. See proposal below.
103-116: Preserve original HTTP URL for metadata HEAD in all HTTP cases.
original_http_urlis set only on the bearer-token branch. Ifpathis HTTP(S) without bearer, fallback HEAD won’t have the original URL. Set it before callingoperator_for_pathwhenpathis HTTP(S).- else: - path, operator, operator_scheme = operator_for_path(path) + else: + if isinstance(path, str) and path.startswith(("http://", "https://")): + original_http_url = path + path, operator, operator_scheme = operator_for_path(path)
720-733: Make Authorization presence check case‑insensitive; avoid partial re‑validation.Use a case‑insensitive check for the Authorization header, and avoid the extra one-off
_validate_header_dictcall since you already validated inputs earlier.- if bearer_token: - if any(k.lower() == "authorization" for k in all_headers.keys()): - self.logger.warning("Authorization header provided - ignoring bearer token") - else: - all_headers["Authorization"] = f"Bearer {bearer_token}" - - if bearer_token and "Authorization" not in (headers or {}): - auth_header = {"Authorization": all_headers["Authorization"]} - self._validate_header_dict(auth_header) + if bearer_token: + if any(k.lower() == "authorization" for k in all_headers): + self.logger.warning("Authorization header provided - ignoring bearer token") + else: + all_headers["Authorization"] = f"Bearer {bearer_token}"
802-815: Translate bearer token validation errors to Click at the CLI boundary.If
_validate_bearer_tokenraisesArgumentError, convert it toclick.ClickExceptionin the CLIflashcommand for a clean UX.- headers = self._parse_headers(header) if header else None + headers = self._parse_headers(header) if header else None + try: + bearer = self._validate_bearer_token(bearer) if bearer else None + except ArgumentError as e: + raise click.ClickException(str(e)) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(18 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.412Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
📚 Learning: 2025-09-15T08:19:27.412Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.412Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-09-15T08:18:48.571Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (5)
260-278: Header quoting looks solid.Single-quote escaping is POSIX-safe; names/values are stripped. With upstream validation, this is good.
307-315: Header leak fixed by earlier gating; no further changes needed here.Once
header_argsis blanked for exporter downloads, these paths are fine.Also applies to: 354-376
487-517: HEAD fallback now accepts headers; add small timeout only if you decide to widen scope later.Functionally correct for this PR. If you later touch this code, consider a short timeout; leaving it out is fine per scope.
[raise_nothing]
687-705: Header validation is RFC‑token compliant and dedupes case‑insensitively.LGTM.
72-85: API additions look good.
headersandbearer_tokeninflash()are coherently integrated.Please confirm no external callers rely on positional args beyond
insecure_tls; adding keyword‑only params can still break callers using**kwargsif they pass unexpected keys.
| def _validate_bearer_token(self, token: str | None) -> str | None: | ||
| if token is None: | ||
| return None | ||
|
|
||
| token = token.strip() | ||
| if not token: | ||
| raise click.ClickException("Bearer token cannot be empty") | ||
|
|
||
| # RFC 6750 allows token68 format (base64url-encoded) or other token formats | ||
| # Basic validation: printable ASCII excluding whitespace and special chars that could cause issues | ||
| if not all(32 < ord(c) < 127 and c not in ' "\\' for c in token): | ||
| raise click.ClickException("Bearer token contains invalid characters") | ||
|
|
||
| if len(token) > 4096: | ||
| raise click.ClickException("Bearer token is too long (max 4096 characters)") | ||
|
|
||
| return token | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use project exception (ArgumentError) instead of Click in driver layer.
Per prior guidance, driver code should raise project exceptions; the CLI should translate to Click. Switch _validate_bearer_token to ArgumentError.
- if not token:
- raise click.ClickException("Bearer token cannot be empty")
+ if not token:
+ raise ArgumentError("Bearer token cannot be empty")
@@
- if not all(32 < ord(c) < 127 and c not in ' "\\' for c in token):
- raise click.ClickException("Bearer token contains invalid characters")
+ if not all(32 < ord(c) < 127 and c not in ' "\\' for c in token):
+ raise ArgumentError("Bearer token contains invalid characters")
@@
- if len(token) > 4096:
- raise click.ClickException("Bearer token is too long (max 4096 characters)")
+ if len(token) > 4096:
+ raise ArgumentError("Bearer token is too long (max 4096 characters)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _validate_bearer_token(self, token: str | None) -> str | None: | |
| if token is None: | |
| return None | |
| token = token.strip() | |
| if not token: | |
| raise click.ClickException("Bearer token cannot be empty") | |
| # RFC 6750 allows token68 format (base64url-encoded) or other token formats | |
| # Basic validation: printable ASCII excluding whitespace and special chars that could cause issues | |
| if not all(32 < ord(c) < 127 and c not in ' "\\' for c in token): | |
| raise click.ClickException("Bearer token contains invalid characters") | |
| if len(token) > 4096: | |
| raise click.ClickException("Bearer token is too long (max 4096 characters)") | |
| return token | |
| def _validate_bearer_token(self, token: str | None) -> str | None: | |
| if token is None: | |
| return None | |
| token = token.strip() | |
| if not token: | |
| raise ArgumentError("Bearer token cannot be empty") | |
| # RFC 6750 allows token68 format (base64url-encoded) or other token formats | |
| # Basic validation: printable ASCII excluding whitespace and special chars that could cause issues | |
| if not all(32 < ord(c) < 127 and c not in ' "\\' for c in token): | |
| raise ArgumentError("Bearer token contains invalid characters") | |
| if len(token) > 4096: | |
| raise ArgumentError("Bearer token is too long (max 4096 characters)") | |
| return token |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 734 to 751, replace uses of click.ClickException with the project
ArgumentError exception: import ArgumentError from the project's exceptions
module (or the correct package path) and raise ArgumentError("message") for the
empty, invalid-character, and too-long token cases; keep all current validation
logic and messages unchanged and ensure the module imports ArgumentError at top
so the driver layer raises project-specific exceptions while the CLI layer
remains responsible for translating them to Click errors.
|
Successfully created backport PR for |
2 participants
Summary by CodeRabbit
New Features
CLI
Tests