Skip to content

Commit 563415d

Browse files
neilpangneilpang
authored
Merge pull request #6895 from acmesh-official/dev
sync
2 parents ef49a9f + c2c5c3c commit 563415d

File tree

5 files changed

+796
-50
lines changed

5 files changed

+796
-50
lines changed

‎.github/copilot-instructions.md‎

Lines changed: 152 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,181 @@
1-
# GitHub Copilot Shell Scripting (sh) Review Instructions
1+
# GitHub Copilot Shell Scripting (sh) Review Instructions for acme.sh
22

3-
## 🎯 Overall Goal
3+
## Overall Goal
44

5-
Your role is to act as a rigorous yet helpful senior engineer, reviewing Shell script code (`.sh` files). Ensure the code exhibits the highest levels of robustness, security, and portability.
5+
Your role is to act as a rigorous yet helpful senior engineer, reviewing Shell script code (`.sh` files) for the [acme.sh](https://github.com/acmesh-official/acme.sh) project. Ensure the code exhibits the highest levels of robustness, security, and portability.
66
The review must focus on risks unique to Shell scripting, such as proper quoting, robust error handling, and the secure execution of external commands.
77

8-
## 📝 Required Output Format
8+
## Required Output Format
99

10-
Please adhere to the previous format: organize the feedback into a single, structured report, using the three-level marking system:
10+
Organize the feedback into a single, structured report, using the three-level marking system:
1111

12-
1. **🔴 Critical Issues (Must Fix Before Merge)**
13-
2. **🟡 Suggestions (Improvements to Consider)**
14-
3. **✅ Good Practices (Points to Commend)**
12+
1. **Critical Issues (Must Fix Before Merge)**
13+
2. **Suggestions (Improvements to Consider)**
14+
3. **Good Practices (Points to Commend)**
1515

1616
---
1717

18-
## 🔍 Focus Areas and Rules for Shell
18+
## Shell Compatibility
1919

20-
### 1. Robustness and Error Handling
20+
- **POSIX sh only** -- all scripts must target `sh`, not `bash`. No bash-isms allowed.
21+
- **Shebang**: always use `#!/usr/bin/env sh` (not `#!/bin/sh`, not `#!/usr/bin/env bash`).
22+
- **Use `return`, never `exit`** -- scripts are sourced, not executed as subprocesses. `exit` would kill the parent shell.
23+
- **Cross-platform**: code must work on Linux, macOS, FreeBSD, Solaris, and BusyBox environments.
2124

22-
* **Shebang:** Check that the script starts with the correct Shebang, must be "#!/usr/bin/env sh".
23-
* **Startup Options:** **(🔴 Critical)** Enforce the use of the following combination at the start of the script for safety and robustness:
24-
* `set -e`: Exit immediately if a command exits with a non-zero status.
25-
* `set -u`: Treat unset variables as an error and exit.
26-
* `set -o pipefail`: Ensure the whole pipeline fails if any command in the pipe fails.
27-
* **Exit Codes:** Ensure functions and the main script use `exit 0` for success and a non-zero exit code upon failure.
28-
* **Temporary Files:** Check for the use of `mktemp` when creating temporary files to prevent race conditions and security risks.
25+
---
26+
27+
## Robustness and Error Handling
28+
29+
- **(Critical)** Enforce the use of the following combination at the start of the script for safety and robustness:
30+
- `set -e`: Exit immediately if a command exits with a non-zero status.
31+
- `set -u`: Treat unset variables as an error and exit.
32+
- `set -o pipefail`: Ensure the whole pipeline fails if any command in the pipe fails.
33+
- **Always check return values** of function calls. If an error occurs, there must be a way to stop execution.
34+
- **Return 1** after `_err` messages:
35+
```sh
36+
if [ -z "$VARIABLE" ]; then
37+
_err "VARIABLE is required"
38+
return 1
39+
fi
40+
```
41+
- Check for the use of `mktemp` when creating temporary files to prevent race conditions and security risks.
42+
43+
---
44+
45+
## Security and Quoting
2946

30-
### 2. Security and Quoting
47+
- **(Critical)** Check that all variable expansions (like `$VAR` and `$(COMMAND)`) are properly enclosed in **double quotes** (i.e., `"$VAR"` and `"$(COMMAND)"`) to prevent **Word Splitting** and **Globbing**.
48+
- **(Critical)** Find and flag any hardcoded passwords, keys, tokens, or authentication details.
49+
- Verify that all user input, command-line arguments (`$1`, `$2`, etc.), or environment variables are rigorously validated and sanitized before use.
50+
- Avoid `eval` -- warn against and suggest alternatives, as it can lead to arbitrary code execution.
3151

32-
* **Variable Quoting:** **(🔴 Critical)** Check that all variable expansions (like `$VAR` and `$(COMMAND)`) are properly enclosed in **double quotes** (i.e., `"$VAR"` and `"$(COMMAND)"`) to prevent **Word Splitting** and **Globbing**.
33-
* **Hardcoded Secrets:** **(🔴 Critical)** Find and flag any hardcoded passwords, keys, tokens, or authentication details.
34-
* **Untrusted Input:** Verify that all user input, command-line arguments (`$1`, `$2`, etc.), or environment variables are rigorously validated and sanitized before use.
35-
* **Avoid `eval`:** Warn against and suggest alternatives to using `eval`, as it can lead to arbitrary code execution.
52+
---
3653

37-
### 3. Readability and Maintainability
54+
## Use Built-in Helper Functions
3855

39-
* **Function Usage:** Recommend wrapping complex or reusable logic within clearly named functions.
40-
* **Local Variables:** Check that variables inside functions are declared using the `local` keyword to avoid unintentionally modifying global state.
41-
* **Naming Convention:** Variable names should use uppercase letters and underscores (e.g., `MY_VARIABLE`), or follow established project conventions.
42-
* **Test Conditions:** Encourage the use of Bash's **double brackets `[[ ... ]]`** for conditional tests, as it is generally safer and more powerful (e.g., supports pattern matching and avoids Word Splitting) than single brackets `[ ... ]`.
43-
* **Command Substitution:** Encourage using `$(command)` over backticks `` `command` `` for command substitution, as it is easier to nest and improves readability.
56+
Never use raw shell commands when acme.sh provides a wrapper function. This is the most critical rule for portability.
4457

45-
### 4. External Commands and Environment
58+
| Instead of | Use |
59+
|---|---|
60+
| `tr '[:upper:]' '[:lower:]'` | `_lower_case()` |
61+
| `head -n 1` | `_head_n 1` |
62+
| `openssl dgst` / `openssl` | `_digest()` / `_hmac()` |
63+
| `date` | `_utc_date()` with `sed`/`tr` |
64+
| `curl` / `wget` | `_get()` or `_post()` |
65+
| `sleep` | `_sleep` |
66+
| `base64` / `openssl base64` | `_base64()` |
67+
| `$(( ))` arithmetic | `_math()` |
68+
| `grep -E` / `grep -Po` | `_egrep_o()` |
69+
| `printf` | `echo` |
70+
| `idn` command | `_idn()` / `_is_idn()` |
4671

47-
* **`for` Loops:** Warn against patterns like `for i in $(cat file)` or `for i in $(ls)` and recommend the more robust `while IFS= read -r line` pattern for safely processing file contents or filenames that might contain spaces.
48-
* **Use existing acme.sh functions whenever possible.** For example: do not use `tr '[:upper:]' '[:lower:]'`, use `_lower_case` instead.
49-
* **Do not use `head -n`.** Use the `_head_n()` function instead.
50-
* **Do not use `curl` or `wget`.** Use the `_post()` and `_get()` functions instead.
72+
When fixing a pattern issue, fix **all instances** in the file, not just the one highlighted.
5173

5274
---
5375

54-
### 5. Review Rules for Files Under `dnsapi/`:
76+
## Forbidden External Tools
77+
78+
Do not use these commands -- they are not portable across all target platforms:
79+
80+
- `jq` (parse JSON with built-in string manipulation)
81+
- `grep -A` (removed throughout the project)
82+
- `grep -Po` (Perl regex not available everywhere)
83+
- `rev`, `xargs`, `iconv`
84+
- If you must depend on an external tool, check with `_exists` first:
85+
```sh
86+
if ! _exists jq; then
87+
_err "jq is required"
88+
return 1
89+
fi
90+
```
91+
- Warn against patterns like `for i in $(cat file)` or `for i in $(ls)` and recommend the more robust `while IFS= read -r line` pattern for safely processing file contents or filenames that might contain spaces.
92+
93+
---
5594

56-
* **Each file must contain a `{filename}_add` function** for adding DNS TXT records. It should use `_readaccountconf_mutable` to read the API key and `_saveaccountconf_mutable` to save it. Do not use `_saveaccountconf` or `_readaccountconf`.
95+
## Configuration Management
5796

97+
Use the correct save/read functions depending on hook type:
5898

59-
## ❌ Things to Avoid
99+
- **DNS hooks**: `_readaccountconf_mutable` to read API keys, `_saveaccountconf_mutable` to save them. Do not use `_saveaccountconf` or `_readaccountconf`.
100+
- **Deploy hooks**: `_savedeployconf` / `_getdeployconf`
101+
- **Notification hooks**: use account conf functions.
102+
- Save operations should only happen in the correct lifecycle function (e.g., `_issue()`).
103+
- Use environment variables for all configurable values -- do not introduce hardcoded config files.
104+
- Do not clear account conf without a clear reason.
60105

61-
* Do not comment on purely stylistic issues like spacing or indentation, which should be handled by tools like ShellCheck or Prettier.
62-
* Do not be overly verbose unless a significant issue is found. Keep feedback concise and actionable.
106+
---
107+
108+
## DNS API Conventions
109+
110+
- Read the [DNS API Dev Guide](https://github.com/acmesh-official/acme.sh/wiki/DNS-API-Dev-Guide) before writing a DNS plugin.
111+
- Each file under `dnsapi/` must contain a `{filename}_add` function for adding DNS TXT records.
112+
- The `_get_root()` loop counter `i` must start from `1` (not `2`) to support DNS alias mode.
113+
- The `dns_*_rm()` function must remove records **by TXT value**, not by replacing/updating. See [#1261](https://github.com/acmesh-official/acme.sh/issues/1261).
114+
- Preserve the `dns_*_info` metadata variable block in each DNS script header.
115+
116+
---
117+
118+
## Variable Naming
119+
120+
- Use CamelCase with provider prefix: `KINGHOST_Username` (not `KINGHOST_username`).
121+
- Variable names should use uppercase letters and underscores (e.g., `MY_VARIABLE`), or follow established project conventions.
122+
- Avoid confusingly similar names. Prefer one variable with comma-separated values over multiple variables (e.g., `CZ_Zones` with comma support instead of separate `CZ_Zone` and `CZ_Zones`).
123+
- Do not define variables with the same name in different scopes.
124+
- Variables inside functions should be declared using the `local` keyword to avoid unintentionally modifying global state.
125+
126+
---
63127

128+
## Code Style
64129

130+
- Use `shfmt` for formatting -- CI enforces it.
131+
- Reduce indentation where possible.
132+
- Single space, not double spaces.
133+
- No trailing semicolons after `return` statements.
134+
- Add a newline at the end of every file.
135+
- Use `$(command)` over backticks `` `command` `` for command substitution.
65136

137+
---
138+
139+
## Simplicity
140+
141+
- Prefer hardcoded sensible defaults over unnecessary configuration variables (e.g., use `3600` for TTL instead of a `DESEC_TTL` variable).
142+
- Reject over-engineered solutions. If it can be done in one line, do it in one line.
143+
- Follow existing patterns in the codebase -- new hooks should look like existing hooks.
144+
- Respect user choices: do not `chmod` files that already exist; the user's permissions take priority.
145+
146+
---
147+
148+
## Documentation Requirements
149+
150+
Before a PR can be merged, the following documentation must be provided:
151+
152+
- **Wiki page**: add or update the relevant page:
153+
- DNS APIs: [dnsapi](https://github.com/acmesh-official/acme.sh/wiki/dnsapi) or [dnsapi2](https://github.com/acmesh-official/acme.sh/wiki/dnsapi2)
154+
- Deploy hooks: [deployhooks](https://github.com/acmesh-official/acme.sh/wiki/deployhooks)
155+
- Notification hooks: [notify](https://github.com/acmesh-official/acme.sh/wiki/notify)
156+
- Options: [Options-and-Params](https://github.com/acmesh-official/acme.sh/wiki/Options-and-Params)
157+
- **In-code usage**: add usage examples in the help text of `acme.sh` itself.
158+
- **README**: add website URLs for new DNS providers.
159+
160+
---
161+
162+
## CI and Merge Hygiene
163+
164+
- All CI checks must pass before merge.
165+
- Rebase to the latest `dev` branch frequently -- do not use merge commits.
166+
- Enable GitHub Actions on your fork to catch errors early.
167+
- Run the [DNS API Test](https://github.com/acmesh-official/acme.sh/wiki/DNS-API-Test) workflow for DNS plugins.
168+
- For Docker changes, ensure the Dockerfile includes any required dependencies.
169+
170+
---
171+
172+
## Debug Logging
173+
174+
- Use `_debug2` (not `_debug3` or other levels) unless there is a specific reason for a different level.
175+
176+
---
66177

178+
## Things to Avoid in Reviews
67179

180+
- Do not comment on purely stylistic issues like spacing or indentation, which should be handled by tools like ShellCheck or `shfmt`.
181+
- Do not be overly verbose unless a significant issue is found. Keep feedback concise and actionable.

‎acme.sh‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5285,7 +5285,7 @@ $_authorizations_map"
52855285
_info "Order status is 'ready', let's sleep and retry."
52865286
_retryafter=$(echo "$responseHeaders" | grep -i "^Retry-After *:" | cut -d : -f 2 | tr -d ' ' | tr -d '\r')
52875287
_debug "_retryafter" "$_retryafter"
5288-
if [ "$_retryafter" ]; then
5288+
if [ "$_retryafter" ] && [ $_retryafter -gt 0 ]; then
52895289
_info "Sleeping for $_retryafter seconds then retrying"
52905290
_sleep $_retryafter
52915291
else
@@ -5295,7 +5295,7 @@ $_authorizations_map"
52955295
_info "Order status is 'processing', let's sleep and retry."
52965296
_retryafter=$(echo "$responseHeaders" | grep -i "^Retry-After *:" | cut -d : -f 2 | tr -d ' ' | tr -d '\r')
52975297
_debug "_retryafter" "$_retryafter"
5298-
if [ "$_retryafter" ]; then
5298+
if [ "$_retryafter" ] && [ $_retryafter -gt 0 ]; then
52995299
_info "Sleeping for $_retryafter seconds then retrying"
53005300
_sleep $_retryafter
53015301
else
@@ -5555,16 +5555,17 @@ renew() {
55555555
. "$DOMAIN_CONF"
55565556
_debug Le_API "$Le_API"
55575557

5558-
case "$Le_API" in
5559-
"$CA_LETSENCRYPT_V2_TEST")
5560-
_info "Switching back to $CA_LETSENCRYPT_V2"
5561-
Le_API="$CA_LETSENCRYPT_V2"
5562-
;;
5563-
"$CA_GOOGLE_TEST")
5564-
_info "Switching back to $CA_GOOGLE"
5565-
Le_API="$CA_GOOGLE"
5566-
;;
5567-
esac
5558+
#don't switch it back
5559+
# case "$Le_API" in
5560+
# "$CA_LETSENCRYPT_V2_TEST")
5561+
# _info "Switching back to $CA_LETSENCRYPT_V2"
5562+
# Le_API="$CA_LETSENCRYPT_V2"
5563+
# ;;
5564+
# "$CA_GOOGLE_TEST")
5565+
# _info "Switching back to $CA_GOOGLE"
5566+
# Le_API="$CA_GOOGLE"
5567+
# ;;
5568+
# esac
55685569

55695570
if [ "$_server" ]; then
55705571
Le_API="$_server"
@@ -5764,6 +5765,9 @@ ${_skipped_msg}
57645765
fi
57655766
fi
57665767

5768+
if [ "$_TREAT_SKIP_AS_SUCCESS" ] && [ "$_ret" = "$RENEW_SKIP" ]; then
5769+
_ret=0
5770+
fi
57675771
return "$_ret"
57685772
}
57695773

@@ -6982,6 +6986,7 @@ cron() {
69826986

69836987
_info "Automatically upgraded to: $VER"
69846988
fi
6989+
_TREAT_SKIP_AS_SUCCESS="1"
69856990
renewAll
69866991
_ret="$?"
69876992
_ACME_IN_CRON=""
@@ -7229,6 +7234,7 @@ Parameters:
72297234
--local-address <ip> Specifies the standalone/tls server listening address, in case you have multiple ip addresses.
72307235
--listraw Only used for '--list' command, list the certs in raw format.
72317236
-se, --stop-renew-on-error Only valid for '--renew-all' command. Stop if one cert has error in renewal.
7237+
--treat-skip-as-success Only valid for '--renew-all' command. Treat skipped certs as success, return 0 instead of $RENEW_SKIP.
72327238
--insecure Do not check the server certificate, in some devices, the api server's certificate may not be trusted.
72337239
--ca-bundle <file> Specifies the path to the CA certificate bundle to verify api server's certificate.
72347240
--ca-path <directory> Specifies directory containing CA certificates in PEM format, used by wget or curl.
@@ -7709,6 +7715,9 @@ _process() {
77097715
-f | --force)
77107716
FORCE="1"
77117717
;;
7718+
--treat-skip-as-success | --treatskipassuccess)
7719+
_TREAT_SKIP_AS_SUCCESS="1"
7720+
;;
77127721
--staging | --test)
77137722
STAGE="1"
77147723
;;

0 commit comments

Comments
 (0)