Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-server "history username env" support: config example entries, DB migration to add mode/format columns, model resync trigger, UI/JS to edit/display options, server-side validation, and sync/key-generation changes to emit per-user per-server environment options into authorized_keys. Changes
Sequence DiagramsequenceDiagram
participant Admin as Administrator
participant UI as Server Settings UI
participant Model as Server Model
participant DB as Database
participant Sync as Sync Service
participant Keys as Authorized_Keys
Admin->>UI: Edit history_username_env mode & format, submit
UI->>Model: Save server settings (mode, format)
Model->>DB: UPDATE server.history_username_env_*
DB-->>Model: OK
Model->>Model: Mark server for resync
Sync->>DB: Read server config for server
DB-->>Sync: Return mode & format (or inherit)
Sync->>Sync: Resolve effective env option (global → server → user)
Sync->>Keys: Generate/write authorized_keys entries with env option prefix
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
🤖 Fix all issues with AI agents
In `@scripts/sync.php`:
- Around line 510-516: The function get_global_history_username_env_enabled
currently defaults to true when
config['privacy']['history_username_env_default'] is absent, which can
unexpectedly inject environment="BASH_HISTORY_USERNAME=..." into
authorized_keys; change its default to false so it returns true only when
history_username_env_default is explicitly set to 1 (e.g., return
intval($config['privacy']['history_username_env_default']) === 1 when key
exists, otherwise return false), and update any upgrade notes or docs to mention
the behavioral change regarding authorized_keys injection and
PermitUserEnvironment.
- Around line 562-564: The escape_authorized_keys_option_value function must
also remove newlines and other control characters before escaping to prevent
breaking authorized_keys entries; update escape_authorized_keys_option_value to
first strip control characters (e.g. using a regex that removes [[:cntrl:]]
characters including "\r" and "\n") from $value, then perform the existing
backslash/quote escaping so the returned string contains no control characters
that could break or inject lines in authorized_keys.
In `@views/server.php`:
- Around line 124-136: Validate and sanitize history_username_env_format on
input before storing: after trimming and checking for empty, ensure the string
contains the required "{uid}" and matches a strict whitelist regex (e.g. allow
only letters, digits, spaces and a small safe set of symbols such as - _ . @ : +
and the braces for {uid}) and explicitly reject or null-out any value containing
newline, comma, equals, quotes, backslash or other disallowed chars; enforce the
same validation in normalize_history_username_env_format so storage and later
use (and escape_authorized_keys_option_value in scripts/sync.php) cannot be
bypassed. Reference variables/functions: $history_username_env_format,
$server->history_username_env_format, normalize_history_username_env_format(),
and escape_authorized_keys_option_value().
🧹 Nitpick comments (2)
templates/server.php (1)
390-393: Inconsistent indentation on the no-pty checkbox.The
no-ptycheckbox div (lines 390-392) has an extra indentation level compared to its sibling checkbox divs (lines 384-389). This appears to be an unintended whitespace change.Proposed fix
- <div class="checkbox"> - <label><input type="checkbox" name="access_option[no-pty][enabled]"<?php if(isset($options['no-pty'])) out(' checked'); ?>> Disable terminal (<code>no-pty</code>)</label> - </div> + <div class="checkbox"> + <label><input type="checkbox" name="access_option[no-pty][enabled]"<?php if(isset($options['no-pty'])) out(' checked'); ?>> Disable terminal (<code>no-pty</code>)</label> + </div>scripts/sync.php (1)
574-580: Consider validating that the expanded format contains a=sign.OpenSSH's
environmentoption expectsNAME=valuesyntax. If an admin configures a format like{uid}(without an=), the resultingenvironment="jsmith"is technically malformed.normalize_history_username_env_formatchecks for{uid}presence but not structural validity. A quick check could prevent subtle misconfigurations.
Msprg
force-pushed
the
feature/configurable-history-username-env
branch
from
4069cfb to
3defb4b
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Default get_global_history_username_env_enabled to false instead of true to avoid breaking exisitng deployments Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…r.php - Added validation functions for history username environment format and value. - Updated normalization and user history environment option functions to utilize new validation. - Improved handling of history username environment format in server settings. This ensures that invalid formats are properly handled, enhancing overall robustness.
…ction in sync.php - Added a regex to remove control characters from the input value. - Ensured that null values are converted to empty strings before validation. This improves the robustness of the authorized keys option handling.
Msprg
force-pushed
the
feature/configurable-history-username-env
branch
from
d957102 to
9c8a8d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/sync.php`:
- Around line 502-527: The validation currently rejects '=' causing the default
format and values to fail; update history_username_env_format_is_valid and
history_username_env_value_is_valid to allow '=' by removing it from the
blacklist patterns (/[\r\n,=\'"\\\\]/ and /[\r\n,=\'"\\\\{}]/) and include '='
in the whitelist character classes (/^[A-Za-z0-9 ._@:+{}-]+$/ and /^[A-Za-z0-9
._@:+-]+$/) so that formats like "BASH_HISTORY_USERNAME={uid}" and values like
"BASH_HISTORY_USERNAME=john" pass validation (this will restore behavior used by
get_user_history_username_env_option()).
In `@views/server.php`:
- Around line 41-56: The function history_username_env_format_is_valid(...) is
duplicated; extract this function into a single shared include (e.g.,
sync-common.php or utilities include) and remove the duplicate definitions, then
require_once that shared file from both places that currently define it (the
server.php and scripts/sync.php locations) so both use the same implementation;
keep the function name unchanged, update the includes to require_once the new
file, and run tests to ensure no symbol collisions or missing includes.
- Around line 141-159: The user-facing alert says equals signs are allowed but
history_username_env_format_is_valid currently rejects '='; update the
validation function history_username_env_format_is_valid to accept '=' (and
ensure it still accepts alphanumerics, underscores, hyphens, and curly braces),
or alternatively change the alert text to list exactly what the validator
allows; make the code paths around $history_username_env_format (where the alert
is created and $server->history_username_env_format is set) consistent with the
validator so the message matches the actual allowed characters.
Comment on lines
+141
to
+159
| $history_username_env_mode = isset($_POST['history_username_env_mode']) ? $_POST['history_username_env_mode'] : 'inherit'; | ||
| if($history_username_env_mode !== 'inherit' && $history_username_env_mode !== 'enabled' && $history_username_env_mode !== 'disabled') { | ||
| $history_username_env_mode = 'inherit'; | ||
| } | ||
| $history_username_env_format = null; | ||
| if(isset($_POST['history_username_env_format'])) { | ||
| $history_username_env_format = trim($_POST['history_username_env_format']); | ||
| if($history_username_env_format === '') { | ||
| $history_username_env_format = null; | ||
| } elseif(!history_username_env_format_is_valid($history_username_env_format)) { | ||
| $alert = new UserAlert; | ||
| $alert->content = "Invalid history username env format. Only alphanumeric characters, underscores, hyphens, equals signs, and curly braces are allowed."; | ||
| $alert->class = 'danger'; | ||
| $active_user->add_alert($alert); | ||
| $history_username_env_format = null; | ||
| } | ||
| } | ||
| $server->history_username_env_mode = $history_username_env_mode; | ||
| $server->history_username_env_format = $history_username_env_format; |
There was a problem hiding this comment.
Mode validation and format handling look correct, pending the = fix.
The mode normalization (Lines 141-144) properly constrains to known values. Format validation (Lines 146-156) provides a clear user alert on invalid input. However, note that the error message at Line 152 says "equals signs" are allowed, but the validation actually rejects them — this will need to be corrected alongside the = fix in the validation function.
🤖 Prompt for AI Agents
In `@views/server.php` around lines 141 - 159, The user-facing alert says equals
signs are allowed but history_username_env_format_is_valid currently rejects
'='; update the validation function history_username_env_format_is_valid to
accept '=' (and ensure it still accepts alphanumerics, underscores, hyphens, and
curly braces), or alternatively change the alert text to list exactly what the
validator allows; make the code paths around $history_username_env_format (where
the alert is created and $server->history_username_env_format is set) consistent
with the validator so the message matches the actual allowed characters.
…sync.php - Updated regex to exclude additional characters in format and value validation functions. - Enhanced the allowed character set for the format to include '=' and '-' for better flexibility. This improves the accuracy of validation for history username environment inputs.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- Moved the history username environment format validation function from sync.php and server.php to a new common file, history_username_env_common.php. - Updated sync.php and server.php to include the new common file for validation, improving code organization and reusability. This change centralizes the validation logic, enhancing maintainability and consistency across the application.
…on in server.php - Revised the alert content to specify the allowed characters and format requirements for the history username environment. - This change enhances clarity for users encountering validation errors, ensuring they understand the expected input format.
…file
- Added a check to ensure the format includes an '=' character in the history_username_env_format_is_valid function.
- Updated the error message in server.php to reflect the new validation requirement, clarifying that both '=' and '{uid}' must be present.
This improves the accuracy of format validation and user feedback for history username environments.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
1 participant
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 by CodeRabbit
New Features
Chores
Bug Fixes