Merged
Conversation
📝 WalkthroughWalkthroughAdds CLI support for supplying an SSL server certificate and propagates it into ADT, OData, and REST connections; introduces config-source fetching and a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Entry
participant ConfigCmd as sap.cli.config
participant ConfigLib as sap.config
participant Source as Local/Remote Source
participant ConfigFile as Local ConfigFile
CLI->>ConfigCmd: sapcli config merge --source <src> [--insecure] [--overwrite]
ConfigCmd->>ConfigLib: fetch_config_source(source, insecure, ssl_verify)
alt source is URL
ConfigLib->>Source: HTTP/HTTPS GET (with ssl_verify/insecure logic)
Source-->>ConfigLib: YAML payload / error
else source is local
ConfigLib->>Source: read file
Source-->>ConfigLib: YAML payload / error
end
ConfigLib->>ConfigLib: _validate_config_data(parsed_yaml)
ConfigLib-->>ConfigCmd: return source_dict
ConfigCmd->>ConfigLib: merge_into(target_config, source_dict, overwrite)
ConfigLib->>ConfigFile: read/create target config
ConfigLib->>ConfigFile: merge connections/users/contexts (add/skip per overwrite)
ConfigLib-->>ConfigCmd: merge summary {added, skipped}
ConfigCmd->>CLI: print merge results / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/configuration.md`:
- Around line 195-213: The docs show connections.<name>.http_timeout but the
runtime ignores it and only uses config_get()/SAPCLI_HTTP_TIMEOUT; update the
connection initialization to read the per-connection "http_timeout" from the
parsed config for the given connection name and pass that value into the HTTP
client/timeout setup instead of (or as an override to) config_get(); locate the
code that constructs connections or HTTP clients (search for config_get(),
SAPCLI_HTTP_TIMEOUT, and the connection factory/initializer function) and wire
connections.<name>.http_timeout into the HTTP timeout parameter, falling back to
config_get()/SAPCLI_HTTP_TIMEOUT when the per-connection value is absent.
In `@sap/cli/__init__.py`:
- Around line 247-251: The port parsing branch currently only catches ValueError
but int(config_values['port']) can raise TypeError for non-string YAML values;
update the except to catch both ValueError and TypeError and re-raise
SAPCliConfigError (from the original exception) with a message that includes the
offending config_values['port'] value so malformed configs consistently surface
SAPCliConfigError; target the block using config_values, args.port and
SAPCliConfigError.
- Around line 303-320: The _apply_config_extra_params function only applies
config values when args already has the attribute, so callers using
build_empty_connection_values()->resolve_default_connection_values() never get
RFC defaults and env-vs-config precedence isn't enforced; change the check to
use getattr(args, param, None) instead of hasattr(args, param) so missing
attributes are created and only set when the current value is falsy (e.g.
None/empty) and the key exists in config_values—i.e., replace "if hasattr(args,
param) and not getattr(args, param) and param in config_values:" with "if not
getattr(args, param, None) and param in config_values:" in
_apply_config_extra_params (this preserves env-provided values and populates
missing fields during
resolve_default_connection_values/build_empty_connection_values flows).
In `@sap/cli/config.py`:
- Around line 42-56: The current_context function incorrectly prints any
positional args.name without validating it or ensuring a loaded config; change
it to first load/validate the config from _get_config_file(args) and if no
config exists return a non‑zero error via console.printerr, and when args.name
is provided verify that name exists in the config (e.g. in config_file.contexts
or the appropriate contexts mapping) before printing — if the provided name is
not found print an error and return non‑zero; otherwise fall back to
config_file.current_context as before.
In `@sap/config.py`:
- Around line 22-26: CONNECTION_FIELDS currently lists 'ssl_server_cert' but
that key is never propagated; either remove 'ssl_server_cert' from
CONNECTION_FIELDS or wire it end-to-end: update
sap.cli.resolve_default_connection_values() and
sap.cli._apply_config_extra_params() to copy the 'ssl_server_cert' value onto
args (same pattern used for other SSL fields) and add a matching CLI
attribute/option in sap/cli/_entry.py so the CLI and config handling accept and
forward ssl_server_cert consistently; reference symbols: CONNECTION_FIELDS,
resolve_default_connection_values, _apply_config_extra_params, and the CLI entry
in sap/cli/_entry.py.
- Around line 398-416: fetch_config_source/_fetch_config_from_url currently
trusts requests' redirects, allowing an initial https:// source to be redirected
to http://; update the fetching logic so redirects cannot downgrade security —
either call requests.get(..., allow_redirects=False) or, after fetching,
validate that response.url still uses the same secure scheme as the original
source (i.e., if source started with "https://" then response.url must also
start with "https://"); apply the same check for the http branch where insecure
is False to ensure no unexpected scheme changes (refer to fetch_config_source
and _fetch_config_from_url to locate and fix the code paths).
- Around line 145-150: ConfigFile.load() currently returns ConfigFile(data,
path) without calling _validate_config_data(), and _validate_config_data()
itself only validates container types at the top level which allows malformed
entries like connections: [] or connections: {srv: 42} to slip through and later
cause TypeError or corrupt merges in merge_into(); fix by calling
_validate_config_data(data, path) from ConfigFile.load() before returning and
extend _validate_config_data() to deeply validate expected section shapes (e.g.,
ensure "connections" is a mapping of connection-name -> dict with expected
keys/types, not a list or scalar) so invalid structures are rejected early with
a clear ValidationError; update any other load/merge entry points (referenced in
the review around lines 346-359) to also invoke _validate_config_data() before
using the config and ensure merge_into() assumes validated input.
🪄 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: 468cca12-3772-4294-b1cc-fd2bdbe2e41c
📒 Files selected for processing (11)
AGENTS.mdREADME.mddoc/configuration.mdsap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.py
Comment on lines
+195
to
+213
| | Field | Type | Required | Default | Env var equivalent | | ||
| |---|---|---|---|---| | ||
| | `ashost` | string | yes (*) | - | `SAP_ASHOST` | | ||
| | `sysnr` | string | no | `"00"` | `SAP_SYSNR` | | ||
| | `client` | string | yes | - | `SAP_CLIENT` | | ||
| | `port` | int | no | `443` | `SAP_PORT` | | ||
| | `ssl` | bool | no | `true` | `SAP_SSL` | | ||
| | `ssl_verify` | bool | no | `true` | `SAP_SSL_VERIFY` | | ||
| | `ssl_server_cert` | string | no | - | `SAP_SSL_SERVER_CERT` | | ||
| | `mshost` | string | no (*) | - | `SAP_MSHOST` | | ||
| | `msserv` | string | no | - | `SAP_MSSERV` | | ||
| | `sysid` | string | no | - | `SAP_SYSID` | | ||
| | `group` | string | no | - | `SAP_GROUP` | | ||
| | `snc_qop` | string | no | - | `SNC_QOP` | | ||
| | `snc_myname` | string | no | - | `SNC_MYNAME` | | ||
| | `snc_partnername` | string | no | - | `SNC_PARTNERNAME` | | ||
| | `snc_lib` | string | no | - | `SNC_LIB` | | ||
| | `http_timeout` | float | no | `900` | `SAPCLI_HTTP_TIMEOUT` | | ||
|
|
There was a problem hiding this comment.
http_timeout is documented here, but YAML never applies it.
The table advertises connections.<name>.http_timeout, but the implementation still gets HTTP timeout from config_get() / SAPCLI_HTTP_TIMEOUT. A value placed in config.yml will be ignored unless this is wired into the runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/configuration.md` around lines 195 - 213, The docs show
connections.<name>.http_timeout but the runtime ignores it and only uses
config_get()/SAPCLI_HTTP_TIMEOUT; update the connection initialization to read
the per-connection "http_timeout" from the parsed config for the given
connection name and pass that value into the HTTP client/timeout setup instead
of (or as an override to) config_get(); locate the code that constructs
connections or HTTP clients (search for config_get(), SAPCLI_HTTP_TIMEOUT, and
the connection factory/initializer function) and wire
connections.<name>.http_timeout into the HTTP timeout parameter, falling back to
config_get()/SAPCLI_HTTP_TIMEOUT when the per-connection value is absent.
Comment on lines
+22
to
+26
| CONNECTION_FIELDS = ( | ||
| 'ashost', 'sysnr', 'client', 'port', 'ssl', 'ssl_verify', | ||
| 'ssl_server_cert', 'mshost', 'msserv', 'sysid', 'group', | ||
| 'snc_qop', 'snc_myname', 'snc_partnername', 'snc_lib', | ||
| ) |
There was a problem hiding this comment.
Wire ssl_server_cert through end-to-end or drop it from the supported fields.
Adding ssl_server_cert here makes it part of the effective context, but sap.cli.resolve_default_connection_values() / _apply_config_extra_params() never copy that key onto args, and sap/cli/_entry.py does not define a matching CLI attribute. A value under this field is silently ignored today.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/config.py` around lines 22 - 26, CONNECTION_FIELDS currently lists
'ssl_server_cert' but that key is never propagated; either remove
'ssl_server_cert' from CONNECTION_FIELDS or wire it end-to-end: update
sap.cli.resolve_default_connection_values() and
sap.cli._apply_config_extra_params() to copy the 'ssl_server_cert' value onto
args (same pattern used for other SSL fields) and add a matching CLI
attribute/option in sap/cli/_entry.py so the CLI and config handling accept and
forward ssl_server_cert consistently; reference symbols: CONNECTION_FIELDS,
resolve_default_connection_values, _apply_config_extra_params, and the CLI entry
in sap/cli/_entry.py.
Comment on lines
+398
to
+416
| def fetch_config_source(source: str, insecure: bool = False, | ||
| ssl_verify: bool = True) -> dict: | ||
| """Load config data from a local file path or HTTPS/HTTP URL. | ||
|
|
||
| Raises SAPCliConfigError on errors (network, parsing, validation). | ||
| Plain HTTP URLs are rejected unless insecure=True. | ||
| When ssl_verify=False, HTTPS certificate validation is skipped. | ||
| """ | ||
|
|
||
| if source.startswith('http://'): | ||
| if not insecure: | ||
| raise SAPCliConfigError( | ||
| f'Plain HTTP is not allowed for security reasons: {source}\n' | ||
| 'Use HTTPS or pass --insecure to allow plain HTTP.' | ||
| ) | ||
| return _fetch_config_from_url(source, ssl_verify=True) | ||
|
|
||
| if source.startswith('https://'): | ||
| return _fetch_config_from_url(source, ssl_verify=ssl_verify) |
There was a problem hiding this comment.
Reject HTTPS-to-HTTP redirect downgrades.
requests.get() follows redirects by default, so a nominal https://... source can still end up loading over http://... without the user ever passing --insecure. Please either disable redirects here or validate response.url before accepting the body.
Also applies to: 421-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/config.py` around lines 398 - 416,
fetch_config_source/_fetch_config_from_url currently trusts requests' redirects,
allowing an initial https:// source to be redirected to http://; update the
fetching logic so redirects cannot downgrade security — either call
requests.get(..., allow_redirects=False) or, after fetching, validate that
response.url still uses the same secure scheme as the original source (i.e., if
source started with "https://" then response.url must also start with
"https://"); apply the same check for the http branch where insecure is False to
ensure no unexpected scheme changes (refer to fetch_config_source and
_fetch_config_from_url to locate and fix the code paths).
…upport Add 'sapcli config merge --source <path-or-url>' command that merges shared configuration files (local paths or HTTPS URLs) into the user's personal config. Connections, users, and contexts are merged additively; existing entries are preserved unless --overwrite is passed. Support --insecure flag to allow plain HTTP source URLs and honor the global --skip-ssl-validation flag to skip HTTPS certificate validation, addressing environments with self-signed certs or missing CA bundles. New functions in sap/config.py: merge_into(), fetch_config_source(), _validate_config_data(), _fetch_config_from_url(), _fetch_config_from_path(). Includes 55 new tests covering validation, merge logic, URL/path fetching, insecure/SSL wiring, and CLI integration.
filak-sap
force-pushed
the
configuration_file_merge
branch
from
e3c9e51 to
188edf7
Compare
…on classes ssl_server_cert was listed in CONNECTION_FIELDS and documented but the config value was never propagated to any connection. The env var SAP_SSL_SERVER_CERT only worked for ADT and REST (read directly via os.environ in _get_session), not OData. - Add --ssl-server-cert CLI argument in _entry.py - Resolve ssl_server_cert in resolve_default_connection_values with precedence: CLI > SAP_SSL_SERVER_CERT env > config > None - Accept ssl_server_cert=None in ADT, REST, and OData constructors - Replace direct os.environ reads in ADT/REST _get_session with the constructor parameter; remove unused os imports - Add ssl_server_cert support to OData (was completely missing) - Pass ssl_server_cert through all connection factory functions - Add ssl_server_cert to build_empty_connection_values - Add 18 new tests across 5 test files
int() raises TypeError for non-string/non-numeric values like None, which YAML produces for bare 'port:' keys. Widen the except clause from ValueError to (ValueError, TypeError) so malformed configs consistently surface SAPCliConfigError.
_apply_config_extra_params used hasattr() to guard setattr(), which silently skipped config values when the attribute did not exist on args—e.g. when using build_empty_connection_values() which omits mshost, snc_*, etc. Replace the hasattr+getattr guard with getattr(args, param, None) so missing attributes are created by setattr and populated from config_values.
…mmand The current-context command blindly printed any args.name without checking it existed in the config, and did not check for a missing config file (unlike use-context and view). Now it returns non-zero with 'No configuration file found.' when config is absent, and validates that a user-supplied context name exists in config_file.contexts before printing it.
ConfigFile.load() did not call _validate_config_data(), so malformed section types (e.g. connections: []) were only caught lazily at use-time by accessor methods. Add the same validation call that _fetch_config_from_path and _fetch_config_from_url already perform so structural errors are rejected early with a clear message.
filak-sap
force-pushed
the
configuration_file_merge
branch
from
188edf7 to
2cd6a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sap/cli/__init__.py (1)
257-276:⚠️ Potential issue | 🟠 MajorPreserve CLI precedence for
--skip-ssl-validation.Lines 275-276 backfill
args.ssl_server_certfrom env/config afterargs.verifyhas already been resolved. The new SSL-server-cert tests, e.g.test/unit/test_sap_odata_connection.pyLines 169-179, make that cert path win oververify=False, so a lower-precedence config/env value can silently undo an explicit CLI--skip-ssl-validation.🛠️ Guard the backfill when `verify=False` came from the CLI
def resolve_default_connection_values(args): + verify_from_cli = getattr(args, 'verify', None) is not None @@ - if not args.ssl_server_cert: + if not args.ssl_server_cert and not (verify_from_cli and args.verify is False): args.ssl_server_cert = os.getenv('SAP_SSL_SERVER_CERT') or config_values.get('ssl_server_cert')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/cli/__init__.py` around lines 257 - 276, When backfilling args.ssl_server_cert, avoid overriding an explicit CLI skip-ssl-validation: only populate args.ssl_server_cert from env/config if args.ssl_server_cert is falsy AND the user did not pass the CLI flag that disables verification. Concretely, in the block that sets args.ssl_server_cert, add a guard that checks whether args.verify is False because the user passed the skip flag (e.g. detect '--skip-ssl-validation' in sys.argv or similar CLI token) and if so do not backfill from os.getenv('SAP_SSL_SERVER_CERT') or config_values.get('ssl_server_cert'); otherwise proceed with the existing backfill. Ensure you reference args.verify and args.ssl_server_cert when implementing the guard.
♻️ Duplicate comments (2)
sap/config.py (1)
364-397:⚠️ Potential issue | 🟡 Minor
merge_intodoes not validate that individual section entries are dicts.While
_validate_config_dataensures section containers are dicts (e.g.,connectionsis a dict), it doesn't validate that each entry within is also a dict. Ifsource_datacontainsconnections: {srv: 42},merge_intowill copy the scalar42into the target, potentially corrupting it.Consider extending validation or adding a check within the merge loop:
🛡️ Suggested defensive check
for name, value in source_section.items(): + if not isinstance(value, dict): + raise SAPCliConfigError( + f"Source configuration section '{section}' entry '{name}' is not a valid mapping" + ) if name in target_section and not overwrite: summary['skipped'][section].append(name) else:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/config.py` around lines 364 - 397, merge_into currently trusts that each entry in a MERGEABLE_SECTIONS mapping is a dict; add a defensive check inside the loop over "for name, value in source_section.items()" in merge_into to verify isinstance(value, dict) (or mapping) before merging, and if it is not a dict, do not copy it into target.data[section] and record the name in summary['skipped'][section] (optionally log a warning); this keeps _validate_config_data as-is but prevents scalar or invalid entries from corrupting target.data when merging.sap/cli/__init__.py (1)
185-201:⚠️ Potential issue | 🟠 MajorRFC/SNC env defaults are still skipped on the helper path.
The new
getattr()check fixes the missing-attribute case for config-backed values, but this path still never readsSAP_MSHOST,SAP_MSSERV,SAP_SYSID,SAP_GROUP, orSNC_*. Callers that usebuild_empty_connection_values()and thenresolve_default_connection_values()will still drop those environment defaults, and env-over-config precedence remains wrong there.🛠️ Apply env values before falling back to config
- for param in extra_params: - if not getattr(args, param, None) and param in config_values: - setattr(args, param, config_values[param]) + for param, env_var in extra_params.items(): + if getattr(args, param, None): + continue + + env_value = os.getenv(env_var) + if env_value: + setattr(args, param, env_value) + elif param in config_values: + setattr(args, param, config_values[param])Also applies to: 312-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/cli/__init__.py` around lines 185 - 201, The helper build_empty_connection_values() currently omits RFC/SNC-related environment keys so callers who create an empty namespace then call resolve_default_connection_values() never get RFC/SNC env defaults; update build_empty_connection_values() to include the missing env-backed fields (e.g. mshost/mssrv/sysid/group and all SNC_* fields used by resolve_default_connection_values()) so resolve_default_connection_values() can apply environment values before falling back to config, or alternatively modify resolve_default_connection_values() to explicitly check os.environ for SAP_MSHOST, SAP_MSSERV, SAP_SYSID, SAP_GROUP and SNC_* when a field on the passed namespace is None; reference build_empty_connection_values() and resolve_default_connection_values() when making the change.
🧹 Nitpick comments (2)
sap/rest/connection.py (1)
166-173: Minor: Log message references env var name but value now comes from CLI.The log message at line 172 says
SAP_SSL_VERIFY = no, which is accurate for context. However, you might consider updating this to be more generic since theverify=Falsecould also come from config file or CLI flag, not just the env var.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/rest/connection.py` around lines 166 - 173, The log message currently hardcodes "SAP_SSL_VERIFY = no" even though the decision to set self._session.verify = False may come from CLI or config; update the mod_log().info call inside the elif block that checks self._ssl_verify to use a generic message (e.g., "SSL Server cert will not be verified") or include the actual source of the setting if available; change the log invocation referencing self._ssl_verify, self._session.verify and mod_log().info to remove the env var name and reflect the generic/actual source.test/unit/test_sap_rest_connection.py (1)
117-153: LGTM! Good test coverage, consider addingverify=Truedefault test for consistency.The tests properly verify the core
ssl_server_certbehaviors. For consistency withTestADTConnectionSSLServerCert, you might consider adding a test that verifiessession.verifydefaults toTruewhenssl_server_certis not provided (similar totest_ssl_server_cert_none_verify_truein the ADT tests).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_rest_connection.py` around lines 117 - 153, Add a unit test to mirror the ADT suite that asserts session.verify defaults to True when no ssl_server_cert is provided: inside TestConnectionSSLServerCert add a method (e.g., test_ssl_server_cert_none_verify_true) that constructs Connection(...) without ssl_server_cert, calls Connection._get_session(), and asserts session.verify is True; reference the TestConnectionSSLServerCert class, the Connection constructor, and the _get_session method to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/test_sap_odata_connection.py`:
- Around line 147-149: Tests instantiate Connection solely for side effects but
assign it to an unused variable named con, causing lint F841; remove the unused
bindings by replacing assignments like "con = Connection(...)" with a plain call
"Connection(...)" (or assign to _ if you prefer) in the three locations where
Connection is only used for side effects (the instantiations that include
ssl_server_cert and similar args), and ensure no later code depends on the con
variable.
---
Outside diff comments:
In `@sap/cli/__init__.py`:
- Around line 257-276: When backfilling args.ssl_server_cert, avoid overriding
an explicit CLI skip-ssl-validation: only populate args.ssl_server_cert from
env/config if args.ssl_server_cert is falsy AND the user did not pass the CLI
flag that disables verification. Concretely, in the block that sets
args.ssl_server_cert, add a guard that checks whether args.verify is False
because the user passed the skip flag (e.g. detect '--skip-ssl-validation' in
sys.argv or similar CLI token) and if so do not backfill from
os.getenv('SAP_SSL_SERVER_CERT') or config_values.get('ssl_server_cert');
otherwise proceed with the existing backfill. Ensure you reference args.verify
and args.ssl_server_cert when implementing the guard.
---
Duplicate comments:
In `@sap/cli/__init__.py`:
- Around line 185-201: The helper build_empty_connection_values() currently
omits RFC/SNC-related environment keys so callers who create an empty namespace
then call resolve_default_connection_values() never get RFC/SNC env defaults;
update build_empty_connection_values() to include the missing env-backed fields
(e.g. mshost/mssrv/sysid/group and all SNC_* fields used by
resolve_default_connection_values()) so resolve_default_connection_values() can
apply environment values before falling back to config, or alternatively modify
resolve_default_connection_values() to explicitly check os.environ for
SAP_MSHOST, SAP_MSSERV, SAP_SYSID, SAP_GROUP and SNC_* when a field on the
passed namespace is None; reference build_empty_connection_values() and
resolve_default_connection_values() when making the change.
In `@sap/config.py`:
- Around line 364-397: merge_into currently trusts that each entry in a
MERGEABLE_SECTIONS mapping is a dict; add a defensive check inside the loop over
"for name, value in source_section.items()" in merge_into to verify
isinstance(value, dict) (or mapping) before merging, and if it is not a dict, do
not copy it into target.data[section] and record the name in
summary['skipped'][section] (optionally log a warning); this keeps
_validate_config_data as-is but prevents scalar or invalid entries from
corrupting target.data when merging.
---
Nitpick comments:
In `@sap/rest/connection.py`:
- Around line 166-173: The log message currently hardcodes "SAP_SSL_VERIFY = no"
even though the decision to set self._session.verify = False may come from CLI
or config; update the mod_log().info call inside the elif block that checks
self._ssl_verify to use a generic message (e.g., "SSL Server cert will not be
verified") or include the actual source of the setting if available; change the
log invocation referencing self._ssl_verify, self._session.verify and
mod_log().info to remove the env var name and reflect the generic/actual source.
In `@test/unit/test_sap_rest_connection.py`:
- Around line 117-153: Add a unit test to mirror the ADT suite that asserts
session.verify defaults to True when no ssl_server_cert is provided: inside
TestConnectionSSLServerCert add a method (e.g.,
test_ssl_server_cert_none_verify_true) that constructs Connection(...) without
ssl_server_cert, calls Connection._get_session(), and asserts session.verify is
True; reference the TestConnectionSSLServerCert class, the Connection
constructor, and the _get_session method to locate where to add the test.
🪄 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: 708c3ba8-4f8d-423d-801c-8d97e9274639
📒 Files selected for processing (16)
AGENTS.mddoc/configuration.mdsap/adt/core.pysap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pysap/odata/connection.pysap/rest/connection.pytest/unit/test_sap_adt_connection.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.pytest/unit/test_sap_odata_connection.pytest/unit/test_sap_rest_connection.py
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- sap/cli/_entry.py
- test/unit/test_sap_cli_config.py
Comment on lines
+147
to
+149
| con = Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD', | ||
| True, True, ssl_server_cert='/path/to/ca.pem') | ||
| self.assertEqual(session.verify, '/path/to/ca.pem') |
There was a problem hiding this comment.
Drop the unused con bindings.
Lines 147, 162, and 177 instantiate Connection only for its side effects, so binding the result to con trips F841 and can fail linting even though the variable is never read.
🧹 Minimal cleanup
- con = Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
- True, True, ssl_server_cert='/path/to/ca.pem')
+ Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
+ True, True, ssl_server_cert='/path/to/ca.pem')
@@
- con = Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
- True, True)
+ Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
+ True, True)
@@
- con = Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
- True, False, ssl_server_cert='/path/to/ca.pem')
+ Connection('SERVICE', 'HOST', 'PORT', 'CLIENT', 'USER', 'PASSWORD',
+ True, False, ssl_server_cert='/path/to/ca.pem')Also applies to: 162-164, 177-179
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 147-147: local variable 'con' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/test_sap_odata_connection.py` around lines 147 - 149, Tests
instantiate Connection solely for side effects but assign it to an unused
variable named con, causing lint F841; remove the unused bindings by replacing
assignments like "con = Connection(...)" with a plain call "Connection(...)" (or
assign to _ if you prefer) in the three locations where Connection is only used
for side effects (the instantiations that include ssl_server_cert and similar
args), and ensure no later code depends on the con variable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sap/adt/core.py (1)
226-229: Avoid logging full cert path at INFO and validate path early.Line 228 logs the full certificate path at INFO level, which can expose local filesystem/user details in shared logs. Also, validating the path up front gives a clearer error than failing later during request send.
Proposed hardening patch
+import os import xml.sax from xml.sax.handler import ContentHandler @@ # requests.session.verify is either boolean or path to CA to use! if self._ssl_server_cert: + if not os.path.isfile(self._ssl_server_cert): + raise ValueError(f'Invalid SSL server cert path: {self._ssl_server_cert}') self._session.verify = self._ssl_server_cert - mod_log().info('Using custom SSL Server cert path: %s', self._session.verify) + mod_log().debug( + 'Using custom SSL server cert file: %s', + os.path.basename(self._ssl_server_cert), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/adt/core.py` around lines 226 - 229, Do not log full certificate paths at INFO and validate the cert path early: check self._ssl_server_cert for existence/readability during initialization (e.g., where the client/session is created) and raise a clear error if invalid, then set self._session.verify = self._ssl_server_cert without printing the path; replace the mod_log().info('Using custom SSL Server cert path: %s', self._session.verify) call with a non-sensitive message at debug/trace (or no path), e.g., mod_log().debug('Custom SSL server cert configured') and ensure validation uses functions like os.path.exists/os.access so failures surface before requests are sent.sap/rest/connection.py (1)
59-60: Fail fast on invalidssl_server_certpath.Right now a bad path is only discovered during the first HTTP call with a low-level SSL/connection error. Validate it in
__init__to raise a clear error earlier.Proposed refactor
import json +from pathlib import Path @@ def __init__(self, icf_path, login_path, host, client, user, password, port=None, ssl=True, verify=True, ssl_server_cert=None): @@ - self._ssl_verify = verify - self._ssl_server_cert = ssl_server_cert + self._ssl_verify = verify + if ssl_server_cert: + cert_path = Path(ssl_server_cert) + if not cert_path.exists(): + raise FileNotFoundError(f"SSL server certificate path does not exist: {ssl_server_cert}") + self._ssl_server_cert = ssl_server_certAlso applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/rest/connection.py` around lines 59 - 60, In __init__ validate the ssl_server_cert parameter early: if ssl_server_cert is provided, check (via pathlib.Path(...).is_file() or os.path.exists/isfile) that the path exists and is a file, and raise a clear exception (ValueError or FileNotFoundError) with a message referencing the bad path so callers fail fast; apply the same validation to the other constructor/initializer referenced around line 85 so both places detect invalid ssl_server_cert paths at creation time rather than at first HTTP call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sap/rest/connection.py`:
- Around line 166-168: The code logs the full certificate path at INFO which can
leak local details; change the logging in the block that sets
self._session.verify (referencing _ssl_server_cert and _session.verify) to avoid
printing the full path at INFO—either lower the level to debug (use
mod_log().debug) or log only the filename portion
(os.path.basename(self._ssl_server_cert)) with a minimal message like "Using
custom SSL Server cert: <filename>", keeping the assignment to
self._session.verify unchanged.
---
Nitpick comments:
In `@sap/adt/core.py`:
- Around line 226-229: Do not log full certificate paths at INFO and validate
the cert path early: check self._ssl_server_cert for existence/readability
during initialization (e.g., where the client/session is created) and raise a
clear error if invalid, then set self._session.verify = self._ssl_server_cert
without printing the path; replace the mod_log().info('Using custom SSL Server
cert path: %s', self._session.verify) call with a non-sensitive message at
debug/trace (or no path), e.g., mod_log().debug('Custom SSL server cert
configured') and ensure validation uses functions like os.path.exists/os.access
so failures surface before requests are sent.
In `@sap/rest/connection.py`:
- Around line 59-60: In __init__ validate the ssl_server_cert parameter early:
if ssl_server_cert is provided, check (via pathlib.Path(...).is_file() or
os.path.exists/isfile) that the path exists and is a file, and raise a clear
exception (ValueError or FileNotFoundError) with a message referencing the bad
path so callers fail fast; apply the same validation to the other
constructor/initializer referenced around line 85 so both places detect invalid
ssl_server_cert paths at creation time rather than at first HTTP call.
🪄 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: 921407ed-8fa2-4291-bdef-002aff7bdacf
📒 Files selected for processing (15)
doc/configuration.mdsap/adt/core.pysap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pysap/odata/connection.pysap/rest/connection.pytest/unit/test_sap_adt_connection.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.pytest/unit/test_sap_odata_connection.pytest/unit/test_sap_rest_connection.py
✅ Files skipped from review due to trivial changes (3)
- test/unit/test_sap_cli__entry.py
- test/unit/test_sap_rest_connection.py
- sap/config.py
🚧 Files skipped from review as they are similar to previous changes (6)
- sap/cli/_entry.py
- sap/cli/init.py
- sap/odata/connection.py
- test/unit/test_sap_adt_connection.py
- test/unit/test_sap_cli.py
- sap/cli/config.py
Comment on lines
+166
to
+168
| if self._ssl_server_cert: | ||
| self._session.verify = self._ssl_server_cert | ||
| mod_log().info('Using custom SSL Server cert path: %s', self._session.verify) |
There was a problem hiding this comment.
Avoid logging full cert paths at INFO level.
Line 168 logs the full certificate path, which can leak local user/machine details in shared logs. Prefer debug level and/or log only filename.
Safer logging tweak
- mod_log().info('Using custom SSL Server cert path: %s', self._session.verify)
+ mod_log().debug('Using custom SSL Server cert: %s', self._ssl_server_cert)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/rest/connection.py` around lines 166 - 168, The code logs the full
certificate path at INFO which can leak local details; change the logging in the
block that sets self._session.verify (referencing _ssl_server_cert and
_session.verify) to avoid printing the full path at INFO—either lower the level
to debug (use mod_log().debug) or log only the filename portion
(os.path.basename(self._ssl_server_cert)) with a minimal message like "Using
custom SSL Server cert: <filename>", keeping the assignment to
self._session.verify unchanged.
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
2 participants
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.
No description provided.