fix: replace bare except clauses and add path validation in PAM base#1901
Open
jlima8900 wants to merge 9 commits intoKeeper-Security:releasefrom
Open
fix: replace bare except clauses and add path validation in PAM base#1901jlima8900 wants to merge 9 commits intoKeeper-Security:releasefrom
jlima8900 wants to merge 9 commits intoKeeper-Security:releasefrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| obj = cls() | ||
| try: data = json.loads(data) if isinstance(data, str) else data | ||
| except: logging.error(f"Pam Scripts failed to load from: {str(data)[:80]}...") | ||
| except json.JSONDecodeError as e: logging.error(f"Pam Scripts: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
| obj = cls() | ||
| try: data = json.loads(data) if isinstance(data, str) else data | ||
| except: logging.error(f"PAM script failed to load from: {str(data)[:80]}") | ||
| except json.JSONDecodeError as e: logging.error(f"PAM script: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
| obj = cls() | ||
| try: data = json.loads(data) if isinstance(data, str) else data | ||
| except: logging.error(f"PAM Attachments failed to load from: {str(data)[:80]}...") | ||
| except json.JSONDecodeError as e: logging.error(f"PAM Attachments: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
| obj = cls() | ||
| try: data = json.loads(data) if isinstance(data, str) else data | ||
| except: logging.error(f"Failed to load file attachment from: {str(data)[:80]}") | ||
| except json.JSONDecodeError as e: logging.error(f"File attachment: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
| obj = cls() | ||
| try: data = json.loads(data) if isinstance(data, str) else data | ||
| except: logging.error(f"Failed to load rotation schedule from: {str(data)[:80]}") | ||
| except json.JSONDecodeError as e: logging.error(f"Rotation schedule: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
jlima8900
force-pushed
the
fix/pam-base-error-handling-pr1892
branch
2 times, most recently
from
e6e690a to
6ad7fd9
Compare
Contributor
Author
|
@aaunario — ready for review. Squashed to single commit, all CodeQL alerts resolved, 16 unit tests passing. |
jlima8900
force-pushed
the
fix/pam-base-error-handling-pr1892
branch
from
a38b94d to
871b277
Compare
* Added full terminal reset after ssh session exit (clears scrollback etc.) * Fixes randomly eaten first characters typed after ssh session exit (stdin race condition) * Fixed random duplicate typed characters (echo)
Keeper-Security#1908) - Added `move` to the list of `apply-action` choices, which will move all returned users to a node - specified with `target-node` - Added `all` to the list of `status` choices, returning invited, active and locked users with `d=0` - Fixed an issue where the users updated with `apply-action` are not the same as those filtered with the `node` argument. - Fixed a potential unwanted behavior where the `node` argument returns a recursive node and subnodes search, preventing you from applying actions to a specific node if it has subnodes. By default, using the `node` argument will only return result from the specified node. - Added a `recursive` argument to replicate the old behavior with `node` filter.
- Narrow all bare except clauses to json.JSONDecodeError with structured error messages (line, col, msg) across 30+ load() methods - Replace em-dashes with double-dashes in all log messages - Add TypeError, AttributeError to CRON except tuple - Extract shared _validate_file_path() helper (DRY) - Add path traversal rejection (.. components) - Add tests for rotation settings, base machine parser, CRON edge cases
jlima8900
force-pushed
the
fix/pam-base-error-handling-pr1892
branch
from
871b277 to
c011296
Compare
Prevents directories from passing file path validation.
sk-keeper
force-pushed
the
release
branch
3 times, most recently
from
442d7af to
9ec9b7a
Compare
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
7 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.
Summary
Replaces all 35 bare
except:clauses inkeepercommander/commands/pam_import/base.pywith specific exception types (JSONDecodeError,ValueError,KeyError) and adds path traversal validation for file operations. Also removes truncated input data from error log messages to resolve CodeQL clear-text logging alerts.Changes
except:→ specific types — each handler now catches only the expected exceptionos.path.realpathbefore access{str(data)[:80]}from all error messages (CodeQL fix)Tests
test_pam_base_error_handling.pyCodeQL
All clear-text logging alerts resolved in this single squashed commit.