fix(scripts): correct END marker parsing in addCertificatesInTs.py #866
Conversation
WalkthroughThe extraction logic in two functions of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common/src/scripts/addCertificatesInTs.py (2)
10-20: DRY the PEM extraction and keep cleaning consistent even on fallbackBoth functions duplicate the same extraction pattern. Consider a small helper to:
- Find [BEGIN..END] chunk relative to BEGIN (as you do now).
- If either marker is missing, default pem_chunk = full_text (no early return).
- Always pass pem_chunk through the same cleaning pipeline for consistent output, matching the docstring promise to remove extra lines.
Example helper and usage:
def _extract_pem_chunk(full_text: str, begin_marker: str, end_marker: str) -> str: start_idx = full_text.find(begin_marker) if start_idx == -1: return full_text # fallback: no early return in callers; let them clean end_pos = full_text.find(end_marker, start_idx) if end_pos == -1: return full_text # fallback return full_text[start_idx:end_pos + len(end_marker)]Use it in both functions:
# In read_certificate_content begin_marker = "-----BEGIN CERTIFICATE-----" end_marker = "-----END CERTIFICATE-----" pem_chunk = _extract_pem_chunk(full_text, begin_marker, end_marker) # then run the existing cleaned_lines = [] logic on pem_chunk, always # In read_private_key_content begin_marker = "-----BEGIN PRIVATE KEY-----" end_marker = "-----END PRIVATE KEY-----" pem_chunk = _extract_pem_chunk(full_text, begin_marker, end_marker) # then run the existing cleaned_lines = [] logic on pem_chunk, alwaysAlso applies to: 37-47
37-47: Broaden private key marker support (RSA/EC/ENCRYPTED) for real-world robustnessMany PEMs use RSA PRIVATE KEY, EC PRIVATE KEY, or ENCRYPTED PRIVATE KEY. Consider trying a small set of known BEGIN/END pairs and picking the first present to avoid returning the raw text when a valid key exists.
Example:
key_types = [ ("-----BEGIN PRIVATE KEY-----", "-----END PRIVATE KEY-----"), ("-----BEGIN RSA PRIVATE KEY-----", "-----END RSA PRIVATE KEY-----"), ("-----BEGIN EC PRIVATE KEY-----", "-----END EC PRIVATE KEY-----"), ("-----BEGIN ENCRYPTED PRIVATE KEY-----", "-----END ENCRYPTED PRIVATE KEY-----"), ] for b, e in key_types: start_idx = full_text.find(b) if start_idx != -1: end_pos = full_text.find(e, start_idx) if end_pos != -1: pem_chunk = full_text[start_idx:end_pos + len(e)] break else: pem_chunk = full_text # fallback
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/scripts/addCertificatesInTs.py(3 hunks)
🔇 Additional comments (3)
common/src/scripts/addCertificatesInTs.py (3)
10-20: Robust END marker search and index validation — good fixSearching the END marker relative to the BEGIN marker and guarding for -1 eliminates the prior slice bug and prevents cross-block captures. This is the correct and safer approach.
37-47: Mirror fix for private keys — looks correctSame benefits here: relative search and index validation fix the incorrect-slice edge cases for key files too.
114-114: Guarded entrypoint is correctThe script runs only when invoked directly; safe for imports and CI.
|
hey, could you give more context on this PR? |
2 participants
Summary by CodeRabbit