fix: reject HP_SHARED_KEY with FRP-incompatible characters#104
Merged
Conversation
…onfig Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
📝 WalkthroughWalkthroughstart.sh now strips surrounding quotes from HP_SHARED_KEY then validates the value and aborts if it contains a double quote (") or backslash (), preventing malformed FRP TOML token lines. The haproxy_agent /info HTTP endpoint JSON now reports "version": 0.4 (was 0.3). 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@start.sh`:
- Around line 131-137: The forbidden-character check runs on HP_SHARED_KEY
before normalization; call the existing strip_quotes normalization on
HP_SHARED_KEY first (i.e., replace HP_SHARED_KEY with the normalized value
produced by strip_quotes) and then compute FORBIDDEN_COUNT (the tr/wc check) so
wrapped values like 'secret' or "secret" are accepted while still rejecting
embedded double-quote, single-quote, or backslash characters; update references
to FORBIDDEN_COUNT and any subsequent usage to use the normalized HP_SHARED_KEY
variable.
🪄 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 Plus
Run ID: 1727ca64-aa21-4de8-9666-06fd34ed326f
📒 Files selected for processing (2)
haproxy_agent.pystart.sh
…ing apostrophes Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
start.sh (1)
130-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApostrophe is still silently lossy with current normalization flow.
At Line 127,
strip_quotes()removes leading/trailing', but Lines 130-141 no longer reject'. This can still mutate a valid key (e.g., edge apostrophes) and lead to FRP token mismatch. If apostrophes are not fully supported end-to-end, they should remain rejected here.💡 Minimal fix
-FORBIDDEN_COUNT=$(printf '%s' "$HP_SHARED_KEY" | LC_ALL=C tr -cd '"\\' | wc -c) +FORBIDDEN_COUNT=$(printf '%s' "$HP_SHARED_KEY" | LC_ALL=C tr -cd "\"\\\\'" | wc -c) if [ "$FORBIDDEN_COUNT" -gt 0 ]; then echo "ERROR: HP_SHARED_KEY contains a forbidden character." - echo "The following characters are not allowed: double quote (\"), backslash (\\)." + echo "The following characters are not allowed: double quote (\"), single quote ('), backslash (\\)." echo "Please choose a password without these characters." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@start.sh` around lines 130 - 141, The current validation after strip_quotes() checks only for double quotes and backslashes but not apostrophes, which allows silent lossiness when strip_quotes() removes edge single quotes; update the forbidden-character check for HP_SHARED_KEY (used with strip_quotes()) to include the single quote character (') in the set counted by FORBIDDEN_COUNT and update the error message to mention single quotes as disallowed so keys containing apostrophes are rejected rather than silently mutated; locate the FORBIDDEN_COUNT calculation and the error output block that references HP_SHARED_KEY to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@start.sh`:
- Around line 130-141: The current validation after strip_quotes() checks only
for double quotes and backslashes but not apostrophes, which allows silent
lossiness when strip_quotes() removes edge single quotes; update the
forbidden-character check for HP_SHARED_KEY (used with strip_quotes()) to
include the single quote character (') in the set counted by FORBIDDEN_COUNT and
update the error message to mention single quotes as disallowed so keys
containing apostrophes are rejected rather than silently mutated; locate the
FORBIDDEN_COUNT calculation and the error output block that references
HP_SHARED_KEY to make this change.
kyteinsky
approved these changes
May 11, 2026
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Rejects FRP-incompatible characters in HP_SHARED_KEY earlier to prevent generating invalid FRP TOML and bumps the /info endpoint version.
Changes:
- Add validation in
start.shto rejectHP_SHARED_KEYvalues containing TOML-breaking characters. - Improve error messaging for invalid
HP_SHARED_KEY. - Bump
/infoendpoint version from0.3to0.4.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| start.sh | Adds upfront validation for HP_SHARED_KEY to prevent corrupt FRP TOML generation. |
| haproxy_agent.py | Updates /info response version to 0.4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+141
| FORBIDDEN_COUNT=$(printf '%s' "$HP_SHARED_KEY" | LC_ALL=C tr -cd '"\\' | wc -c) | ||
| if [ "$FORBIDDEN_COUNT" -gt 0 ]; then | ||
| echo "ERROR: HP_SHARED_KEY contains a forbidden character." | ||
| echo "The following characters are not allowed: double quote (\"), backslash (\\)." | ||
| echo "Please choose a password without these characters." | ||
| exit 1 | ||
| fi |
Contributor
Author
There was a problem hiding this comment.
Updated PR description 👍
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
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.
Reject
HP_SHARED_KEYvalues that contain"or\after the existingstrip_quotesnormalization, with a clear error message naming the forbidden characters. These characters otherwise corrupt the generated FRP TOML linemetadatas.token = "..."and causefrpcto fail at startup.The check runs after
strip_quotesso that accidentally-wrapped values like"secret"/'secret'(the use case PR #83 addedstrip_quotesfor) keep working. The single-quote character is intentionally not forbidden: it is valid inside a TOML basic string and rejecting it would break legitimate apostrophe-in-passphrase values (e.g.don'tguessit).Also bumps
/infoendpoint version to 0.4.Fixes #103