Fix bugs, security issues, and reliability problems#146
Fix bugs, security issues, and reliability problems#146stephen-bailey-carta wants to merge 1 commit intogrokability:mainfrom
Conversation
**Critical bugs fixed:** - `search_jamf_mobile` called `search_jamf_asset` on rate-limit retry, returning computer data instead of mobile device data - `get_snipe_models` checked `response.status_code` instead of `newresponse.status_code` after second page fetch - `get_snipe_users` used mutable default argument (`previous=[]`), causing data leakage across calls - `logging.verbose(jamf)` crashes at runtime — `verbose` is not a valid logging level **Security fixes:** - XML injection in `update_jamf_asset_tag` and `update_jamf_mobiledevice_asset_tag` — asset tags are now escaped via `xml.sax.saxutils.escape` - Bare `except:` clauses replaced with specific exception types throughout - Snipe API key no longer logged in debug output **Reliability fixes:** - Added 30-second timeout to all HTTP requests (21 calls) to prevent indefinite hangs - Removed `id` builtin shadowing in `get_snipe_user_id` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ParadoxGuitarist
left a comment
ParadoxGuitarist
left a comment
There was a problem hiding this comment.
Hey, thanks for opening a PR! :)
Just to clarify, I don't work for Snipe or anything, but there's some changes I would make and some things @uberbrady or @snipe might need to weigh in on.
| logging.debug("The client_secret you provided for Jamf is: {}".format(jamf_client_secret)) | ||
|
|
||
| # This is the address, cname, or FQDN for your snipe-it instance. | ||
| logging.info("Setting the base URL for SnipeIT.") | ||
| snipe_base = config['snipe-it']['url'] | ||
| logging.debug("The configured Snipe-IT base url is: {}".format(snipe_base)) | ||
|
|
||
| logging.info("Setting the API key for SnipeIT.") | ||
| snipe_apiKey = config['snipe-it']['apikey'] | ||
| logging.debug("The API key you provided for Snipe is: {}".format(snipe_apiKey)) | ||
| logging.debug("Snipe API key is set (not logging for security).") |
There was a problem hiding this comment.
I'm a little confused about what drives part of the change here.
The debug level only runs when it's called so we're not logging these things by default and it's the MOST verbose out of any of the logging levels. Personally I think it's useful as a double idiot check to make sure:
- We're using the right config.
- That config has the right key.
Maybe there's something I don't know about python logging where that gets run either way? I don't think it does, and running it in debug mode add thousands of lines (or even hundreds of thousands depending on org size). Either way, the Jamf client secret is listed right above here , but we're not removing it there?
Personally I would revert line R157, but if we don't I think we'd need to remove the key from R144 for consistency/concerns.
There was a problem hiding this comment.
I was actually thinking about this after I posted and I realized that I think debug mode dumps all the request (including the headers) So all keys are getting exposed there (even if they're base64 encoded) so trying to remove is mostly a fruitless venture, but maybe there's a different side to it. I'd trust @snipe or @uberbrady's opinions a lot more than mine when it comes to security. :)
| # A list of valid subsets are: | ||
| version = "1.0.7" | ||
|
|
||
| DEFAULT_TIMEOUT = 30 |
There was a problem hiding this comment.
Would it be possible to change this?
Ideally we should pass this in either from the setting file, or as a runtime variable.
I think you could stick this down in the variable section (R70 or so)
runtimeargs.add_argument("-t", "--timeout", type=int, default=10, help="Sets the DEFAULT_TIMEOUT in seconds for API calls. Expects an integer.")
Last thing, is 30 seconds still too long? It seems like a really long time if the script continues considering it's probably making thousands of calls?
| @@ -32,6 +32,8 @@ | |||
| # A list of valid subsets are: | |||
| version = "1.0.7" | |||
There was a problem hiding this comment.
nitpick: should probably bump this to 1.1.0
| logging.error("Could not generate an asset tag for this device. Skipping") | ||
| # Dump the object for debugging. | ||
| logging.verbose(jamf) | ||
| logging.debug(jamf) |
There was a problem hiding this comment.
Since all the payloads get dumped in debug mode, I'd leave this as verbose.
We're just adding more noise in debug mode like this and making verbose mode less useful for an in between step.
2 participants
Summary
Targeted fixes for bugs, security issues, and reliability problems — no structural changes.
Critical bugs fixed
search_jamf_mobilecalledsearch_jamf_asseton rate-limit retry (line 416), returning computer data instead of mobile device dataget_snipe_modelscheckedresponse.status_codeinstead ofnewresponse.status_codeafter fetching the second page of models, so failures on the second request were never caughtget_snipe_usersused mutable default argument (previous=[]), a classic Python gotcha where the default list persists across calls and leaks datalogging.verbose(jamf)crashes at runtime — Python's logging module has noverboselevel; changed tologging.debugSecurity fixes
update_jamf_asset_tag/update_jamf_mobiledevice_asset_tag— asset tags containing<,>, or&could break or exploit XML payloads; now escaped viaxml.sax.saxutils.escapeexcept:clauses replaced with specific exception types (KeyError,ValueError,TypeError, etc.) throughout — bare excepts catchKeyboardInterruptandSystemExit, masking real errorsReliability fixes
idbuiltin shadowing inget_snipe_user_idTest plan
python3 -c "import py_compile; py_compile.compile('jamf2snipe', doraise=True)"--dryrunto verify no behavioral changes in normal flowsearch_jamf_mobilefix🤖 Generated with Claude Code